[PATCH] block: Suppress uevent for hidden device when removed

[PATCH] block: Suppress uevent for hidden device when removed

[Daniel Wagner]

register_disk() suppress uevents for devices with the GENHD_FL_HIDDEN
but enables uevents at the end again in order to announce disk after
possible partitions are created.

When the device is removed the uevents are still on and user land sees
'remove' messages for devices which were never 'add'ed to the system.

  KERNEL[95481.571887] remove   /devices/virtual/nvme-fabrics/ctl/nvme5/nvme0c5n1 (block)

Let's suppress the uevents for GENHD_FL_HIDDEN again before calling
device_del() which will write trigger uevents.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 block/genhd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index c55e8f0fced1..ab9ed355bdef 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -731,6 +731,9 @@ void del_gendisk(struct gendisk *disk)
 	if (!sysfs_deprecated)
 		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
 	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
+
+	if (disk->flags & GENHD_FL_HIDDEN)
+		dev_set_uevent_suppress(disk_to_dev(disk), 1);
 	device_del(disk_to_dev(disk));
 }
 EXPORT_SYMBOL(del_gendisk);
-- 
2.30.1

Re: [PATCH] block: Suppress uevent for hidden device when removed

[Martin Wilck]

On Wed, 2021-03-03 at 18:12 +0100, Daniel Wagner wrote:
> register_disk() suppress uevents for devices with the GENHD_FL_HIDDEN
> but enables uevents at the end again in order to announce disk after
> possible partitions are created.
> 
> When the device is removed the uevents are still on and user land
> sees
> 'remove' messages for devices which were never 'add'ed to the system.
> 
>   KERNEL[95481.571887] remove   /devices/virtual/nvme-
> fabrics/ctl/nvme5/nvme0c5n1 (block)
> 
> Let's suppress the uevents for GENHD_FL_HIDDEN again before calling
> device_del() which will write trigger uevents.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  block/genhd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index c55e8f0fced1..ab9ed355bdef 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -731,6 +731,9 @@ void del_gendisk(struct gendisk *disk)
>         if (!sysfs_deprecated)
>                 sysfs_remove_link(block_depr,
> dev_name(disk_to_dev(disk)));
>         pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
> +
> +       if (disk->flags & GENHD_FL_HIDDEN)
> +               dev_set_uevent_suppress(disk_to_dev(disk), 1);
>         device_del(disk_to_dev(disk));
>  }
>  EXPORT_SYMBOL(del_gendisk);

I wonder if it wouldn't be wiser to remove this code

        if (disk->flags & GENHD_FL_HIDDEN) {
                dev_set_uevent_suppress(ddev, 0);
                return;
        }

from register_disk(). The way you did it now, we would receive neither
"add" nor "remove" events in user space, but there might be change
events in between ?

This said, I'd like to raise the question whether it was the right
decision to suppress these uevents in the first place. 8ddcd653257c
("block: introduce GENHD_FL_HIDDEN") simply stated that they aren't
registered as usable block devices. Maybe the kernel should leave the
decision whether or not they are interesting to user space itself?

For example, I've written an extension for multipath-tools some time
ago which displays the topology of NVMe native multipath devices, and
uses sysfs data from the hidden NVMe private namespaces for that
purpose. Not receiving  uevents for them makes it practically
impossible to track the status of these devices.

Regards,
Martin

Re: [PATCH] block: Suppress uevent for hidden device when removed

[Christoph Hellwig]

On Wed, Mar 03, 2021 at 06:30:34PM +0100, Martin Wilck wrote:
> I wonder if it wouldn't be wiser to remove this code
> 
>         if (disk->flags & GENHD_FL_HIDDEN) {
>                 dev_set_uevent_suppress(ddev, 0);
>                 return;
>         }
> 
> from register_disk(). The way you did it now, we would receive neither
> "add" nor "remove" events in user space, but there might be change
> events in between ?

Well, we'll need to keep the return.  That being said keepign the
uevents supressed entirely might be a good idea.