| Message ID | 20200921105718.29006-4-houpu@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: improve timeout handling and a fix | expand |
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. > --- > 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. > 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
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
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; +} + 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); + } } if (info->attrs[NBD_ATTR_SOCKETS]) { @@ -2136,6 +2151,14 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info) clear_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); + } else { + clear_bit(NBD_RT_WAIT_ON_TIMEOUT, + &config->runtime_flags); + } + } if (info->attrs[NBD_ATTR_SOCKETS]) { diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h index 20d6cc91435d..cc3abfb92de5 100644 --- a/include/uapi/linux/nbd.h +++ b/include/uapi/linux/nbd.h @@ -56,6 +56,10 @@ enum { #define NBD_CFLAG_DISCONNECT_ON_CLOSE (1 << 1) /* disconnect the nbd device on * close by last opener. */ +#define NBD_CFLAG_WAIT_ON_TIMEOUT (1 << 2) /* do not fallback to other sockets + * in io timeout, instead just reset + * timer and wait. + */ /* userspace doesn't need the nbd_device structure */
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> --- drivers/block/nbd.c | 25 ++++++++++++++++++++++++- include/uapi/linux/nbd.h | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)