linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [resend v1 0/5] Add support for block disk resize notification
@ 2020-01-02  7:53 Balbir Singh
  2020-01-02  7:53 ` [resend v1 1/5] block/genhd: Notify udev about capacity change Balbir Singh
                   ` (5 more replies)
  0 siblings, 6 replies; 38+ messages in thread
From: Balbir Singh @ 2020-01-02  7:53 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni, Balbir Singh

Allow block/genhd to notify user space about disk size changes
using a new helper disk_set_capacity(), which is a wrapper on top
of set_capacity(). disk_set_capacity() will only notify if
the current capacity or the target capacity is not zero.

Background:

As a part of a patch to allow sending the RESIZE event on disk capacity
change, Christoph (hch@lst.de) requested that the patch be made generic
and the hacks for virtio block and xen block devices be removed and
merged via a generic helper.

This series consists of 5 changes. The first one adds the basic
support for changing the size and notifying. The follow up patches
are per block subsystem changes. Other block drivers can add their
changes as necessary on top of this series.

Testing:
1. I did some basic testing with an NVME device, by resizing it in
the backend and ensured that udevd received the event.

NOTE: After these changes, the notification might happen before
revalidate disk, where as it occured later before.

Balbir Singh (5):
  block/genhd: Notify udev about capacity change
  drivers/block/virtio_blk.c: Convert to use disk_set_capacity
  drivers/block/xen-blkfront.c: Convert to use disk_set_capacity
  drivers/nvme/host/core.c: Convert to use disk_set_capacity
  drivers/scsi/sd.c: Convert to use disk_set_capacity

 block/genhd.c                | 19 +++++++++++++++++++
 drivers/block/virtio_blk.c   |  4 +---
 drivers/block/xen-blkfront.c |  5 +----
 drivers/nvme/host/core.c     |  2 +-
 drivers/scsi/sd.c            |  2 +-
 include/linux/genhd.h        |  1 +
 6 files changed, 24 insertions(+), 9 deletions(-)

-- 
2.16.5


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
@ 2020-01-02  7:53 ` Balbir Singh
  2020-01-03  6:16   ` Chaitanya Kulkarni
  2020-01-07  3:32   ` Martin K. Petersen
  2020-01-02  7:53 ` [resend v1 2/5] drivers/block/virtio_blk.c: Convert to use disk_set_capacity Balbir Singh
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 38+ messages in thread
From: Balbir Singh @ 2020-01-02  7:53 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni, Balbir Singh

Allow block/genhd to notify user space (via udev) about disk size changes
using a new helper disk_set_capacity(), which is a wrapper on top
of set_capacity(). disk_set_capacity() will only notify via udev if
the current capacity or the target capacity is not zero.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Someswarudu Sangaraju <ssomesh@amazon.com>
Signed-off-by: Balbir Singh <sblbir@amazon.com>
---
 block/genhd.c         | 19 +++++++++++++++++++
 include/linux/genhd.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..94faec98607b 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -46,6 +46,25 @@ static void disk_add_events(struct gendisk *disk);
 static void disk_del_events(struct gendisk *disk);
 static void disk_release_events(struct gendisk *disk);
 
+/*
+ * Set disk capacity and notify if the size is not currently
+ * zero and will not be set to zero
+ */
+void disk_set_capacity(struct gendisk *disk, sector_t size)
+{
+	sector_t capacity = get_capacity(disk);
+
+	set_capacity(disk, size);
+	if (capacity != 0 && size != 0) {
+		char *envp[] = { "RESIZE=1", NULL };
+
+		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
+	}
+}
+
+EXPORT_SYMBOL_GPL(disk_set_capacity);
+
+
 void part_inc_in_flight(struct request_queue *q, struct hd_struct *part, int rw)
 {
 	if (queue_is_mq(q))
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index a927829bb73a..f1a5ddcc781d 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -449,6 +449,7 @@ static inline int get_disk_ro(struct gendisk *disk)
 extern void disk_block_events(struct gendisk *disk);
 extern void disk_unblock_events(struct gendisk *disk);
 extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
+extern void disk_set_capacity(struct gendisk *disk, sector_t size);
 extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
 
 /* drivers/char/random.c */
-- 
2.16.5


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [resend v1 2/5] drivers/block/virtio_blk.c: Convert to use disk_set_capacity
  2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
  2020-01-02  7:53 ` [resend v1 1/5] block/genhd: Notify udev about capacity change Balbir Singh
@ 2020-01-02  7:53 ` Balbir Singh
  2020-01-02  7:53 ` [resend v1 3/5] drivers/block/xen-blkfront.c: " Balbir Singh
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 38+ messages in thread
From: Balbir Singh @ 2020-01-02  7:53 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni, Balbir Singh

block/genhd provides disk_set_capacity() for sending
RESIZE notifications via uevents.

Signed-off-by: Balbir Singh <sblbir@amazon.com>
---
 drivers/block/virtio_blk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index fbbf18ac1d5d..9848c94a7eb4 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -479,18 +479,16 @@ static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize)
 		   cap_str_10,
 		   cap_str_2);
 
-	set_capacity(vblk->disk, capacity);
+	disk_set_capacity(vblk->disk, capacity);
 }
 
 static void virtblk_config_changed_work(struct work_struct *work)
 {
 	struct virtio_blk *vblk =
 		container_of(work, struct virtio_blk, config_work);
-	char *envp[] = { "RESIZE=1", NULL };
 
 	virtblk_update_capacity(vblk, true);
 	revalidate_disk(vblk->disk);
-	kobject_uevent_env(&disk_to_dev(vblk->disk)->kobj, KOBJ_CHANGE, envp);
 }
 
 static void virtblk_config_changed(struct virtio_device *vdev)
-- 
2.16.5


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [resend v1 3/5] drivers/block/xen-blkfront.c: Convert to use disk_set_capacity
  2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
  2020-01-02  7:53 ` [resend v1 1/5] block/genhd: Notify udev about capacity change Balbir Singh
  2020-01-02  7:53 ` [resend v1 2/5] drivers/block/virtio_blk.c: Convert to use disk_set_capacity Balbir Singh
@ 2020-01-02  7:53 ` Balbir Singh
  2020-01-02  7:53 ` [resend v1 4/5] drivers/nvme/host/core.c: " Balbir Singh
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 38+ messages in thread
From: Balbir Singh @ 2020-01-02  7:53 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni, Balbir Singh

block/genhd provides disk_set_capacity() for sending
RESIZE notifications via uevents.

Signed-off-by: Balbir Singh <sblbir@amazon.com>
---
 drivers/block/xen-blkfront.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 23c86350a5ab..bed7050a697d 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2337,7 +2337,6 @@ static void blkfront_connect(struct blkfront_info *info)
 	unsigned long sector_size;
 	unsigned int physical_sector_size;
 	unsigned int binfo;
-	char *envp[] = { "RESIZE=1", NULL };
 	int err, i;
 
 	switch (info->connected) {
@@ -2352,10 +2351,8 @@ static void blkfront_connect(struct blkfront_info *info)
 			return;
 		printk(KERN_INFO "Setting capacity to %Lu\n",
 		       sectors);
-		set_capacity(info->gd, sectors);
+		disk_set_capacity(info->gd, sectors);
 		revalidate_disk(info->gd);
-		kobject_uevent_env(&disk_to_dev(info->gd)->kobj,
-				   KOBJ_CHANGE, envp);
 
 		return;
 	case BLKIF_STATE_SUSPENDED:
-- 
2.16.5


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
                   ` (2 preceding siblings ...)
  2020-01-02  7:53 ` [resend v1 3/5] drivers/block/xen-blkfront.c: " Balbir Singh
@ 2020-01-02  7:53 ` Balbir Singh
  2020-01-04 22:27   ` Chaitanya Kulkarni
  2020-01-02  7:53 ` [resend v1 5/5] drivers/scsi/sd.c: " Balbir Singh
  2020-01-06  5:59 ` [resend v1 0/5] Add support for block disk resize notification Bob Liu
  5 siblings, 1 reply; 38+ messages in thread
From: Balbir Singh @ 2020-01-02  7:53 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni, Balbir Singh

block/genhd provides disk_set_capacity() for sending
RESIZE notifications via uevents. This notification is
newly added to NVME devices

Signed-off-by: Balbir Singh <sblbir@amazon.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 667f18f465be..cb214e914fc2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1808,7 +1808,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	    ns->lba_shift > PAGE_SHIFT)
 		capacity = 0;
 
-	set_capacity(disk, capacity);
+	disk_set_capacity(disk, capacity);
 
 	nvme_config_discard(disk, ns);
 	nvme_config_write_zeroes(disk, ns);
-- 
2.16.5


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
                   ` (3 preceding siblings ...)
  2020-01-02  7:53 ` [resend v1 4/5] drivers/nvme/host/core.c: " Balbir Singh
@ 2020-01-02  7:53 ` Balbir Singh
  2020-01-02 22:21   ` Chaitanya Kulkarni
  2020-01-07  3:48   ` Martin K. Petersen
  2020-01-06  5:59 ` [resend v1 0/5] Add support for block disk resize notification Bob Liu
  5 siblings, 2 replies; 38+ messages in thread
From: Balbir Singh @ 2020-01-02  7:53 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni, Balbir Singh

block/genhd provides disk_set_capacity() for sending
RESIZE notifications via uevents. This notification is
newly added to scsi sd.

Signed-off-by: Balbir Singh <sblbir@amazon.com>
---
 drivers/scsi/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5afb0046b12a..1a3be30b6b78 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 
 	sdkp->first_scan = 0;
 
-	set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
+	disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
 	sd_config_write_same(sdkp);
 	kfree(buffer);
 
-- 
2.16.5


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-02  7:53 ` [resend v1 5/5] drivers/scsi/sd.c: " Balbir Singh
@ 2020-01-02 22:21   ` Chaitanya Kulkarni
  2020-01-03  0:23     ` Singh, Balbir
  2020-01-07  3:48   ` Martin K. Petersen
  1 sibling, 1 reply; 38+ messages in thread
From: Chaitanya Kulkarni @ 2020-01-02 22:21 UTC (permalink / raw)
  To: Balbir Singh, linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst

On 01/01/2020 11:53 PM, Balbir Singh wrote:
> block/genhd provides disk_set_capacity() for sending
> RESIZE notifications via uevents. This notification is
> newly added to scsi sd.

nit :-

The above commit messages in this series are looking odd from
the ones we have in the tree and is it possible to fold it in
two lines ?

[Can be done at the time of applying series.]


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-02 22:21   ` Chaitanya Kulkarni
@ 2020-01-03  0:23     ` Singh, Balbir
  0 siblings, 0 replies; 38+ messages in thread
From: Singh, Balbir @ 2020-01-03  0:23 UTC (permalink / raw)
  To: linux-kernel, Chaitanya.Kulkarni, linux-block, linux-nvme
  Cc: hch, jejb, mst, axboe, Sangaraju, Someswarudu

On Thu, 2020-01-02 at 22:21 +0000, Chaitanya Kulkarni wrote:
> On 01/01/2020 11:53 PM, Balbir Singh wrote:
> > block/genhd provides disk_set_capacity() for sending
> > RESIZE notifications via uevents. This notification is
> > newly added to scsi sd.
> 
> nit :-
> 
> The above commit messages in this series are looking odd from
> the ones we have in the tree and is it possible to fold it in
> two lines ?
> 
> [Can be done at the time of applying series.]
> 

Sounds good! I'll request the maintainer or repost if needed

Balbir Singh.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-02  7:53 ` [resend v1 1/5] block/genhd: Notify udev about capacity change Balbir Singh
@ 2020-01-03  6:16   ` Chaitanya Kulkarni
  2020-01-04  4:44     ` Singh, Balbir
  2020-01-08 15:02     ` hch
  2020-01-07  3:32   ` Martin K. Petersen
  1 sibling, 2 replies; 38+ messages in thread
From: Chaitanya Kulkarni @ 2020-01-03  6:16 UTC (permalink / raw)
  To: Balbir Singh, linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst

On 01/01/2020 11:53 PM, Balbir Singh wrote:
> Allow block/genhd to notify user space (via udev) about disk size changes
> using a new helper disk_set_capacity(), which is a wrapper on top
> of set_capacity(). disk_set_capacity() will only notify via udev if
> the current capacity or the target capacity is not zero.
>
> Suggested-by: Christoph Hellwig<hch@lst.de>
> Signed-off-by: Someswarudu Sangaraju<ssomesh@amazon.com>
> Signed-off-by: Balbir Singh<sblbir@amazon.com>
> ---

Since disk_set_capacity(() is on the same line as set_capacity()
we should follow the same convention, which is :-

1. Avoid exporting symbol.
2. Mark new function in-line.

Unless there is a very specific reason for breaking the pattern.

Why not this (totally untested but easy convey above comment)
on the top of this patch ?

# git diff
diff --git a/block/genhd.c b/block/genhd.c
index 94faec98607b..ff6268970ddc 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -46,25 +46,6 @@ static void disk_add_events(struct gendisk *disk);
  static void disk_del_events(struct gendisk *disk);
  static void disk_release_events(struct gendisk *disk);

-/*
- * Set disk capacity and notify if the size is not currently
- * zero and will not be set to zero
- */
-void disk_set_capacity(struct gendisk *disk, sector_t size)
-{
-       sector_t capacity = get_capacity(disk);
-
-       set_capacity(disk, size);
-       if (capacity != 0 && size != 0) {
-               char *envp[] = { "RESIZE=1", NULL };
-
-               kobject_uevent_env(&disk_to_dev(disk)->kobj, 
KOBJ_CHANGE, envp);
-       }
-}
-
-EXPORT_SYMBOL_GPL(disk_set_capacity);
-
-
  void part_inc_in_flight(struct request_queue *q, struct hd_struct 
*part, int rw)
  {
         if (queue_is_mq(q))
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index d5e87d7cc357..5e595a28f893 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -469,6 +469,22 @@ static inline void set_capacity(struct gendisk 
*disk, sector_t size)
         disk->part0.nr_sects = size;
  }

+/*
+ * Set disk capacity and notify if the size is not currently
+ * zero and will not be set to zero
+ */
+static inline void disk_set_capacity(struct gendisk *disk, sector_t size)
+{
+       sector_t capacity = get_capacity(disk);
+
+       set_capacity(disk, size);
+       if (capacity != 0 && size != 0) {
+               char *envp[] = { "RESIZE=1", NULL };
+
+               kobject_uevent_env(&disk_to_dev(disk)->kobj, 
KOBJ_CHANGE, envp);
+       }
+}
+
  #ifdef CONFIG_SOLARIS_X86_PARTITION



^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-03  6:16   ` Chaitanya Kulkarni
@ 2020-01-04  4:44     ` Singh, Balbir
  2020-01-04 22:32       ` Chaitanya Kulkarni
  2020-01-08 15:02     ` hch
  1 sibling, 1 reply; 38+ messages in thread
From: Singh, Balbir @ 2020-01-04  4:44 UTC (permalink / raw)
  To: linux-kernel, Chaitanya.Kulkarni, linux-block, linux-nvme
  Cc: hch, jejb, mst, axboe, Sangaraju, Someswarudu

On Fri, 2020-01-03 at 06:16 +0000, Chaitanya Kulkarni wrote:
> On 01/01/2020 11:53 PM, Balbir Singh wrote:
> > Allow block/genhd to notify user space (via udev) about disk size changes
> > using a new helper disk_set_capacity(), which is a wrapper on top
> > of set_capacity(). disk_set_capacity() will only notify via udev if
> > the current capacity or the target capacity is not zero.
> > 
> > Suggested-by: Christoph Hellwig<hch@lst.de>
> > Signed-off-by: Someswarudu Sangaraju<ssomesh@amazon.com>
> > Signed-off-by: Balbir Singh<sblbir@amazon.com>
> > ---
> 
> Since disk_set_capacity(() is on the same line as set_capacity()
> we should follow the same convention, which is :-
> 
> 1. Avoid exporting symbol.
> 2. Mark new function in-line.
> 
> Unless there is a very specific reason for breaking the pattern.
> 

I don't see the benefit of marking it as inline. I expect this code to be
potentially expanded to provide callbacks into file system code for expansion
(something you brought up), marking it as inline might be a limitation.

I'd rather not clutter the headers with this code, but I am open to what the
maintainers think as well.

Balbir Singh.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-02  7:53 ` [resend v1 4/5] drivers/nvme/host/core.c: " Balbir Singh
@ 2020-01-04 22:27   ` Chaitanya Kulkarni
  2020-01-06  0:46     ` Singh, Balbir
  0 siblings, 1 reply; 38+ messages in thread
From: Chaitanya Kulkarni @ 2020-01-04 22:27 UTC (permalink / raw)
  To: Balbir Singh, linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst

Quick question here if user executes nvme ns-rescan /dev/nvme1
will following code result in triggering uevent(s) for
the namespace(s( for which there is no change in the size ?

If so is that an expected behavior ?

On 01/01/2020 11:54 PM, Balbir Singh wrote:
> block/genhd provides disk_set_capacity() for sending
> RESIZE notifications via uevents. This notification is
> newly added to NVME devices
>
> Signed-off-by: Balbir Singh <sblbir@amazon.com>
> ---
>   drivers/nvme/host/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 667f18f465be..cb214e914fc2 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1808,7 +1808,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>   	    ns->lba_shift > PAGE_SHIFT)
>   		capacity = 0;
>
> -	set_capacity(disk, capacity);
> +	disk_set_capacity(disk, capacity);
>
>   	nvme_config_discard(disk, ns);
>   	nvme_config_write_zeroes(disk, ns);
>


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-04  4:44     ` Singh, Balbir
@ 2020-01-04 22:32       ` Chaitanya Kulkarni
  0 siblings, 0 replies; 38+ messages in thread
From: Chaitanya Kulkarni @ 2020-01-04 22:32 UTC (permalink / raw)
  To: Singh, Balbir, linux-kernel, linux-block, linux-nvme
  Cc: hch, jejb, mst, axboe, Sangaraju, Someswarudu

On 01/03/2020 08:44 PM, Singh, Balbir wrote:
>> >Since disk_set_capacity(() is on the same line as set_capacity()
>> >we should follow the same convention, which is :-
>> >
>> >1. Avoid exporting symbol.
>> >2. Mark new function in-line.
>> >
>> >Unless there is a very specific reason for breaking the pattern.
>> >
> I don't see the benefit of marking it as inline. I expect this code to be
> potentially expanded to provide callbacks into file system code for expansion
> (something you brought up), marking it as inline might be a limitation.
>
That can be done as a prep patch when framework is ready.
> I'd rather not clutter the headers with this code, but I am open to what the
> maintainers think as well.
>
Okay.
> Balbir Singh.
>
>


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-04 22:27   ` Chaitanya Kulkarni
@ 2020-01-06  0:46     ` Singh, Balbir
  2020-01-08 15:04       ` hch
  0 siblings, 1 reply; 38+ messages in thread
From: Singh, Balbir @ 2020-01-06  0:46 UTC (permalink / raw)
  To: linux-kernel, Chaitanya.Kulkarni, linux-block, linux-nvme
  Cc: hch, jejb, mst, axboe, Sangaraju, Someswarudu

On Sat, 2020-01-04 at 22:27 +0000, Chaitanya Kulkarni wrote:
> Quick question here if user executes nvme ns-rescan /dev/nvme1
> will following code result in triggering uevent(s) for
> the namespace(s( for which there is no change in the size ?
> 
> If so is that an expected behavior ?
> 

My old code had a check to see if old_capacity != new_capacity as well.
I can redo those bits if needed.

The expected behaviour is not clear, but the functionality is not broken, user
space should be able to deal with a resize event where the previous capacity
== new capacity IMHO.

Balbir Singh.


> On 01/01/2020 11:54 PM, Balbir Singh wrote:
> > block/genhd provides disk_set_capacity() for sending
> > RESIZE notifications via uevents. This notification is
> > newly added to NVME devices
> > 
> > Signed-off-by: Balbir Singh <sblbir@amazon.com>
> > ---
> >   drivers/nvme/host/core.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 667f18f465be..cb214e914fc2 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1808,7 +1808,7 @@ static void nvme_update_disk_info(struct gendisk
> > *disk,
> >   	    ns->lba_shift > PAGE_SHIFT)
> >   		capacity = 0;
> > 
> > -	set_capacity(disk, capacity);
> > +	disk_set_capacity(disk, capacity);


> > 
> >   	nvme_config_discard(disk, ns);
> >   	nvme_config_write_zeroes(disk, ns);
> > 
> 
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 0/5] Add support for block disk resize notification
  2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
                   ` (4 preceding siblings ...)
  2020-01-02  7:53 ` [resend v1 5/5] drivers/scsi/sd.c: " Balbir Singh
@ 2020-01-06  5:59 ` Bob Liu
  2020-01-06  8:47   ` Singh, Balbir
  5 siblings, 1 reply; 38+ messages in thread
From: Bob Liu @ 2020-01-06  5:59 UTC (permalink / raw)
  To: Balbir Singh, linux-kernel, linux-block, linux-nvme
  Cc: axboe, ssomesh, jejb, hch, mst, Chaitanya.Kulkarni

On 1/2/20 3:53 PM, Balbir Singh wrote:
> Allow block/genhd to notify user space about disk size changes
> using a new helper disk_set_capacity(), which is a wrapper on top
> of set_capacity(). disk_set_capacity() will only notify if
> the current capacity or the target capacity is not zero.
> 

set_capacity_and_notify() may be a more straightforward name.

> Background:
> 
> As a part of a patch to allow sending the RESIZE event on disk capacity
> change, Christoph (hch@lst.de) requested that the patch be made generic
> and the hacks for virtio block and xen block devices be removed and
> merged via a generic helper.
> 
> This series consists of 5 changes. The first one adds the basic
> support for changing the size and notifying. The follow up patches
> are per block subsystem changes. Other block drivers can add their
> changes as necessary on top of this series.
> 
> Testing:
> 1. I did some basic testing with an NVME device, by resizing it in
> the backend and ensured that udevd received the event.
> 
> NOTE: After these changes, the notification might happen before
> revalidate disk, where as it occured later before.
> 

It's better not to change original behavior.
How about something like:

+void set_capacity_and_notify(struct gendisk *disk, sector_t size, bool revalidate)
{
	sector_t capacity = get_capacity(disk);

	set_capacity(disk, size);

+        if (revalidate)
+		revalidate_disk(disk);
	if (capacity != 0 && size != 0) {
		char *envp[] = { "RESIZE=1", NULL };

		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
	}
}

> Balbir Singh (5):
>   block/genhd: Notify udev about capacity change
>   drivers/block/virtio_blk.c: Convert to use disk_set_capacity
>   drivers/block/xen-blkfront.c: Convert to use disk_set_capacity
>   drivers/nvme/host/core.c: Convert to use disk_set_capacity
>   drivers/scsi/sd.c: Convert to use disk_set_capacity
> 
>  block/genhd.c                | 19 +++++++++++++++++++
>  drivers/block/virtio_blk.c   |  4 +---
>  drivers/block/xen-blkfront.c |  5 +----
>  drivers/nvme/host/core.c     |  2 +-
>  drivers/scsi/sd.c            |  2 +-
>  include/linux/genhd.h        |  1 +
>  6 files changed, 24 insertions(+), 9 deletions(-)
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 0/5] Add support for block disk resize notification
  2020-01-06  5:59 ` [resend v1 0/5] Add support for block disk resize notification Bob Liu
@ 2020-01-06  8:47   ` Singh, Balbir
  2020-01-06  9:08     ` Bob Liu
  0 siblings, 1 reply; 38+ messages in thread
From: Singh, Balbir @ 2020-01-06  8:47 UTC (permalink / raw)
  To: linux-kernel, bob.liu, linux-block, linux-nvme
  Cc: hch, jejb, Chaitanya.Kulkarni, mst, axboe, Sangaraju, Someswarudu

On Mon, 2020-01-06 at 13:59 +0800, Bob Liu wrote:
> On 1/2/20 3:53 PM, Balbir Singh wrote:
> > Allow block/genhd to notify user space about disk size changes
> > using a new helper disk_set_capacity(), which is a wrapper on top
> > of set_capacity(). disk_set_capacity() will only notify if
> > the current capacity or the target capacity is not zero.
> > 
> 
> set_capacity_and_notify() may be a more straightforward name.
> 

Yes, agreed.

> > Background:
> > 
> > As a part of a patch to allow sending the RESIZE event on disk capacity
> > change, Christoph (hch@lst.de) requested that the patch be made generic
> > and the hacks for virtio block and xen block devices be removed and
> > merged via a generic helper.
> > 
> > This series consists of 5 changes. The first one adds the basic
> > support for changing the size and notifying. The follow up patches
> > are per block subsystem changes. Other block drivers can add their
> > changes as necessary on top of this series.
> > 
> > Testing:
> > 1. I did some basic testing with an NVME device, by resizing it in
> > the backend and ensured that udevd received the event.
> > 
> > NOTE: After these changes, the notification might happen before
> > revalidate disk, where as it occured later before.
> > 
> 
> It's better not to change original behavior.
> How about something like:
> 
> +void set_capacity_and_notify(struct gendisk *disk, sector_t size, bool
> revalidate)
> {
> 	sector_t capacity = get_capacity(disk);
> 
> 	set_capacity(disk, size);
> 
> +        if (revalidate)
> +		revalidate_disk(disk);

Do you see a concern with the notification going out before revalidate_disk?
I could keep the behaviour and come up with a suitable name

revalidate_disk_and_notify() (set_capacity is a part of the revalidation
process), or feel free to suggest a better name

Thanks,
Balbir Singh


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 0/5] Add support for block disk resize notification
  2020-01-06  8:47   ` Singh, Balbir
@ 2020-01-06  9:08     ` Bob Liu
  0 siblings, 0 replies; 38+ messages in thread
From: Bob Liu @ 2020-01-06  9:08 UTC (permalink / raw)
  To: Singh, Balbir, linux-kernel, linux-block, linux-nvme
  Cc: hch, jejb, Chaitanya.Kulkarni, mst, axboe, Sangaraju, Someswarudu

On 1/6/20 4:47 PM, Singh, Balbir wrote:
> On Mon, 2020-01-06 at 13:59 +0800, Bob Liu wrote:
>> On 1/2/20 3:53 PM, Balbir Singh wrote:
>>> Allow block/genhd to notify user space about disk size changes
>>> using a new helper disk_set_capacity(), which is a wrapper on top
>>> of set_capacity(). disk_set_capacity() will only notify if
>>> the current capacity or the target capacity is not zero.
>>>
>>
>> set_capacity_and_notify() may be a more straightforward name.
>>
> 
> Yes, agreed.
> 
>>> Background:
>>>
>>> As a part of a patch to allow sending the RESIZE event on disk capacity
>>> change, Christoph (hch@lst.de) requested that the patch be made generic
>>> and the hacks for virtio block and xen block devices be removed and
>>> merged via a generic helper.
>>>
>>> This series consists of 5 changes. The first one adds the basic
>>> support for changing the size and notifying. The follow up patches
>>> are per block subsystem changes. Other block drivers can add their
>>> changes as necessary on top of this series.
>>>
>>> Testing:
>>> 1. I did some basic testing with an NVME device, by resizing it in
>>> the backend and ensured that udevd received the event.
>>>
>>> NOTE: After these changes, the notification might happen before
>>> revalidate disk, where as it occured later before.
>>>
>>
>> It's better not to change original behavior.
>> How about something like:
>>
>> +void set_capacity_and_notify(struct gendisk *disk, sector_t size, bool
>> revalidate)
>> {
>> 	sector_t capacity = get_capacity(disk);
>>
>> 	set_capacity(disk, size);
>>
>> +        if (revalidate)
>> +		revalidate_disk(disk);
> 
> Do you see a concern with the notification going out before revalidate_disk?

Not aware any, but keep the original behavior is safer especial when not must change..

> I could keep the behaviour and come up with a suitable name
> 
> revalidate_disk_and_notify() (set_capacity is a part of the revalidation
> process), or feel free to suggest a better name
> 

Feel free to add my reviewed-by next version in this series.
Reviewed-by: Bob Liu <bob.liu@oracle.com>

> Thanks,
> Balbir Singh
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-02  7:53 ` [resend v1 1/5] block/genhd: Notify udev about capacity change Balbir Singh
  2020-01-03  6:16   ` Chaitanya Kulkarni
@ 2020-01-07  3:32   ` Martin K. Petersen
  2020-01-07 22:30     ` Singh, Balbir
  1 sibling, 1 reply; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-07  3:32 UTC (permalink / raw)
  To: Balbir Singh
  Cc: linux-kernel, linux-block, linux-nvme, axboe, ssomesh, jejb, hch,
	mst, Chaitanya.Kulkarni


Hi Balbir,

> Allow block/genhd to notify user space (via udev) about disk size
> changes using a new helper disk_set_capacity(), which is a wrapper on
> top of set_capacity(). disk_set_capacity() will only notify via udev
> if the current capacity or the target capacity is not zero.

I know set_capacity() is called all over the place making it a bit of a
pain to audit. Is that the reason you introduced a new function instead
of just emitting the event in set_capacity()?

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-02  7:53 ` [resend v1 5/5] drivers/scsi/sd.c: " Balbir Singh
  2020-01-02 22:21   ` Chaitanya Kulkarni
@ 2020-01-07  3:48   ` Martin K. Petersen
  2020-01-07  3:57     ` James Bottomley
                       ` (2 more replies)
  1 sibling, 3 replies; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-07  3:48 UTC (permalink / raw)
  To: Balbir Singh
  Cc: linux-kernel, linux-block, linux-nvme, axboe, ssomesh, jejb, hch,
	mst, Chaitanya.Kulkarni


Balbir,

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5afb0046b12a..1a3be30b6b78 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  
>  	sdkp->first_scan = 0;
>  
> -	set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
> +	disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
>  	sd_config_write_same(sdkp);
>  	kfree(buffer);

We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
capacity changes. However, this event does not automatically cause
revalidation.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07  3:48   ` Martin K. Petersen
@ 2020-01-07  3:57     ` James Bottomley
  2020-01-07  4:39       ` Martin K. Petersen
  2020-01-07 22:28     ` Singh, Balbir
  2020-01-08 15:06     ` Christoph Hellwig
  2 siblings, 1 reply; 38+ messages in thread
From: James Bottomley @ 2020-01-07  3:57 UTC (permalink / raw)
  To: Martin K. Petersen, Balbir Singh
  Cc: linux-kernel, linux-block, linux-nvme, axboe, ssomesh, hch, mst,
	Chaitanya.Kulkarni

On Mon, 2020-01-06 at 22:48 -0500, Martin K. Petersen wrote:
> Balbir,
> 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 5afb0046b12a..1a3be30b6b78 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk
> > *disk)
> >  
> >  	sdkp->first_scan = 0;
> >  
> > -	set_capacity(disk, logical_to_sectors(sdp, sdkp-
> > >capacity));
> > +	disk_set_capacity(disk, logical_to_sectors(sdp, sdkp-
> > >capacity));
> >  	sd_config_write_same(sdkp);
> >  	kfree(buffer);
> 
> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
> capacity changes. However, this event does not automatically cause
> revalidation.

Which I seem to remember was a deliberate choice: some change
capacities occur because the path goes passive and default values get
installed.

James


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07  3:57     ` James Bottomley
@ 2020-01-07  4:39       ` Martin K. Petersen
  2020-01-07 21:37         ` Ewan D. Milne
  0 siblings, 1 reply; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-07  4:39 UTC (permalink / raw)
  To: James Bottomley
  Cc: Martin K. Petersen, Balbir Singh, linux-kernel, linux-block,
	linux-nvme, axboe, ssomesh, hch, mst, Chaitanya.Kulkarni


James,

>> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
>> capacity changes. However, this event does not automatically cause
>> revalidation.
>
> Which I seem to remember was a deliberate choice: some change
> capacities occur because the path goes passive and default values get
> installed.

Yep, it's very tricky territory.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07  4:39       ` Martin K. Petersen
@ 2020-01-07 21:37         ` Ewan D. Milne
  2020-01-08  2:59           ` Martin K. Petersen
  0 siblings, 1 reply; 38+ messages in thread
From: Ewan D. Milne @ 2020-01-07 21:37 UTC (permalink / raw)
  To: Martin K. Petersen, James Bottomley
  Cc: axboe, Chaitanya.Kulkarni, mst, linux-kernel, linux-nvme,
	linux-block, ssomesh, Balbir Singh, hch

On Mon, 2020-01-06 at 23:39 -0500, Martin K. Petersen wrote:
> James,
> 
> > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
> > > capacity changes. However, this event does not automatically cause
> > > revalidation.
> > 
> > Which I seem to remember was a deliberate choice: some change
> > capacities occur because the path goes passive and default values get
> > installed.
> 
> Yep, it's very tricky territory.
> 

Yes, there are some storage arrays that refuse a READ CAPACITY
command in certain ALUA states so you can't get the new capacity anyway.

It might be nice to improve this, though, there are some cases now where
we set the capacity to zero when we revalidate and can't get the value.

-Ewan




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07  3:48   ` Martin K. Petersen
  2020-01-07  3:57     ` James Bottomley
@ 2020-01-07 22:28     ` Singh, Balbir
  2020-01-08  3:15       ` Martin K. Petersen
  2020-01-08 15:06     ` Christoph Hellwig
  2 siblings, 1 reply; 38+ messages in thread
From: Singh, Balbir @ 2020-01-07 22:28 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-kernel, linux-block, Sangaraju, Someswarudu, jejb, hch,
	axboe, mst, linux-nvme, Chaitanya.Kulkarni

On Mon, 2020-01-06 at 22:48 -0500, Martin K. Petersen wrote:
> Balbir,
> 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 5afb0046b12a..1a3be30b6b78 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> >  
> >  	sdkp->first_scan = 0;
> >  
> > -	set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
> > +	disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
> >  	sd_config_write_same(sdkp);
> >  	kfree(buffer);
> 
> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
> capacity changes. However, this event does not automatically cause
> revalidation.
> 

The proposed idea is to not reinforce revalidation, unless explictly specified
(in the thread before Bob Liu had suggestions). The goal is to notify user
space of changes via RESIZE. SCSI sd can opt out of this IOW, I can remove
this if you feel SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current
use cases.

Balbir Singh.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-07  3:32   ` Martin K. Petersen
@ 2020-01-07 22:30     ` Singh, Balbir
  2020-01-08  3:15       ` Martin K. Petersen
  0 siblings, 1 reply; 38+ messages in thread
From: Singh, Balbir @ 2020-01-07 22:30 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-kernel, linux-block, Sangaraju, Someswarudu, jejb, hch,
	axboe, mst, linux-nvme, Chaitanya.Kulkarni

On Mon, 2020-01-06 at 22:32 -0500, Martin K. Petersen wrote:
> Hi Balbir,
> 
> > Allow block/genhd to notify user space (via udev) about disk size
> > changes using a new helper disk_set_capacity(), which is a wrapper on
> > top of set_capacity(). disk_set_capacity() will only notify via udev
> > if the current capacity or the target capacity is not zero.
> 
> I know set_capacity() is called all over the place making it a bit of a
> pain to audit. Is that the reason you introduced a new function instead
> of just emitting the event in set_capacity()?
> 

I did this to avoid having to enforce that set_capacity() implied a
notification. Largely to control the impact of the change by default.

Balbir Singh.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07 21:37         ` Ewan D. Milne
@ 2020-01-08  2:59           ` Martin K. Petersen
  2020-01-08 21:27             ` Ewan D. Milne
  0 siblings, 1 reply; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-08  2:59 UTC (permalink / raw)
  To: Ewan D. Milne
  Cc: Martin K. Petersen, James Bottomley, axboe, Chaitanya.Kulkarni,
	mst, linux-kernel, linux-nvme, linux-block, ssomesh,
	Balbir Singh, hch


Ewan,

> Yes, there are some storage arrays that refuse a READ CAPACITY
> command in certain ALUA states so you can't get the new capacity
> anyway.

Yep. And some devices will temporarily return a capacity of
0xFFFFFFFF... If we were to trigger a filesystem resize, the results
would be disastrous.

> It might be nice to improve this, though, there are some cases now
> where we set the capacity to zero when we revalidate and can't get the
> value.

If you have a test case, let's fix it.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07 22:28     ` Singh, Balbir
@ 2020-01-08  3:15       ` Martin K. Petersen
  2020-01-08  4:20         ` Singh, Balbir
  2020-01-08 21:32         ` Ewan D. Milne
  0 siblings, 2 replies; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-08  3:15 UTC (permalink / raw)
  To: Singh, Balbir
  Cc: martin.petersen, linux-kernel, linux-block, Sangaraju,
	Someswarudu, jejb, hch, axboe, mst, linux-nvme,
	Chaitanya.Kulkarni


Balbir,

>> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
>> capacity changes. However, this event does not automatically cause
>> revalidation.
>
> The proposed idea is to not reinforce revalidation, unless explictly
> specified (in the thread before Bob Liu had suggestions). The goal is
> to notify user space of changes via RESIZE. SCSI sd can opt out of
> this IOW, I can remove this if you feel
> SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases.

I have no particular objection to the code change. I was just observing
that in the context of sd.c, RESIZE=1 is more of a "your request to
resize was successful" notification due to the requirement of an
explicit userland action in case a device reports a capacity change.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-07 22:30     ` Singh, Balbir
@ 2020-01-08  3:15       ` Martin K. Petersen
  2020-01-08 15:04         ` hch
  0 siblings, 1 reply; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-08  3:15 UTC (permalink / raw)
  To: Singh, Balbir
  Cc: martin.petersen, linux-kernel, linux-block, Sangaraju,
	Someswarudu, jejb, hch, axboe, mst, linux-nvme,
	Chaitanya.Kulkarni


Balbir,

> I did this to avoid having to enforce that set_capacity() implied a
> notification. Largely to control the impact of the change by default.

What I thought. I'm OK with set_capacity_and_notify(), btw.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-08  3:15       ` Martin K. Petersen
@ 2020-01-08  4:20         ` Singh, Balbir
  2020-01-08 21:32         ` Ewan D. Milne
  1 sibling, 0 replies; 38+ messages in thread
From: Singh, Balbir @ 2020-01-08  4:20 UTC (permalink / raw)
  To: martin.petersen
  Cc: linux-kernel, linux-block, Sangaraju, Someswarudu, jejb, hch,
	axboe, mst, linux-nvme, Chaitanya.Kulkarni

On Tue, 2020-01-07 at 22:15 -0500, Martin K. Petersen wrote:
> Balbir,
> 
> > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
> > > capacity changes. However, this event does not automatically cause
> > > revalidation.
> > 
> > The proposed idea is to not reinforce revalidation, unless explictly
> > specified (in the thread before Bob Liu had suggestions). The goal is
> > to notify user space of changes via RESIZE. SCSI sd can opt out of
> > this IOW, I can remove this if you feel
> > SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases.
> 
> I have no particular objection to the code change. I was just observing
> that in the context of sd.c, RESIZE=1 is more of a "your request to
> resize was successful" notification due to the requirement of an
> explicit userland action in case a device reports a capacity change.
> 

That is true, yes I agree with your observation.

Balbir Singh.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-03  6:16   ` Chaitanya Kulkarni
  2020-01-04  4:44     ` Singh, Balbir
@ 2020-01-08 15:02     ` hch
  1 sibling, 0 replies; 38+ messages in thread
From: hch @ 2020-01-08 15:02 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: Balbir Singh, linux-kernel, linux-block, linux-nvme, axboe,
	ssomesh, jejb, hch, mst

On Fri, Jan 03, 2020 at 06:16:39AM +0000, Chaitanya Kulkarni wrote:
> Since disk_set_capacity(() is on the same line as set_capacity()
> we should follow the same convention, which is :-
> 
> 1. Avoid exporting symbol.
> 2. Mark new function in-line.
> 
> Unless there is a very specific reason for breaking the pattern.

Why would we mark it as inline?  It isn't by any means in the fast
path, and there are no easy opportunities for constant propagation,
so the only thing that would do is increase the code size.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-08  3:15       ` Martin K. Petersen
@ 2020-01-08 15:04         ` hch
  2020-01-21 19:57           ` Singh, Balbir
  0 siblings, 1 reply; 38+ messages in thread
From: hch @ 2020-01-08 15:04 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: Singh, Balbir, linux-kernel, linux-block, Sangaraju, Someswarudu,
	jejb, hch, axboe, mst, linux-nvme, Chaitanya.Kulkarni

On Tue, Jan 07, 2020 at 10:15:34PM -0500, Martin K. Petersen wrote:
> 
> Balbir,
> 
> > I did this to avoid having to enforce that set_capacity() implied a
> > notification. Largely to control the impact of the change by default.
> 
> What I thought. I'm OK with set_capacity_and_notify(), btw.

To some extent it might make sense to always notify from set_capacity
and have a set_capacity_nonotify if we don't want to notify, as in
general we probably should notify unless we have a good reason not to.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-06  0:46     ` Singh, Balbir
@ 2020-01-08 15:04       ` hch
  2020-01-09  3:33         ` Martin K. Petersen
  2020-01-21 19:52         ` Singh, Balbir
  0 siblings, 2 replies; 38+ messages in thread
From: hch @ 2020-01-08 15:04 UTC (permalink / raw)
  To: Singh, Balbir
  Cc: linux-kernel, Chaitanya.Kulkarni, linux-block, linux-nvme, hch,
	jejb, mst, axboe, Sangaraju, Someswarudu

On Mon, Jan 06, 2020 at 12:46:26AM +0000, Singh, Balbir wrote:
> On Sat, 2020-01-04 at 22:27 +0000, Chaitanya Kulkarni wrote:
> > Quick question here if user executes nvme ns-rescan /dev/nvme1
> > will following code result in triggering uevent(s) for
> > the namespace(s( for which there is no change in the size ?
> > 
> > If so is that an expected behavior ?
> > 
> 
> My old code had a check to see if old_capacity != new_capacity as well.
> I can redo those bits if needed.
> 
> The expected behaviour is not clear, but the functionality is not broken, user
> space should be able to deal with a resize event where the previous capacity
> == new capacity IMHO.

I think it makes sense to not bother with a notification unless there
is an actual change.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-07  3:48   ` Martin K. Petersen
  2020-01-07  3:57     ` James Bottomley
  2020-01-07 22:28     ` Singh, Balbir
@ 2020-01-08 15:06     ` Christoph Hellwig
  2020-01-09  2:53       ` Martin K. Petersen
  2 siblings, 1 reply; 38+ messages in thread
From: Christoph Hellwig @ 2020-01-08 15:06 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: Balbir Singh, linux-kernel, linux-block, linux-nvme, axboe,
	ssomesh, jejb, hch, mst, Chaitanya.Kulkarni

On Mon, Jan 06, 2020 at 10:48:30PM -0500, Martin K. Petersen wrote:
> 
> Balbir,
> 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 5afb0046b12a..1a3be30b6b78 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> >  
> >  	sdkp->first_scan = 0;
> >  
> > -	set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
> > +	disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
> >  	sd_config_write_same(sdkp);
> >  	kfree(buffer);
> 
> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
> capacity changes. However, this event does not automatically cause
> revalidation.

Who is looking at these sdev specific events?  (And who is looking
at the virtio/xenblk ones?)  It  makes a lot of sense to have one event
supported by all devices.  But don't ask me which one would be the best..

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-08  2:59           ` Martin K. Petersen
@ 2020-01-08 21:27             ` Ewan D. Milne
  0 siblings, 0 replies; 38+ messages in thread
From: Ewan D. Milne @ 2020-01-08 21:27 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: James Bottomley, axboe, Chaitanya.Kulkarni, mst, linux-kernel,
	linux-nvme, linux-block, ssomesh, Balbir Singh, hch

On Tue, 2020-01-07 at 21:59 -0500, Martin K. Petersen wrote:
> Ewan,
> 
> > Yes, there are some storage arrays that refuse a READ CAPACITY
> > command in certain ALUA states so you can't get the new capacity
> > anyway.
> 
> Yep. And some devices will temporarily return a capacity of
> 0xFFFFFFFF... If we were to trigger a filesystem resize, the results
> would be disastrous.
> 
> > It might be nice to improve this, though, there are some cases now
> > where we set the capacity to zero when we revalidate and can't get the
> > value.
> 
> If you have a test case, let's fix it.
> 

This happens with NVMe fabric devices, I thought.  I'll check.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-08  3:15       ` Martin K. Petersen
  2020-01-08  4:20         ` Singh, Balbir
@ 2020-01-08 21:32         ` Ewan D. Milne
  1 sibling, 0 replies; 38+ messages in thread
From: Ewan D. Milne @ 2020-01-08 21:32 UTC (permalink / raw)
  To: Martin K. Petersen, Singh, Balbir
  Cc: axboe, Chaitanya.Kulkarni, mst, jejb, linux-kernel, linux-nvme,
	linux-block, Sangaraju, Someswarudu, hch

On Tue, 2020-01-07 at 22:15 -0500, Martin K. Petersen wrote:
> Balbir,
> 
> > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
> > > capacity changes. However, this event does not automatically cause
> > > revalidation.
> > 
> > The proposed idea is to not reinforce revalidation, unless explictly
> > specified (in the thread before Bob Liu had suggestions). The goal is
> > to notify user space of changes via RESIZE. SCSI sd can opt out of
> > this IOW, I can remove this if you feel
> > SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases.

Remember that this event is generated because of a Unit Attention from
the device.  We are only passing on this indication to udev.  It basically
allows automation without having to scrape the log file.  We don't proactively
look. e.g. in the case of SCSI unless you have commands being sent to the
device to return the UA status you won't hear about it.

> 
> I have no particular objection to the code change. I was just observing
> that in the context of sd.c, RESIZE=1 is more of a "your request to
> resize was successful" notification due to the requirement of an
> explicit userland action in case a device reports a capacity change.
> 



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 5/5] drivers/scsi/sd.c: Convert to use disk_set_capacity
  2020-01-08 15:06     ` Christoph Hellwig
@ 2020-01-09  2:53       ` Martin K. Petersen
  0 siblings, 0 replies; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-09  2:53 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Martin K. Petersen, Balbir Singh, linux-kernel, linux-block,
	linux-nvme, axboe, ssomesh, jejb, mst, Chaitanya.Kulkarni


Christoph,

>> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device
>> capacity changes. However, this event does not automatically cause
>> revalidation.
>
> Who is looking at these sdev specific events?  (And who is looking
> at the virtio/xenblk ones?)  It  makes a lot of sense to have one event
> supported by all devices.  But don't ask me which one would be the best..

Users typically have site-specific udev rules or similar. There is no
standard entity handling these events. Sadly.

I'm all for standardizing on RESIZE=1. However, we can't really get rid
of the emitting existing SDEV* event without breaking existing setups.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-08 15:04       ` hch
@ 2020-01-09  3:33         ` Martin K. Petersen
  2020-01-09 13:12           ` Ewan D. Milne
  2020-01-21 19:52         ` Singh, Balbir
  1 sibling, 1 reply; 38+ messages in thread
From: Martin K. Petersen @ 2020-01-09  3:33 UTC (permalink / raw)
  To: hch
  Cc: Singh, Balbir, linux-kernel, Chaitanya.Kulkarni, linux-block,
	linux-nvme, jejb, mst, axboe, Sangaraju, Someswarudu


Christoph,

>> The expected behaviour is not clear, but the functionality is not
>> broken, user space should be able to deal with a resize event where
>> the previous capacity == new capacity IMHO.
>
> I think it makes sense to not bother with a notification unless there
> is an actual change.

I agree.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-09  3:33         ` Martin K. Petersen
@ 2020-01-09 13:12           ` Ewan D. Milne
  0 siblings, 0 replies; 38+ messages in thread
From: Ewan D. Milne @ 2020-01-09 13:12 UTC (permalink / raw)
  To: Martin K. Petersen, hch
  Cc: axboe, Chaitanya.Kulkarni, mst, jejb, linux-kernel, linux-nvme,
	linux-block, Sangaraju, Someswarudu, Singh, Balbir

On Wed, 2020-01-08 at 22:33 -0500, Martin K. Petersen wrote:
> Christoph,
> 
> > > The expected behaviour is not clear, but the functionality is not
> > > broken, user space should be able to deal with a resize event where
> > > the previous capacity == new capacity IMHO.
> > 
> > I think it makes sense to not bother with a notification unless there
> > is an actual change.
> 
> I agree.
> 

Yes, absolutely.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 4/5] drivers/nvme/host/core.c: Convert to use disk_set_capacity
  2020-01-08 15:04       ` hch
  2020-01-09  3:33         ` Martin K. Petersen
@ 2020-01-21 19:52         ` Singh, Balbir
  1 sibling, 0 replies; 38+ messages in thread
From: Singh, Balbir @ 2020-01-21 19:52 UTC (permalink / raw)
  To: hch
  Cc: Sangaraju, Someswarudu, linux-kernel, linux-block, jejb, axboe,
	mst, linux-nvme, Chaitanya.Kulkarni

On Wed, 2020-01-08 at 16:04 +0100, hch@lst.de wrote:
> On Mon, Jan 06, 2020 at 12:46:26AM +0000, Singh, Balbir wrote:
> > On Sat, 2020-01-04 at 22:27 +0000, Chaitanya Kulkarni wrote:
> > > Quick question here if user executes nvme ns-rescan /dev/nvme1
> > > will following code result in triggering uevent(s) for
> > > the namespace(s( for which there is no change in the size ?
> > > 
> > > If so is that an expected behavior ?
> > > 
> > 
> > My old code had a check to see if old_capacity != new_capacity as well.
> > I can redo those bits if needed.
> > 
> > The expected behaviour is not clear, but the functionality is not broken,
> > user
> > space should be able to deal with a resize event where the previous
> > capacity
> > == new capacity IMHO.
> 
> I think it makes sense to not bother with a notification unless there
> is an actual change.

[Sorry for the delayed response, just back from LCA]

Agreed!

Balbir Singh

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [resend v1 1/5] block/genhd: Notify udev about capacity change
  2020-01-08 15:04         ` hch
@ 2020-01-21 19:57           ` Singh, Balbir
  0 siblings, 0 replies; 38+ messages in thread
From: Singh, Balbir @ 2020-01-21 19:57 UTC (permalink / raw)
  To: hch, martin.petersen
  Cc: linux-kernel, linux-block, Sangaraju, Someswarudu, jejb, axboe,
	mst, linux-nvme, Chaitanya.Kulkarni

On Wed, 2020-01-08 at 16:04 +0100, hch@lst.de wrote:
> On Tue, Jan 07, 2020 at 10:15:34PM -0500, Martin K. Petersen wrote:
> > 
> > Balbir,
> > 
> > > I did this to avoid having to enforce that set_capacity() implied a
> > > notification. Largely to control the impact of the change by default.
> > 
> > What I thought. I'm OK with set_capacity_and_notify(), btw.
> 
> To some extent it might make sense to always notify from set_capacity
> and have a set_capacity_nonotify if we don't want to notify, as in
> general we probably should notify unless we have a good reason not to.

I am not sure if this is the right path, since with the new interface, we'll
now have revalidate disk bits built into the function (see the comments from
Bob earlier). I'd be happier converting interfaces a few at a time, A quick
grep found over 100 references and I am not even sure how many of them are
resizable without hotplug in/out on the fly. Hence, I limited myself to the
small subset, I could consider expanding it later based on the need and their
ability to resize on the fly

Balbir Singh.

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2020-01-21 19:57 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-02  7:53 [resend v1 0/5] Add support for block disk resize notification Balbir Singh
2020-01-02  7:53 ` [resend v1 1/5] block/genhd: Notify udev about capacity change Balbir Singh
2020-01-03  6:16   ` Chaitanya Kulkarni
2020-01-04  4:44     ` Singh, Balbir
2020-01-04 22:32       ` Chaitanya Kulkarni
2020-01-08 15:02     ` hch
2020-01-07  3:32   ` Martin K. Petersen
2020-01-07 22:30     ` Singh, Balbir
2020-01-08  3:15       ` Martin K. Petersen
2020-01-08 15:04         ` hch
2020-01-21 19:57           ` Singh, Balbir
2020-01-02  7:53 ` [resend v1 2/5] drivers/block/virtio_blk.c: Convert to use disk_set_capacity Balbir Singh
2020-01-02  7:53 ` [resend v1 3/5] drivers/block/xen-blkfront.c: " Balbir Singh
2020-01-02  7:53 ` [resend v1 4/5] drivers/nvme/host/core.c: " Balbir Singh
2020-01-04 22:27   ` Chaitanya Kulkarni
2020-01-06  0:46     ` Singh, Balbir
2020-01-08 15:04       ` hch
2020-01-09  3:33         ` Martin K. Petersen
2020-01-09 13:12           ` Ewan D. Milne
2020-01-21 19:52         ` Singh, Balbir
2020-01-02  7:53 ` [resend v1 5/5] drivers/scsi/sd.c: " Balbir Singh
2020-01-02 22:21   ` Chaitanya Kulkarni
2020-01-03  0:23     ` Singh, Balbir
2020-01-07  3:48   ` Martin K. Petersen
2020-01-07  3:57     ` James Bottomley
2020-01-07  4:39       ` Martin K. Petersen
2020-01-07 21:37         ` Ewan D. Milne
2020-01-08  2:59           ` Martin K. Petersen
2020-01-08 21:27             ` Ewan D. Milne
2020-01-07 22:28     ` Singh, Balbir
2020-01-08  3:15       ` Martin K. Petersen
2020-01-08  4:20         ` Singh, Balbir
2020-01-08 21:32         ` Ewan D. Milne
2020-01-08 15:06     ` Christoph Hellwig
2020-01-09  2:53       ` Martin K. Petersen
2020-01-06  5:59 ` [resend v1 0/5] Add support for block disk resize notification Bob Liu
2020-01-06  8:47   ` Singh, Balbir
2020-01-06  9:08     ` Bob Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).