| Message ID | 20230312191031.551204-1-hi@alyssa.is (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] loop: LOOP_CONFIGURE: send uevents for partitions | expand |
On Sun, Mar 12, 2023 at 07:10:31PM +0000, Alyssa Ross wrote: > + * Now that we are done, reread the partitions with uevent > + * re-enabled if appropriate to let userspace know about the > + * changes. > + */ > + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), !partscan_uevent); > + if (partscan) > + loop_reread_partitions(lo); > + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); What worries me here is that you move the partition re-read out of the exclusive claim, which is another potentially user visible change (and user visible behavior changes are a field of landmines in loop as you have noticed). But in the end we only need to suppress the events until Lo_Bound is set. So something like the patch below that reduces the no even critical section might do the job? diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 839373451c2b7d..9d61c027185141 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, /* This is safe, since we have a reference from open(). */ __module_get(THIS_MODULE); - /* suppress uevents while reconfiguring the device */ - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); - /* * If we don't hold exclusive handle for the device, upgrade to it * here to avoid changing device under exclusive owner. @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, } } + /* suppress uevents while reconfiguring the device */ + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); + disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0); @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_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); + loop_global_unlock(lo, is_loop); if (partscan) loop_reread_partitions(lo); + if (!(mode & FMODE_EXCL)) bd_abort_claiming(bdev, loop_configure); - error = 0; -done: - /* enable and uncork uevent now that we are done */ - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); - return error; + return 0; out_unlock: loop_global_unlock(lo, is_loop); @@ -1130,7 +1130,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, fput(file); /* This is safe: open() is still holding a reference. */ module_put(THIS_MODULE); - goto done; + return error; } static void __loop_clr_fd(struct loop_device *lo, bool release)
On Wed, Mar 15, 2023 at 08:48:40AM -0700, Christoph Hellwig wrote: > On Sun, Mar 12, 2023 at 07:10:31PM +0000, Alyssa Ross wrote: > > + * Now that we are done, reread the partitions with uevent > > + * re-enabled if appropriate to let userspace know about the > > + * changes. > > + */ > > + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), !partscan_uevent); > > + if (partscan) > > + loop_reread_partitions(lo); > > + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); > > What worries me here is that you move the partition re-read out of > the exclusive claim, which is another potentially user visible > change (and user visible behavior changes are a field of landmines > in loop as you have noticed). Makes sense. > But in the end we only need to suppress the events until Lo_Bound > is set. So something like the patch below that reduces the no even > critical section might do the job? If you say so! I had trouble understanding which parts of the function uevents needed to be suppressed for, so I was trying to move as little as possible out of that section. What happens next? I'm still getting up to speed on the kernel development process — will you submit this as a patch with a patch body and a S-o-b? Or am I supposed to do something with it? I know enough to know that I should give you a: Tested-by: Alyssa Ross <hi@alyssa.is> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 839373451c2b7d..9d61c027185141 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, > /* This is safe, since we have a reference from open(). */ > __module_get(THIS_MODULE); > > - /* suppress uevents while reconfiguring the device */ > - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); > - > /* > * If we don't hold exclusive handle for the device, upgrade to it > * here to avoid changing device under exclusive owner. > @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, > } > } > > + /* suppress uevents while reconfiguring the device */ > + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); > + > disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); > set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0); > > @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_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); > + > loop_global_unlock(lo, is_loop); > if (partscan) > loop_reread_partitions(lo); > + > if (!(mode & FMODE_EXCL)) > bd_abort_claiming(bdev, loop_configure); > > - error = 0; > -done: > - /* enable and uncork uevent now that we are done */ > - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); > - return error; > + return 0; > > out_unlock: > loop_global_unlock(lo, is_loop); > @@ -1130,7 +1130,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, > fput(file); > /* This is safe: open() is still holding a reference. */ > module_put(THIS_MODULE); > - goto done; > + return error; > } > > static void __loop_clr_fd(struct loop_device *lo, bool release)
On Sat, Mar 18, 2023 at 01:50:05AM +0000, Alyssa Ross wrote: > If you say so! I had trouble understanding which parts of the function > uevents needed to be suppressed for, so I was trying to move as little > as possible out of that section. > > What happens next? I'm still getting up to speed on the kernel > development process — will you submit this as a patch with a patch body > and a S-o-b? Or am I supposed to do something with it? Given that you're done all the hard work, and I've just reduced the critical section, I'd prefer to give all the credit to you. If you're fine with it, I'll send out this version later: --- From 4648015b4193c81d3de8c1632876314b4a2ab40d Mon Sep 17 00:00:00 2001 Subject: loop: LOOP_CONFIGURE: send uevents for partitions LOOP_CONFIGURE is, as far as I understand it, supposed to be a way to combine LOOP_SET_FD and LOOP_SET_STATUS64 into a single syscall. When using LOOP_SET_FD+LOOP_SET_STATUS64, a single uevent would be sent for each partition found on the loop device after the second ioctl(), but when using LOOP_CONFIGURE, no such uevent was being sent. In the old setup, uevents are disabled for LOOP_SET_FD, but not for LOOP_SET_STATUS64. This makes sense, as it prevents uevents being sent for a partially configured device during LOOP_SET_FD - they're only sent at the end of LOOP_SET_STATUS64. But for LOOP_CONFIGURE, uevents were disabled for the entire operation, so that final notification was never issued. To fix this, reduce the critical section to exclude the loop_reread_partitions() call, which causes the uevents to be issued, to after uevents are re-enabled, matching the behaviour of the LOOP_SET_FD+LOOP_SET_STATUS64 combination. I noticed this because Busybox's losetup program recently changed from using LOOP_SET_FD+LOOP_SET_STATUS64 to LOOP_CONFIGURE, and this broke my setup, for which I want a notification from the kernel any time a new partition becomes available. Signed-off-by: Alyssa Ross <hi@alyssa.is> [hch: reduced the critical section] Signed-off-by: Christoph Hellwig <hch@lst.de> Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl") --- drivers/block/loop.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 839373451c2b7d..9d61c027185141 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, /* This is safe, since we have a reference from open(). */ __module_get(THIS_MODULE); - /* suppress uevents while reconfiguring the device */ - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); - /* * If we don't hold exclusive handle for the device, upgrade to it * here to avoid changing device under exclusive owner. @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, } } + /* suppress uevents while reconfiguring the device */ + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); + disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0); @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_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); + loop_global_unlock(lo, is_loop); if (partscan) loop_reread_partitions(lo); + if (!(mode & FMODE_EXCL)) bd_abort_claiming(bdev, loop_configure); - error = 0; -done: - /* enable and uncork uevent now that we are done */ - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); - return error; + return 0; out_unlock: loop_global_unlock(lo, is_loop); @@ -1130,7 +1130,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, fput(file); /* This is safe: open() is still holding a reference. */ module_put(THIS_MODULE); - goto done; + return error; } static void __loop_clr_fd(struct loop_device *lo, bool release)
On Sun, Mar 19, 2023 at 11:22:25PM -0700, Christoph Hellwig wrote: > On Sat, Mar 18, 2023 at 01:50:05AM +0000, Alyssa Ross wrote: > > What happens next? I'm still getting up to speed on the kernel > > development process — will you submit this as a patch with a patch body > > and a S-o-b? Or am I supposed to do something with it? > > Given that you're done all the hard work, and I've just reduced the > critical section, I'd prefer to give all the credit to you. If you're > fine with it, I'll send out this version later: LGTM, thanks! > --- > From 4648015b4193c81d3de8c1632876314b4a2ab40d Mon Sep 17 00:00:00 2001 > Subject: loop: LOOP_CONFIGURE: send uevents for partitions > > LOOP_CONFIGURE is, as far as I understand it, supposed to be a way to > combine LOOP_SET_FD and LOOP_SET_STATUS64 into a single syscall. When > using LOOP_SET_FD+LOOP_SET_STATUS64, a single uevent would be sent for > each partition found on the loop device after the second ioctl(), but > when using LOOP_CONFIGURE, no such uevent was being sent. > > In the old setup, uevents are disabled for LOOP_SET_FD, but not for > LOOP_SET_STATUS64. This makes sense, as it prevents uevents being > sent for a partially configured device during LOOP_SET_FD - they're > only sent at the end of LOOP_SET_STATUS64. But for LOOP_CONFIGURE, > uevents were disabled for the entire operation, so that final > notification was never issued. To fix this, reduce the critical > section to exclude the loop_reread_partitions() call, which causes > the uevents to be issued, to after uevents are re-enabled, matching > the behaviour of the LOOP_SET_FD+LOOP_SET_STATUS64 combination. > > I noticed this because Busybox's losetup program recently changed from > using LOOP_SET_FD+LOOP_SET_STATUS64 to LOOP_CONFIGURE, and this broke > my setup, for which I want a notification from the kernel any time a > new partition becomes available. > > Signed-off-by: Alyssa Ross <hi@alyssa.is> > [hch: reduced the critical section] > Signed-off-by: Christoph Hellwig <hch@lst.de> > Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl") > --- > drivers/block/loop.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 839373451c2b7d..9d61c027185141 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, > /* This is safe, since we have a reference from open(). */ > __module_get(THIS_MODULE); > > - /* suppress uevents while reconfiguring the device */ > - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); > - > /* > * If we don't hold exclusive handle for the device, upgrade to it > * here to avoid changing device under exclusive owner. > @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, > } > } > > + /* suppress uevents while reconfiguring the device */ > + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); > + > disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); > set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0); > > @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo, fmode_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); > + > loop_global_unlock(lo, is_loop); > if (partscan) > loop_reread_partitions(lo); > + > if (!(mode & FMODE_EXCL)) > bd_abort_claiming(bdev, loop_configure); > > - error = 0; > -done: > - /* enable and uncork uevent now that we are done */ > - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); > - return error; > + return 0; > > out_unlock: > loop_global_unlock(lo, is_loop); > @@ -1130,7 +1130,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, > fput(file); > /* This is safe: open() is still holding a reference. */ > module_put(THIS_MODULE); > - goto done; > + return error; > } > > static void __loop_clr_fd(struct loop_device *lo, bool release) > -- > 2.39.2 >
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 839373451c2b..f00a0209b522 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -992,7 +992,8 @@ loop_set_status_from_info(struct loop_device *lo, static int loop_configure(struct loop_device *lo, fmode_t mode, struct block_device *bdev, - const struct loop_config *config) + const struct loop_config *config, + bool partscan_uevent) { struct file *file = fget(config->fd); struct inode *inode; @@ -1110,15 +1111,21 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state); loop_global_unlock(lo, is_loop); - if (partscan) - loop_reread_partitions(lo); if (!(mode & FMODE_EXCL)) bd_abort_claiming(bdev, loop_configure); + /* + * Now that we are done, reread the partitions with uevent + * re-enabled if appropriate to let userspace know about the + * changes. + */ + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), !partscan_uevent); + if (partscan) + loop_reread_partitions(lo); + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); + error = 0; done: - /* enable and uncork uevent now that we are done */ - dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); return error; out_unlock: @@ -1130,6 +1136,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode, fput(file); /* This is safe: open() is still holding a reference. */ module_put(THIS_MODULE); + dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0); goto done; } @@ -1547,7 +1554,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, memset(&config, 0, sizeof(config)); config.fd = arg; - return loop_configure(lo, mode, bdev, &config); + return loop_configure(lo, mode, bdev, &config, false); } case LOOP_CONFIGURE: { struct loop_config config; @@ -1555,7 +1562,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, if (copy_from_user(&config, argp, sizeof(config))) return -EFAULT; - return loop_configure(lo, mode, bdev, &config); + return loop_configure(lo, mode, bdev, &config, true); } case LOOP_CHANGE_FD: return loop_change_fd(lo, bdev, arg);
LOOP_CONFIGURE is, as far as I understand it, supposed to be a way to combine LOOP_SET_FD and LOOP_SET_STATUS64 into a single syscall. When using LOOP_SET_FD+LOOP_SET_STATUS64, a single uevent would be sent for each partition found on the loop device after the second ioctl(), but when using LOOP_CONFIGURE, no such uevent was being sent. In the old setup, uevents are disabled for LOOP_SET_FD, but not for LOOP_SET_STATUS64. This makes sense, as it prevents uevents being sent for a partially configured device during LOOP_SET_FD — they're only sent at the end of LOOP_SET_STATUS64. But for LOOP_CONFIGURE, uevents were disabled for the entire operation, so that final notification was never issued. To fix this, I've moved the loop_reread_partitions() call, which causes the uevents to be issued, to after uevents are re-enabled, matching the behaviour of the LOOP_SET_FD+LOOP_SET_STATUS64 combination. I noticed this because Busybox's losetup program recently changed from using LOOP_SET_FD+LOOP_SET_STATUS64 to LOOP_CONFIGURE, and this broke my setup, for which I want a notification from the kernel any time a new partition becomes available. Signed-off-by: Alyssa Ross <hi@alyssa.is> Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl") --- v1: https://lore.kernel.org/linux-block/20230221222847.607096-1-hi@alyssa.is/ v1 was an RFC, because I was looking for advice on how to handle distinguishing between LOOP_SET_FD with non-zero max_part (in which case partscan will be true, but a uevent should not be emitted), and LOOP_CONFIGURE (where a uevent should be emitted). I didn't hear anything, but I did some experimentation of my own, and adding a partscan_uevent parameter to distinguish between LOOP_SET_FD and LOOP_CONFIGURE feels like the least bad solution to me. drivers/block/loop.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)