| Message ID | 20230330160247.16030-1-hi@alyssa.is (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] loop/009: add test for loop partition uvents | expand |
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+
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
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
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
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>
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 >
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!
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
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