| Message ID | 20250317-loop-uevent-changed-v1-1-cb29cb91b62d@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | loop: Properly send KOBJ_CHANGED uevent for disk device | expand |
| Context | Check | Description |
|---|---|---|
| shin/vmtest-linus-master-PR | fail | PR summary |
| shin/vmtest-linus-master-VM_Test-2 | success | Logs for run-tests-on-kernel |
| shin/vmtest-linus-master-VM_Test-1 | success | Logs for build-kernel |
| shin/vmtest-linus-master-VM_Test-0 | success | Logs for build-kernel |
On Mon, Mar 17, 2025 at 03:13:25PM +0100, Thomas Weißschuh wrote: > The wording "uncork" in the code comment indicates that it is expected that > the suppressed event instances are automatically sent after unsuppressing. > This is not the case, they are discarded. > In effect this means that no "changed" events are emitted on the device > itself by default. On the other hand each discovered partition does trigger > a "changed" event on the loop device itself. Therefore no event is emitted for > devices without partitions. > > This leads to udev missing the device creation and prompting workarounds in > userspace, see the linked util-linux/losetup bug. > > Explicitly emit the events and drop the confusingly worded comments. > > Link: https://github.com/util-linux/util-linux/issues/2434 > Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl") > Cc: stable@vger.kernel.org > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> > --- > drivers/block/loop.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index c05fe27a96b64f1f1ea3868510fdd0c7f4937f55..fbc67ff29e07c15f2e3b3e225a4a37df016fe9de 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -654,8 +654,8 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, > > error = 0; > done: > - /* enable and uncork uevent now that we are done */ > dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); > + kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE); Why not just remove the place where the uevent was suppressed to start with? It feels by manually sending a change event, you are doing exactly what the suppress was trying to prevent, which makes me think this is wrong. thanks, greg k-h
+ Christoph who added the uevent suppression originally On Mon, Mar 17, 2025 at 03:33:55PM +0100, Greg KH wrote: > On Mon, Mar 17, 2025 at 03:13:25PM +0100, Thomas Weißschuh wrote: > > The wording "uncork" in the code comment indicates that it is expected that > > the suppressed event instances are automatically sent after unsuppressing. > > This is not the case, they are discarded. > > In effect this means that no "changed" events are emitted on the device > > itself by default. On the other hand each discovered partition does trigger > > a "changed" event on the loop device itself. Therefore no event is emitted for > > devices without partitions. > > > > This leads to udev missing the device creation and prompting workarounds in > > userspace, see the linked util-linux/losetup bug. > > > > Explicitly emit the events and drop the confusingly worded comments. > > > > Link: https://github.com/util-linux/util-linux/issues/2434 > > Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl") > > Cc: stable@vger.kernel.org > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> > > --- > > drivers/block/loop.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > > index c05fe27a96b64f1f1ea3868510fdd0c7f4937f55..fbc67ff29e07c15f2e3b3e225a4a37df016fe9de 100644 > > --- a/drivers/block/loop.c > > +++ b/drivers/block/loop.c > > @@ -654,8 +654,8 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, > > > > error = 0; > > done: > > - /* enable and uncork uevent now that we are done */ > > dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); > > + kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE); > > Why not just remove the place where the uevent was suppressed to start > with? It feels by manually sending a change event, you are doing > exactly what the suppress was trying to prevent, which makes me think > this is wrong. The suppression was intentionally added in commit 498ef5c777d9 ("loop: suppress uevents while reconfiguring the device") to "make sure userspace reacting to the change event will see the new device state by generating the event only when the device is setup". The device is completely setup after loop_configure() and *I think* the same is true for loop_change_fd(). This commit would also be the correct target for Fixes:. Thomas
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index c05fe27a96b64f1f1ea3868510fdd0c7f4937f55..fbc67ff29e07c15f2e3b3e225a4a37df016fe9de 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -654,8 +654,8 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, error = 0; done: - /* enable and uncork uevent now that we are done */ dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); + kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE); return error; out_err: @@ -1115,8 +1115,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode, if (partscan) clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state); - /* enable and uncork uevent now that we are done */ dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); + kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE); loop_global_unlock(lo, is_loop); if (partscan)
The wording "uncork" in the code comment indicates that it is expected that the suppressed event instances are automatically sent after unsuppressing. This is not the case, they are discarded. In effect this means that no "changed" events are emitted on the device itself by default. On the other hand each discovered partition does trigger a "changed" event on the loop device itself. Therefore no event is emitted for devices without partitions. This leads to udev missing the device creation and prompting workarounds in userspace, see the linked util-linux/losetup bug. Explicitly emit the events and drop the confusingly worded comments. Link: https://github.com/util-linux/util-linux/issues/2434 Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl") Cc: stable@vger.kernel.org Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> --- drivers/block/loop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1 change-id: 20250307-loop-uevent-changed-aa3690f43e03 Best regards,