Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <66a9b7ff4958ab990f58a3dad8152d00c59775ce.camel@suse.com>
Subject: Re: [PATCH] block: Suppress uevent for hidden device when removed
From:   Martin Wilck <mwilck@suse.com>
To:     Daniel Wagner <dwagner@suse.de>, linux-block@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Jens Axboe <axboe@kernel.dk>,
        Christoph Hellwig <hch@lst.de>
Date:   Wed, 03 Mar 2021 18:30:34 +0100
In-Reply-To: <20210303171201.8432-1-dwagner@suse.de>
References: <20210303171201.8432-1-dwagner@suse.de>
Content-Type: text/plain; charset="ISO-8859-15"
User-Agent: Evolution 3.38.2 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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