[PATCH v1 0/2] nbd: reset the queue/io_timeout to default on disconnect

[PATCH v1 0/2] nbd: reset the queue/io_timeout to default on disconnect

[pkalever]

From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

Hi,

This series has changes to reset the queue/io_timeout for nbd devices and
a cleanup patch.

Thank you!

Prasanna Kumar Kalever (2):
  block: cleanup: define default command timeout and use it
  nbd: reset the queue/io_timeout to default on disconnect

 block/blk-mq.c         | 2 +-
 drivers/block/nbd.c    | 9 +++++++--
 include/linux/blkdev.h | 2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH v1 1/2] block: cleanup: define default command timeout and use it

[pkalever]

From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

defined BLK_DEFAULT_CMD_TIMEOUT and reuse it everywhere else.

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
 block/blk-mq.c         | 2 +-
 drivers/block/nbd.c    | 2 +-
 include/linux/blkdev.h | 2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2c4ac51e54eb..1cba91eca6ee 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3289,7 +3289,7 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 		goto err_hctxs;
 
 	INIT_WORK(&q->timeout_work, blk_mq_timeout_work);
-	blk_queue_rq_timeout(q, set->timeout ? set->timeout : 30 * HZ);
+	blk_queue_rq_timeout(q, set->timeout ? set->timeout : BLK_DEFAULT_CMD_TIMEOUT);
 
 	q->tag_set = set;
 
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c38317979f74..16a1a14b1fd1 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1376,7 +1376,7 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
 	if (timeout)
 		blk_queue_rq_timeout(nbd->disk->queue, timeout * HZ);
 	else
-		blk_queue_rq_timeout(nbd->disk->queue, 30 * HZ);
+		blk_queue_rq_timeout(nbd->disk->queue, BLK_DEFAULT_CMD_TIMEOUT);
 }
 
 /* Must be called with config_lock held */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d3afea47ade6..e50a9a5356d3 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -378,6 +378,8 @@ static inline int blkdev_zone_mgmt_ioctl(struct block_device *bdev,
 
 #endif /* CONFIG_BLK_DEV_ZONED */
 
+#define BLK_DEFAULT_CMD_TIMEOUT	(30*HZ) /* 30 seconds */
+
 struct request_queue {
 	struct request		*last_merge;
 	struct elevator_queue	*elevator;
-- 
2.31.1

Re: [PATCH v1 1/2] block: cleanup: define default command timeout and use it

[Ming Lei]

On Fri, Aug 06, 2021 at 07:59:13PM +0530, pkalever@redhat.com wrote:
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> 
> defined BLK_DEFAULT_CMD_TIMEOUT and reuse it everywhere else.
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH v1 1/2] block: cleanup: define default command timeout and use it

[Prasanna Kalever]

On Thu, Sep 16, 2021 at 1:52 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Fri, Aug 06, 2021 at 07:59:13PM +0530, pkalever@redhat.com wrote:
> > From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> >
> > defined BLK_DEFAULT_CMD_TIMEOUT and reuse it everywhere else.
> >
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > ---
>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks for the review Ming.

Attempting to bring this to the top again for more reviews/acks.


BRs,
--
Prasanna

>
> --
> Ming
>

Re: [PATCH v1 1/2] block: cleanup: define default command timeout and use it

[Prasanna Kalever]

On Wed, Oct 27, 2021 at 7:48 AM Prasanna Kalever <pkalever@redhat.com> wrote:
>
> On Thu, Sep 16, 2021 at 1:52 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Fri, Aug 06, 2021 at 07:59:13PM +0530, pkalever@redhat.com wrote:
> > > From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > >
> > > defined BLK_DEFAULT_CMD_TIMEOUT and reuse it everywhere else.
> > >
> > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > > ---
> >
> > Reviewed-by: Ming Lei <ming.lei@redhat.com>
>
> Thanks for the review Ming.
>
> Attempting to bring this to the top again for more reviews/acks.

oops! please ignore, this is the wrong thread.

>
>
> BRs,
> --
> Prasanna
>
> >
> > --
> > Ming
> >

[PATCH v1 2/2] nbd: reset the queue/io_timeout to default on disconnect

[pkalever]

From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

Without any changes to NBD_ATTR_TIMEOUT (default is 30 secs),
$ rbd-nbd map rbd-pool/image0 --try-netlink
/dev/nbd0
$ cat /sys/block/nbd0/queue/io_timeout
30000
$ rbd-nbd unmap /dev/nbd0
$ cat /sys/block/nbd0/queue/io_timeout
30000

Now user sets NBD_ATTR_TIMEOUT to 60,
$ rbd-nbd map rbd-pool/image0 --try-netlink --io-timeout 60
/dev/nbd0
$ cat /sys/block/nbd0/queue/io_timeout
60000
$ rbd-nbd unmap /dev/nbd0
$ cat /sys/block/nbd0/queue/io_timeout
60000

Now user doesn't alter NBD_ATTR_TIMEOUT, but sysfs still shows it as 60,
$ rbd-nbd map rbd-pool/image0 --try-netlink
/dev/nbd0
$ cat /sys/block/nbd0/queue/io_timeout
60000
$ rbd-nbd unmap /dev/nbd0
$ cat /sys/block/nbd0/queue/io_timeout
60000

The problem exists with ioctl interface too.

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
 drivers/block/nbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 16a1a14b1fd1..a45aabc4914b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -158,6 +158,7 @@ static void nbd_connect_reply(struct genl_info *info, int index);
 static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info);
 static void nbd_dead_link_work(struct work_struct *work);
 static void nbd_disconnect_and_put(struct nbd_device *nbd);
+static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout);
 
 static inline struct device *nbd_to_dev(struct nbd_device *nbd)
 {
@@ -1250,7 +1251,7 @@ static void nbd_config_put(struct nbd_device *nbd)
 			destroy_workqueue(nbd->recv_workq);
 		nbd->recv_workq = NULL;
 
-		nbd->tag_set.timeout = 0;
+		nbd_set_cmd_timeout(nbd, 0);
 		nbd->disk->queue->limits.discard_granularity = 0;
 		nbd->disk->queue->limits.discard_alignment = 0;
 		blk_queue_max_discard_sectors(nbd->disk->queue, UINT_MAX);
@@ -2124,6 +2125,10 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
 	if (ret)
 		goto out;
 
+	/*
+	 * On reconfigure, if NBD_ATTR_TIMEOUT is not provided, we will
+	 * continue to use the cmd timeout provided with connect initially.
+	 */
 	if (info->attrs[NBD_ATTR_TIMEOUT])
 		nbd_set_cmd_timeout(nbd,
 				    nla_get_u64(info->attrs[NBD_ATTR_TIMEOUT]));
-- 
2.31.1

Re: [PATCH v1 2/2] nbd: reset the queue/io_timeout to default on disconnect

[Ming Lei]

On Fri, Aug 06, 2021 at 07:59:14PM +0530, pkalever@redhat.com wrote:
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> 
> Without any changes to NBD_ATTR_TIMEOUT (default is 30 secs),
> $ rbd-nbd map rbd-pool/image0 --try-netlink
> /dev/nbd0
> $ cat /sys/block/nbd0/queue/io_timeout
> 30000
> $ rbd-nbd unmap /dev/nbd0
> $ cat /sys/block/nbd0/queue/io_timeout
> 30000
> 
> Now user sets NBD_ATTR_TIMEOUT to 60,
> $ rbd-nbd map rbd-pool/image0 --try-netlink --io-timeout 60
> /dev/nbd0
> $ cat /sys/block/nbd0/queue/io_timeout
> 60000
> $ rbd-nbd unmap /dev/nbd0
> $ cat /sys/block/nbd0/queue/io_timeout
> 60000
> 
> Now user doesn't alter NBD_ATTR_TIMEOUT, but sysfs still shows it as 60,
> $ rbd-nbd map rbd-pool/image0 --try-netlink
> /dev/nbd0
> $ cat /sys/block/nbd0/queue/io_timeout
> 60000
> $ rbd-nbd unmap /dev/nbd0
> $ cat /sys/block/nbd0/queue/io_timeout
> 60000
> 
> The problem exists with ioctl interface too.
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---
>  drivers/block/nbd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 16a1a14b1fd1..a45aabc4914b 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -158,6 +158,7 @@ static void nbd_connect_reply(struct genl_info *info, int index);
>  static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info);
>  static void nbd_dead_link_work(struct work_struct *work);
>  static void nbd_disconnect_and_put(struct nbd_device *nbd);
> +static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout);
>  
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> @@ -1250,7 +1251,7 @@ static void nbd_config_put(struct nbd_device *nbd)
>  			destroy_workqueue(nbd->recv_workq);
>  		nbd->recv_workq = NULL;
>  
> -		nbd->tag_set.timeout = 0;
> +		nbd_set_cmd_timeout(nbd, 0);
>  		nbd->disk->queue->limits.discard_granularity = 0;
>  		nbd->disk->queue->limits.discard_alignment = 0;
>  		blk_queue_max_discard_sectors(nbd->disk->queue, UINT_MAX);
> @@ -2124,6 +2125,10 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
>  	if (ret)
>  		goto out;
>  
> +	/*
> +	 * On reconfigure, if NBD_ATTR_TIMEOUT is not provided, we will
> +	 * continue to use the cmd timeout provided with connect initially.
> +	 */
>  	if (info->attrs[NBD_ATTR_TIMEOUT])
>  		nbd_set_cmd_timeout(nbd,
>  				    nla_get_u64(info->attrs[NBD_ATTR_TIMEOUT]));
> -- 
> 2.31.1
> 

Looks fine:

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH v1 2/2] nbd: reset the queue/io_timeout to default on disconnect

[Prasanna Kalever]

On Thu, Sep 16, 2021 at 1:53 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Fri, Aug 06, 2021 at 07:59:14PM +0530, pkalever@redhat.com wrote:
> > From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> >
> > Without any changes to NBD_ATTR_TIMEOUT (default is 30 secs),
> > $ rbd-nbd map rbd-pool/image0 --try-netlink
> > /dev/nbd0
> > $ cat /sys/block/nbd0/queue/io_timeout
> > 30000
> > $ rbd-nbd unmap /dev/nbd0
> > $ cat /sys/block/nbd0/queue/io_timeout
> > 30000
> >
> > Now user sets NBD_ATTR_TIMEOUT to 60,
> > $ rbd-nbd map rbd-pool/image0 --try-netlink --io-timeout 60
> > /dev/nbd0
> > $ cat /sys/block/nbd0/queue/io_timeout
> > 60000
> > $ rbd-nbd unmap /dev/nbd0
> > $ cat /sys/block/nbd0/queue/io_timeout
> > 60000
> >
> > Now user doesn't alter NBD_ATTR_TIMEOUT, but sysfs still shows it as 60,
> > $ rbd-nbd map rbd-pool/image0 --try-netlink
> > /dev/nbd0
> > $ cat /sys/block/nbd0/queue/io_timeout
> > 60000
> > $ rbd-nbd unmap /dev/nbd0
> > $ cat /sys/block/nbd0/queue/io_timeout
> > 60000
> >
> > The problem exists with ioctl interface too.
> >
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > ---
> >  drivers/block/nbd.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 16a1a14b1fd1..a45aabc4914b 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -158,6 +158,7 @@ static void nbd_connect_reply(struct genl_info *info, int index);
> >  static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info);
> >  static void nbd_dead_link_work(struct work_struct *work);
> >  static void nbd_disconnect_and_put(struct nbd_device *nbd);
> > +static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout);
> >
> >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
> >  {
> > @@ -1250,7 +1251,7 @@ static void nbd_config_put(struct nbd_device *nbd)
> >                       destroy_workqueue(nbd->recv_workq);
> >               nbd->recv_workq = NULL;
> >
> > -             nbd->tag_set.timeout = 0;
> > +             nbd_set_cmd_timeout(nbd, 0);
> >               nbd->disk->queue->limits.discard_granularity = 0;
> >               nbd->disk->queue->limits.discard_alignment = 0;
> >               blk_queue_max_discard_sectors(nbd->disk->queue, UINT_MAX);
> > @@ -2124,6 +2125,10 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
> >       if (ret)
> >               goto out;
> >
> > +     /*
> > +      * On reconfigure, if NBD_ATTR_TIMEOUT is not provided, we will
> > +      * continue to use the cmd timeout provided with connect initially.
> > +      */
> >       if (info->attrs[NBD_ATTR_TIMEOUT])
> >               nbd_set_cmd_timeout(nbd,
> >                                   nla_get_u64(info->attrs[NBD_ATTR_TIMEOUT]));
> > --
> > 2.31.1
> >
>
> Looks fine:
>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks for the review Ming.
Attempting to bring this to the top again for more reviews/acks.


Thanks!
--
Prasanna


>
> --
> Ming
>

Re: [PATCH v1 0/2] nbd: reset the queue/io_timeout to default on disconnect

[Prasanna Kalever]

Hello,

Just wanted to bring this up again and remind you about the pending review.

Many Thanks,
--
Prasanna

On Fri, Aug 6, 2021 at 7:59 PM <pkalever@redhat.com> wrote:
>
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
>
> Hi,
>
> This series has changes to reset the queue/io_timeout for nbd devices and
> a cleanup patch.
>
> Thank you!
>
> Prasanna Kumar Kalever (2):
>   block: cleanup: define default command timeout and use it
>   nbd: reset the queue/io_timeout to default on disconnect
>
>  block/blk-mq.c         | 2 +-
>  drivers/block/nbd.c    | 9 +++++++--
>  include/linux/blkdev.h | 2 ++
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> --
> 2.31.1
>

Re: [PATCH v1 0/2] nbd: reset the queue/io_timeout to default on disconnect

[Xiubo Li]

On 8/6/21 10:29 PM, pkalever@redhat.com wrote:
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
>
> Hi,
>
> This series has changes to reset the queue/io_timeout for nbd devices and
> a cleanup patch.
>
> Thank you!
>
> Prasanna Kumar Kalever (2):
>    block: cleanup: define default command timeout and use it
>    nbd: reset the queue/io_timeout to default on disconnect
>
>   block/blk-mq.c         | 2 +-
>   drivers/block/nbd.c    | 9 +++++++--
>   include/linux/blkdev.h | 2 ++
>   3 files changed, 10 insertions(+), 3 deletions(-)
>
This series LGTM.

Reviewed-by: Xiubo Li <xiubli@redhat.com>