Re: [PATCH 3/3] nbd: introduce a client flag to do zero timeout handling

[Hou Pu <houpu@bytedance.com>]

On 2020/9/22 2:03 AM, Josef Bacik wrote:
> On 9/21/20 6:57 AM, Hou Pu wrote:
> > Introduce a dedicated client flag NBD_RT_WAIT_ON_TIMEOUT to reset
> > timer in nbd_xmit_timer instead of depending on tag_set.timeout == 0.
> > So that the timeout value could be configured by the user to
> > whatever they like instead of the default 30s. A smaller timeout
> > value allow us to detect if a new socket is reconfigured in a
> > shorter time. Thus the io could be requeued more quickly.
> >
> > The tag_set.timeout == 0 setting still works like before.
> >
> > Signed-off-by: Hou Pu <houpu@bytedance.com>
>
> I like this idea in general, just a few comments below.

Thanks,
Hou

> > ---
> >   drivers/block/nbd.c      | 25 ++++++++++++++++++++++++-
> >   include/uapi/linux/nbd.h |  4 ++++
> >   2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 4c0bbb981cbc..1d7a9de7bdcc 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -80,6 +80,7 @@ struct link_dead_args {
> >   #define NBD_RT_BOUND            5
> >   #define NBD_RT_DESTROY_ON_DISCONNECT    6
> >   #define NBD_RT_DISCONNECT_ON_CLOSE    7
> > +#define NBD_RT_WAIT_ON_TIMEOUT        8
> >   #define NBD_DESTROY_ON_DISCONNECT    0
> >   #define NBD_DISCONNECT_REQUESTED    1
> > @@ -378,6 +379,16 @@ static u32 req_to_nbd_cmd_type(struct request *req)
> >       }
> >   }
> > +static inline bool wait_on_timeout(struct nbd_device *nbd,
> > +                   struct nbd_config *config)
> > +{
> > +    if (test_bit(NBD_RT_WAIT_ON_TIMEOUT, &config->runtime_flags) ||
> > +        nbd->tag_set.timeout == 0)
> > +        return true;
> > +    else
> > +        return false;
> > +}
> > +
>
> This obviously needs to be updated to match my comments from the 
> previous patch.

Thanks. Please see v2 latter.

> >   static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
> >                            bool reserved)
> >   {
> > @@ -400,7 +411,7 @@ static enum blk_eh_timer_return 
> > nbd_xmit_timeout(struct request *req,
> >           nbd->tag_set.timeout)
> >           goto error_out;
> > -    if (nbd->tag_set.timeout) {
> > +    if (!wait_on_timeout(nbd, config)) {
> >           dev_err_ratelimited(nbd_to_dev(nbd),
> >                       "Connection timed out, retrying (%d/%d alive)\n",
> >                       atomic_read(&config->live_connections),
> > @@ -1953,6 +1964,10 @@ static int nbd_genl_connect(struct sk_buff 
> > *skb, struct genl_info *info)
> >               set_bit(NBD_RT_DISCONNECT_ON_CLOSE,
> >                   &config->runtime_flags);
> >           }
> > +        if (flags & NBD_CFLAG_WAIT_ON_TIMEOUT) {
> > +            set_bit(NBD_RT_WAIT_ON_TIMEOUT,
> > +                &config->runtime_flags);
> > +        }
>
> Documentation/process/coding-style.rst
>
> "Do not unnecessarily use braces where a single statement will do."
>
> same goes for below.  Thanks,
>
> Josef

Thanks. Will fix this in v2.


Thanks,
Hou