diff mbox series

[v2] loop: properly send KOBJ_CHANGED uevent for disk device

Message ID 20250415-loop-uevent-changed-v2-1-0c4e6a923b2a@linutronix.de (mailing list archive)
State New
Headers show
Series [v2] loop: properly send KOBJ_CHANGED uevent for disk device | expand

Commit Message

Thomas Weißschuh April 15, 2025, 8:51 a.m. UTC 
The original commit message and the wording "uncork" in the code comment
indicate that it is expected that the suppressed event instances are
automatically sent after unsuppressing.
This is not the case, instead they are discarded.
In effect this means that no "changed" events are emitted on the device
itself by default.
While each discovered partition does trigger a changed event on the
device, devices without partitions don't have any event emitted.

This makes udev miss the device creation and prompted 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: 498ef5c777d9 ("loop: suppress uevents while reconfiguring the device")
Cc: stable@vger.kernel.org
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Changes in v2:
- Use correct Fixes tag
- Rework commit message slightly
- Rebase onto v6.15-rc1
- Link to v1: https://lore.kernel.org/r/20250317-loop-uevent-changed-v1-1-cb29cb91b62d@linutronix.de
---
 drivers/block/loop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250307-loop-uevent-changed-aa3690f43e03

Best regards,

Comments

Greg KH April 15, 2025, 10:18 a.m. UTC | #1 
On Tue, Apr 15, 2025 at 10:51:47AM +0200, Thomas Weißschuh wrote:
> The original commit message and the wording "uncork" in the code comment
> indicate that it is expected that the suppressed event instances are
> automatically sent after unsuppressing.
> This is not the case, instead they are discarded.
> In effect this means that no "changed" events are emitted on the device
> itself by default.
> While each discovered partition does trigger a changed event on the
> device, devices without partitions don't have any event emitted.
> 
> This makes udev miss the device creation and prompted 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: 498ef5c777d9 ("loop: suppress uevents while reconfiguring the device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> Changes in v2:
> - Use correct Fixes tag
> - Rework commit message slightly
> - Rebase onto v6.15-rc1
> - Link to v1: https://lore.kernel.org/r/20250317-loop-uevent-changed-v1-1-cb29cb91b62d@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 674527d770dc669e982a2b441af1171559aa427c..09a725710a21171e0adf5888f929ccaf94e98992 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -667,8 +667,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:
> @@ -1129,8 +1129,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)
> 
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250307-loop-uevent-changed-aa3690f43e03
> 
> Best regards,
> -- 
> Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Jan Kara April 15, 2025, 10:24 a.m. UTC | #2 
On Tue 15-04-25 10:51:47, Thomas Weißschuh wrote:
> The original commit message and the wording "uncork" in the code comment
> indicate that it is expected that the suppressed event instances are
> automatically sent after unsuppressing.
> This is not the case, instead they are discarded.
> In effect this means that no "changed" events are emitted on the device
> itself by default.
> While each discovered partition does trigger a changed event on the
> device, devices without partitions don't have any event emitted.
> 
> This makes udev miss the device creation and prompted 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: 498ef5c777d9 ("loop: suppress uevents while reconfiguring the device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Thanks for the fix! When reading the code, I'm a bit curious: What is
actually generating events for partitions with loop_change_fd() call?
Because there loop_reread_partitions() still happens with uevents
supressed... I suspect event supressing there should be shorter.

								Honza

> ---
> Changes in v2:
> - Use correct Fixes tag
> - Rework commit message slightly
> - Rebase onto v6.15-rc1
> - Link to v1: https://lore.kernel.org/r/20250317-loop-uevent-changed-v1-1-cb29cb91b62d@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 674527d770dc669e982a2b441af1171559aa427c..09a725710a21171e0adf5888f929ccaf94e98992 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -667,8 +667,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:
> @@ -1129,8 +1129,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)
> 
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250307-loop-uevent-changed-aa3690f43e03
> 
> Best regards,
> -- 
> Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>
Thomas Weißschuh April 15, 2025, 11:07 a.m. UTC | #3 
On Tue, Apr 15, 2025 at 12:24:55PM +0200, Jan Kara wrote:
> On Tue 15-04-25 10:51:47, Thomas Weißschuh wrote:
> > The original commit message and the wording "uncork" in the code comment
> > indicate that it is expected that the suppressed event instances are
> > automatically sent after unsuppressing.
> > This is not the case, instead they are discarded.
> > In effect this means that no "changed" events are emitted on the device
> > itself by default.
> > While each discovered partition does trigger a changed event on the
> > device, devices without partitions don't have any event emitted.
> > 
> > This makes udev miss the device creation and prompted 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: 498ef5c777d9 ("loop: suppress uevents while reconfiguring the device")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> 
> Thanks for the fix! When reading the code, I'm a bit curious: What is
> actually generating events for partitions with loop_change_fd() call?
> Because there loop_reread_partitions() still happens with uevents
> supressed... I suspect event supressing there should be shorter.

Makes sense.
For loop_configure() this was fixed in
commit bb430b694226 ("loop: LOOP_CONFIGURE: send uevents for partitions").
I guess we need the same for loop_change_fd().

I'm not entirely sure on how to order the commits or if they should be
folded together.
My current preference is to first have the current patch under discussion and
then the fix for loop_change_fd().


Thomas

> > ---
> > Changes in v2:
> > - Use correct Fixes tag
> > - Rework commit message slightly
> > - Rebase onto v6.15-rc1
> > - Link to v1: https://lore.kernel.org/r/20250317-loop-uevent-changed-v1-1-cb29cb91b62d@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 674527d770dc669e982a2b441af1171559aa427c..09a725710a21171e0adf5888f929ccaf94e98992 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -667,8 +667,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:
> > @@ -1129,8 +1129,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)
> > 
> > ---
> > base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> > change-id: 20250307-loop-uevent-changed-aa3690f43e03
> > 
> > Best regards,
> > -- 
> > Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
Jens Axboe April 15, 2025, 1:48 p.m. UTC | #4 
On Tue, 15 Apr 2025 10:51:47 +0200, Thomas Weißschuh wrote:
> The original commit message and the wording "uncork" in the code comment
> indicate that it is expected that the suppressed event instances are
> automatically sent after unsuppressing.
> This is not the case, instead they are discarded.
> In effect this means that no "changed" events are emitted on the device
> itself by default.
> While each discovered partition does trigger a changed event on the
> device, devices without partitions don't have any event emitted.
> 
> [...]

Applied, thanks!

[1/1] loop: properly send KOBJ_CHANGED uevent for disk device
      commit: 7ed2a771b5fb3edee9c4608181235c30b40bb042

Best regards,
Jan Kara April 15, 2025, 4:33 p.m. UTC | #5 
On Tue 15-04-25 13:07:48, Thomas Weißschuh wrote:
> On Tue, Apr 15, 2025 at 12:24:55PM +0200, Jan Kara wrote:
> > On Tue 15-04-25 10:51:47, Thomas Weißschuh wrote:
> > > The original commit message and the wording "uncork" in the code comment
> > > indicate that it is expected that the suppressed event instances are
> > > automatically sent after unsuppressing.
> > > This is not the case, instead they are discarded.
> > > In effect this means that no "changed" events are emitted on the device
> > > itself by default.
> > > While each discovered partition does trigger a changed event on the
> > > device, devices without partitions don't have any event emitted.
> > > 
> > > This makes udev miss the device creation and prompted 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: 498ef5c777d9 ("loop: suppress uevents while reconfiguring the device")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > 
> > Thanks for the fix! When reading the code, I'm a bit curious: What is
> > actually generating events for partitions with loop_change_fd() call?
> > Because there loop_reread_partitions() still happens with uevents
> > supressed... I suspect event supressing there should be shorter.
> 
> Makes sense.
> For loop_configure() this was fixed in
> commit bb430b694226 ("loop: LOOP_CONFIGURE: send uevents for partitions").
> I guess we need the same for loop_change_fd().
> 
> I'm not entirely sure on how to order the commits or if they should be
> folded together.
> My current preference is to first have the current patch under discussion and
> then the fix for loop_change_fd().

Yeah, whatever. I was asking mainly to make sure my understanding of the
current code is correct :). With this one feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
diff mbox series

Patch

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 674527d770dc669e982a2b441af1171559aa427c..09a725710a21171e0adf5888f929ccaf94e98992 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -667,8 +667,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:
@@ -1129,8 +1129,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)