[PATCH] loop: LOOP_CONFIGURE: send uevents for partitions

[PATCH] loop: LOOP_CONFIGURE: send uevents for partitions

[Christoph Hellwig]

From: Alyssa Ross <hi@alyssa.is>

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

Re: [PATCH] loop: LOOP_CONFIGURE: send uevents for partitions

[Chaitanya Kulkarni]

Alyssa,

On 3/20/23 05:54, Christoph Hellwig wrote:
> From: Alyssa Ross <hi@alyssa.is>
>
> 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")
>

Can you please add the testcase in the blktests to test the
above mentioned behavior ?

-ck

[PATCH blktests] loop/009: add test for loop partition uvents

[Alyssa Ross]

Link: https://lore.kernel.org/r/20230320125430.55367-1-hch@lst.de/
Suggested-by: Chaitanya Kulkarni <chaitanyak@nvidia.com>
Signed-off-by: Alyssa Ross <hi@alyssa.is>
---
 tests/loop/009     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/loop/009.out |  3 +++
 2 files changed, 65 insertions(+)
 create mode 100755 tests/loop/009
 create mode 100644 tests/loop/009.out

diff --git a/tests/loop/009 b/tests/loop/009
new file mode 100755
index 0000000..dfa9de3
--- /dev/null
+++ b/tests/loop/009
@@ -0,0 +1,62 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright 2023 Alyssa Ross
+#
+# Regression test for patch "loop: LOOP_CONFIGURE: send uevents for partitions".
+
+. tests/loop/rc
+
+DESCRIPTION="check that LOOP_CONFIGURE sends uevents for partitions"
+
+QUICK=1
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	# Make a disk image with a partition.
+	truncate -s 3MiB "$TMPDIR/img"
+	sfdisk "$TMPDIR/img" >"$FULL" <<EOF
+label: gpt
+size=1MiB
+EOF
+
+	mkfifo "$TMPDIR/mon"
+	timeout 5 udevadm monitor -ks block/partition >"$TMPDIR/mon" &
+
+	# Open the fifo for reading, and wait for udevadm monitor to start.
+	exec 3< "$TMPDIR/mon"
+	read -r _ <&3
+	read -r _ <&3
+	read -r _ <&3
+
+	dev="$(losetup -f)"
+
+	# The default udev behavior is to watch loop devices, which means that
+	# udevd will explicitly prompt the kernel to rescan the partitions with
+	# ioctl(BLKRRPART).  We want to make sure we're getting uevents from
+	# ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
+	# avoid false positives.
+	echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
+		>/run/udev/rules.d/99-blktests-$$.rules
+	udevadm control -R
+
+	# Open and close the loop device for writing, to trigger the inotify
+	# event udevd had already started listening for.
+	: > "$dev"
+
+	# Wait for udev to have processed the inotify event.
+	udevadm control --ping
+
+	losetup -P "$dev" "$TMPDIR/img"
+
+	# Wait for at most 1 add event so we don't need to wait for timeout if
+	# we get what we're looking for.
+	<&3 grep -m 1 '^KERNEL\[.*\] add' |
+		sed -e 's/\[.*\]//' -e 's/loop[0-9]\+/loop_/g'
+
+	rm /run/udev/rules.d/99-blktests-$$.rules
+	udevadm control -R
+	losetup -d "$dev"
+
+	echo "Test complete"
+}
diff --git a/tests/loop/009.out b/tests/loop/009.out
new file mode 100644
index 0000000..658dcff
--- /dev/null
+++ b/tests/loop/009.out
@@ -0,0 +1,3 @@
+Running loop/009
+KERNEL add      /devices/virtual/block/loop_/loop_p1 (block)
+Test complete
-- 
2.37.1

Re: [PATCH blktests] loop/009: add test for loop partition uvents

[kernel test robot]

Hi Alyssa,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc4 next-20230330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alyssa-Ross/loop-009-add-test-for-loop-partition-uvents/20230331-001157
patch link:    https://lore.kernel.org/r/20230330160247.16030-1-hi%40alyssa.is
patch subject: [PATCH blktests] loop/009: add test for loop partition uvents
reproduce:
        scripts/spdxcheck.py

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303310316.QS2vADHM-lkp@intel.com/

spdxcheck warnings: (new ones prefixed by >>)
>> tests/loop/009: 2:27 Invalid License ID: GPL-3.0+

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

LKP kernel test robot and blktests patches

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Hi LKP team,

blktests's CONTRIBUTING.md says that blktests patches should be
sent either on GitHub, or to linux-block with the "PATCH blktests"
subject prefix.  I just sent such a patch, and because it was just
adding a file, the kernel test robot happily applied and tested it as if
it was a kernel patch, which naturally resulted in it finding problems.

Maybe the kernel test robot should skip patches starting with
"[PATCH blktests]"?

n Fri, Mar 31, 2023 at 03:22:28AM +0800, kernel test robot wrote:
> Hi Alyssa,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.3-rc4 next-20230330]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Alyssa-Ross/loop-009-add-test-for-loop-partition-uvents/20230331-001157
> patch link:    https://lore.kernel.org/r/20230330160247.16030-1-hi%40alyssa.is
> patch subject: [PATCH blktests] loop/009: add test for loop partition uvents
> reproduce:
>         scripts/spdxcheck.py
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202303310316.QS2vADHM-lkp@intel.com/
>
> spdxcheck warnings: (new ones prefixed by >>)
> >> tests/loop/009: 2:27 Invalid License ID: GPL-3.0+
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: LKP kernel test robot and blktests patches

[Yujie Liu]

Hi Alyssa,

On Thu, Mar 30, 2023 at 07:50:53PM +0000, Alyssa Ross wrote:
> Hi LKP team,
> 
> blktests's CONTRIBUTING.md says that blktests patches should be
> sent either on GitHub, or to linux-block with the "PATCH blktests"
> subject prefix.  I just sent such a patch, and because it was just
> adding a file, the kernel test robot happily applied and tested it as if
> it was a kernel patch, which naturally resulted in it finding problems.
> 
> Maybe the kernel test robot should skip patches starting with
> "[PATCH blktests]"?

Sorry for wrongly applying this as a kernel patch. We've configured the
robot to skip blktests patches.

--
Best Regards,
Yujie

> n Fri, Mar 31, 2023 at 03:22:28AM +0800, kernel test robot wrote:
> > Hi Alyssa,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on linus/master]
> > [also build test WARNING on v6.3-rc4 next-20230330]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Alyssa-Ross/loop-009-add-test-for-loop-partition-uvents/20230331-001157
> > patch link:    https://lore.kernel.org/r/20230330160247.16030-1-hi%40alyssa.is
> > patch subject: [PATCH blktests] loop/009: add test for loop partition uvents
> > reproduce:
> >         scripts/spdxcheck.py
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Link: https://lore.kernel.org/oe-kbuild-all/202303310316.QS2vADHM-lkp@intel.com/
> >
> > spdxcheck warnings: (new ones prefixed by >>)
> > >> tests/loop/009: 2:27 Invalid License ID: GPL-3.0+
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests

Re: [PATCH blktests] loop/009: add test for loop partition uvents

[Chaitanya Kulkarni]

On 3/30/2023 9:02 AM, Alyssa Ross wrote:
> Link: https://lore.kernel.org/r/20230320125430.55367-1-hch@lst.de/
> Suggested-by: Chaitanya Kulkarni <chaitanyak@nvidia.com>
> Signed-off-by: Alyssa Ross <hi@alyssa.is>

Thanks a lot for this, overall this looks good to me.

This does exactly the testing for loop configure that your patch
is for, let's just make sure to apply this patch once your kernel
patch is merged.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH blktests] loop/009: add test for loop partition uvents

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On Tue, Apr 04, 2023 at 06:29:00PM +0000, Chaitanya Kulkarni wrote:
> On 3/30/2023 9:02 AM, Alyssa Ross wrote:
> > Link: https://lore.kernel.org/r/20230320125430.55367-1-hch@lst.de/
> > Suggested-by: Chaitanya Kulkarni <chaitanyak@nvidia.com>
> > Signed-off-by: Alyssa Ross <hi@alyssa.is>
>
> Thanks a lot for this, overall this looks good to me.
>
> This does exactly the testing for loop configure that your patch
> is for, let's just make sure to apply this patch once your kernel
> patch is merged.

The kernel patch was included in v6.3-rc5. :)

> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH blktests] loop/009: add test for loop partition uvents

[Shin'ichiro Kawasaki]

Hello Alyssa, thanks for the patch and sorry for this late response.

Please find one comment in line. Other than that, this patch looks good to me.
I also ran the test case in my environment and confirmed that it passes with
the kernel fix. Good.

On Mar 30, 2023 / 16:02, Alyssa Ross wrote:
> Link: https://lore.kernel.org/r/20230320125430.55367-1-hch@lst.de/
> Suggested-by: Chaitanya Kulkarni <chaitanyak@nvidia.com>
> Signed-off-by: Alyssa Ross <hi@alyssa.is>
> ---
>  tests/loop/009     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/loop/009.out |  3 +++
>  2 files changed, 65 insertions(+)
>  create mode 100755 tests/loop/009
>  create mode 100644 tests/loop/009.out
> 
> diff --git a/tests/loop/009 b/tests/loop/009
> new file mode 100755
> index 0000000..dfa9de3
> --- /dev/null
> +++ b/tests/loop/009
> @@ -0,0 +1,62 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright 2023 Alyssa Ross
> +#
> +# Regression test for patch "loop: LOOP_CONFIGURE: send uevents for partitions".
> +
> +. tests/loop/rc
> +
> +DESCRIPTION="check that LOOP_CONFIGURE sends uevents for partitions"
> +
> +QUICK=1
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# Make a disk image with a partition.
> +	truncate -s 3MiB "$TMPDIR/img"
> +	sfdisk "$TMPDIR/img" >"$FULL" <<EOF
> +label: gpt
> +size=1MiB
> +EOF
> +
> +	mkfifo "$TMPDIR/mon"
> +	timeout 5 udevadm monitor -ks block/partition >"$TMPDIR/mon" &
> +
> +	# Open the fifo for reading, and wait for udevadm monitor to start.
> +	exec 3< "$TMPDIR/mon"
> +	read -r _ <&3
> +	read -r _ <&3
> +	read -r _ <&3
> +
> +	dev="$(losetup -f)"
> +
> +	# The default udev behavior is to watch loop devices, which means that
> +	# udevd will explicitly prompt the kernel to rescan the partitions with
> +	# ioctl(BLKRRPART).  We want to make sure we're getting uevents from
> +	# ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
> +	# avoid false positives.
> +	echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
> +		>/run/udev/rules.d/99-blktests-$$.rules

On Fedora Server 37, the line above prints a failure message because the
directory /run/udev/rules.d/ does not exist. To avoid it, I needed the change
below. I suggest to apply this change.

diff --git a/tests/loop/009 b/tests/loop/009
index dfa9de3..2b7a042 100755
--- a/tests/loop/009
+++ b/tests/loop/009
@@ -36,6 +36,7 @@ EOF
        # ioctl(BLKRRPART).  We want to make sure we're getting uevents from
        # ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
        # avoid false positives.
+       [[ ! -d /run/udev/rules.d ]] && mkdir -p /run/udev/rules.d
        echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
                >/run/udev/rules.d/99-blktests-$$.rules
        udevadm control -R

> +	udevadm control -R
> +
> +	# Open and close the loop device for writing, to trigger the inotify
> +	# event udevd had already started listening for.
> +	: > "$dev"
> +
> +	# Wait for udev to have processed the inotify event.
> +	udevadm control --ping
> +
> +	losetup -P "$dev" "$TMPDIR/img"
> +
> +	# Wait for at most 1 add event so we don't need to wait for timeout if
> +	# we get what we're looking for.
> +	<&3 grep -m 1 '^KERNEL\[.*\] add' |
> +		sed -e 's/\[.*\]//' -e 's/loop[0-9]\+/loop_/g'
> +
> +	rm /run/udev/rules.d/99-blktests-$$.rules
> +	udevadm control -R
> +	losetup -d "$dev"
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/loop/009.out b/tests/loop/009.out
> new file mode 100644
> index 0000000..658dcff
> --- /dev/null
> +++ b/tests/loop/009.out
> @@ -0,0 +1,3 @@
> +Running loop/009
> +KERNEL add      /devices/virtual/block/loop_/loop_p1 (block)
> +Test complete
> -- 
> 2.37.1
> 

Re: [PATCH blktests] loop/009: add test for loop partition uvents

[Shinichiro Kawasaki]

On Apr 06, 2023 / 19:30, Shin'ichiro Kawasaki wrote:
[...]
> > +	# The default udev behavior is to watch loop devices, which means that
> > +	# udevd will explicitly prompt the kernel to rescan the partitions with
> > +	# ioctl(BLKRRPART).  We want to make sure we're getting uevents from
> > +	# ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
> > +	# avoid false positives.
> > +	echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
> > +		>/run/udev/rules.d/99-blktests-$$.rules
> 
> On Fedora Server 37, the line above prints a failure message because the
> directory /run/udev/rules.d/ does not exist. To avoid it, I needed the change
> below. I suggest to apply this change.
> 
> diff --git a/tests/loop/009 b/tests/loop/009
> index dfa9de3..2b7a042 100755
> --- a/tests/loop/009
> +++ b/tests/loop/009
> @@ -36,6 +36,7 @@ EOF
>         # ioctl(BLKRRPART).  We want to make sure we're getting uevents from
>         # ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
>         # avoid false positives.
> +       [[ ! -d /run/udev/rules.d ]] && mkdir -p /run/udev/rules.d
>         echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
>                 >/run/udev/rules.d/99-blktests-$$.rules
>         udevadm control -R
>

I've applied the patch with the edit above. Thanks!

Re: [PATCH] loop: LOOP_CONFIGURE: send uevents for partitions

[Jens Axboe]

On Mon, 20 Mar 2023 13:54:30 +0100, Christoph Hellwig wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] loop: LOOP_CONFIGURE: send uevents for partitions
      (no commit info)

Best regards,
-- 
Jens Axboe