| Message ID | 1552563917-8388-2-git-send-email-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests,1/2] loop/001: fix typo 'paritition' to 'partition' | expand |
On Thu, Mar 14, 2019 at 07:45:17PM +0800, Dongli Zhang wrote: > loop/001 does not test whether all partitions are removed successfully > during loop device partition scanning. As a result, the regression > introduced by 0da03cab87e6 ("loop: Fix deadlock when calling > blkdev_reread_part()") can not be detected. > > The regression will generate below message in dmesg: > > [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > > and leave orphan partitions like below: > > - /dev/loop0p1 > - /sys/block/loop0/loop0p1 > > This patch verifies all partitions are removed by checking if there is > /sys/block/loopX/loopXpY left. The expected number of partitions left is 0. Thanks for the test! A couple of comments below. > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > tests/loop/001 | 5 +++++ > tests/loop/001.out | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/tests/loop/001 b/tests/loop/001 > index 47f760a..a0326b7 100755 > --- a/tests/loop/001 > +++ b/tests/loop/001 > @@ -4,6 +4,9 @@ > # > # Test loop device partition scanning. Regression test for commit e02898b42380 > # ("loop: fix LO_FLAGS_PARTSCAN hang"). > +# > +# Test loop device paritition scanning. Regression test for commit 758a58d0bc67 > +# ("loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). These can just be combined to # Test loop device partition scanning. Regression test for commits e02898b42380 # ("loop: fix LO_FLAGS_PARTSCAN hang") and 758a58d0bc67 ("loop: set # GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > . tests/loop/rc > > @@ -24,9 +27,11 @@ test() { > mkpart primary 50% 100% > > loop_device="$(losetup -P -f --show "$TMPDIR/img")" > + loop_name=${loop_device:5} > lsblk -ln "$loop_device" | wc -l > > losetup -d "$loop_device" > + ls /sys/block/$loop_name | grep loop | wc -l We can just repeat the same `lsblk -ln "$loop_device" | wc -l` from earlier, right? That's a bit cleaner than the hardcoded string slicing and ls. > rm "$TMPDIR/img" > echo "Test complete" > } > diff --git a/tests/loop/001.out b/tests/loop/001.out > index 75979f0..8c4917f 100644 > --- a/tests/loop/001.out > +++ b/tests/loop/001.out > @@ -1,3 +1,4 @@ > Running loop/001 > 3 > +0 > Test complete > -- > 2.7.4 >
On 3/15/19 1:55 AM, Omar Sandoval wrote: > On Thu, Mar 14, 2019 at 07:45:17PM +0800, Dongli Zhang wrote: >> loop/001 does not test whether all partitions are removed successfully >> during loop device partition scanning. As a result, the regression >> introduced by 0da03cab87e6 ("loop: Fix deadlock when calling >> blkdev_reread_part()") can not be detected. >> >> The regression will generate below message in dmesg: >> >> [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) >> >> and leave orphan partitions like below: >> >> - /dev/loop0p1 >> - /sys/block/loop0/loop0p1 >> >> This patch verifies all partitions are removed by checking if there is >> /sys/block/loopX/loopXpY left. The expected number of partitions left is 0. > > Thanks for the test! A couple of comments below. > >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> --- >> tests/loop/001 | 5 +++++ >> tests/loop/001.out | 1 + >> 2 files changed, 6 insertions(+) >> >> diff --git a/tests/loop/001 b/tests/loop/001 >> index 47f760a..a0326b7 100755 >> --- a/tests/loop/001 >> +++ b/tests/loop/001 >> @@ -4,6 +4,9 @@ >> # >> # Test loop device partition scanning. Regression test for commit e02898b42380 >> # ("loop: fix LO_FLAGS_PARTSCAN hang"). >> +# >> +# Test loop device paritition scanning. Regression test for commit 758a58d0bc67 >> +# ("loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > > These can just be combined to > > # Test loop device partition scanning. Regression test for commits e02898b42380 > # ("loop: fix LO_FLAGS_PARTSCAN hang") and 758a58d0bc67 ("loop: set > # GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > >> . tests/loop/rc >> >> @@ -24,9 +27,11 @@ test() { >> mkpart primary 50% 100% >> >> loop_device="$(losetup -P -f --show "$TMPDIR/img")" >> + loop_name=${loop_device:5} >> lsblk -ln "$loop_device" | wc -l >> >> losetup -d "$loop_device" >> + ls /sys/block/$loop_name | grep loop | wc -l > > We can just repeat the same `lsblk -ln "$loop_device" | wc -l` from > earlier, right? That's a bit cleaner than the hardcoded string slicing > and ls. Seems 'lsblk' does not work here. step1: truncate -s 100M /tmp/tmp.raw step2: parted /tmp/tmp.raw --script mklabel msdos \ mkpart primary 0% 50% mkpart primary 50% 100% step3: losetup -P -f --show /tmp/tmp.raw Now we are able to see two loop partitions from 'lsblk' # lsblk -ln /dev/loop0 loop0 7:0 0 100M 0 loop loop0p1 259:0 0 50M 0 loop loop0p2 259:1 0 50M 0 loop step4: # losetup -d /dev/loop0 There is below syslog as partscan is failed. [ 261.181049] __loop_clr_fd: partition scan of loop0 failed (rc=-22) There are 2 partitions left: # ls /dev | grep loop0 loop0 loop0p1 loop0p2 # ls /sys/block/loop0 | grep loop loop0p1 loop0p2 However, 'lsblk -ln' does not report the orphan paritions: # lsblk -ln sr0 11:0 1 1024M 0 rom sda 8:0 0 20G 0 disk sda2 8:2 0 1K 0 part sda5 8:5 0 4.1G 0 part [SWAP] sda1 8:1 0 15.9G 0 part / Therefore, we would not be able to use 'lsblk' here. Dongli Zhang
On Fri, Mar 15, 2019 at 10:00:27AM +0800, Dongli Zhang wrote: > > > On 3/15/19 1:55 AM, Omar Sandoval wrote: > > On Thu, Mar 14, 2019 at 07:45:17PM +0800, Dongli Zhang wrote: > >> loop/001 does not test whether all partitions are removed successfully > >> during loop device partition scanning. As a result, the regression > >> introduced by 0da03cab87e6 ("loop: Fix deadlock when calling > >> blkdev_reread_part()") can not be detected. > >> > >> The regression will generate below message in dmesg: > >> > >> [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > >> > >> and leave orphan partitions like below: > >> > >> - /dev/loop0p1 > >> - /sys/block/loop0/loop0p1 > >> > >> This patch verifies all partitions are removed by checking if there is > >> /sys/block/loopX/loopXpY left. The expected number of partitions left is 0. > > > > Thanks for the test! A couple of comments below. > > > >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > >> --- > >> tests/loop/001 | 5 +++++ > >> tests/loop/001.out | 1 + > >> 2 files changed, 6 insertions(+) > >> > >> diff --git a/tests/loop/001 b/tests/loop/001 > >> index 47f760a..a0326b7 100755 > >> --- a/tests/loop/001 > >> +++ b/tests/loop/001 > >> @@ -4,6 +4,9 @@ > >> # > >> # Test loop device partition scanning. Regression test for commit e02898b42380 > >> # ("loop: fix LO_FLAGS_PARTSCAN hang"). > >> +# > >> +# Test loop device paritition scanning. Regression test for commit 758a58d0bc67 > >> +# ("loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > > > > These can just be combined to > > > > # Test loop device partition scanning. Regression test for commits e02898b42380 > > # ("loop: fix LO_FLAGS_PARTSCAN hang") and 758a58d0bc67 ("loop: set > > # GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > > > >> . tests/loop/rc > >> > >> @@ -24,9 +27,11 @@ test() { > >> mkpart primary 50% 100% > >> > >> loop_device="$(losetup -P -f --show "$TMPDIR/img")" > >> + loop_name=${loop_device:5} > >> lsblk -ln "$loop_device" | wc -l > >> > >> losetup -d "$loop_device" > >> + ls /sys/block/$loop_name | grep loop | wc -l > > > > We can just repeat the same `lsblk -ln "$loop_device" | wc -l` from > > earlier, right? That's a bit cleaner than the hardcoded string slicing > > and ls. > > Seems 'lsblk' does not work here. > > step1: truncate -s 100M /tmp/tmp.raw > step2: parted /tmp/tmp.raw --script mklabel msdos \ > mkpart primary 0% 50% mkpart primary 50% 100% > step3: losetup -P -f --show /tmp/tmp.raw > > Now we are able to see two loop partitions from 'lsblk' > > # lsblk -ln /dev/loop0 > loop0 7:0 0 100M 0 loop > loop0p1 259:0 0 50M 0 loop > loop0p2 259:1 0 50M 0 loop > > > step4: # losetup -d /dev/loop0 > > There is below syslog as partscan is failed. > > [ 261.181049] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > > > There are 2 partitions left: > > # ls /dev | grep loop0 > loop0 > loop0p1 > loop0p2 > > # ls /sys/block/loop0 | grep loop > loop0p1 > loop0p2 > > > However, 'lsblk -ln' does not report the orphan paritions: > > # lsblk -ln > sr0 11:0 1 1024M 0 rom > sda 8:0 0 20G 0 disk > sda2 8:2 0 1K 0 part > sda5 8:5 0 4.1G 0 part [SWAP] > sda1 8:1 0 15.9G 0 part / > > > Therefore, we would not be able to use 'lsblk' here. I see. I think we should check both lsblk and sysfs here. How about something like https://github.com/osandov/blktests/commit/6c1237cd358008024ece90bd915a67c23add8a2a?
On 3/22/19 7:26 AM, Omar Sandoval wrote: > On Fri, Mar 15, 2019 at 10:00:27AM +0800, Dongli Zhang wrote: >> >> >> On 3/15/19 1:55 AM, Omar Sandoval wrote: >>> On Thu, Mar 14, 2019 at 07:45:17PM +0800, Dongli Zhang wrote: >>>> loop/001 does not test whether all partitions are removed successfully >>>> during loop device partition scanning. As a result, the regression >>>> introduced by 0da03cab87e6 ("loop: Fix deadlock when calling >>>> blkdev_reread_part()") can not be detected. >>>> >>>> The regression will generate below message in dmesg: >>>> >>>> [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) >>>> >>>> and leave orphan partitions like below: >>>> >>>> - /dev/loop0p1 >>>> - /sys/block/loop0/loop0p1 >>>> >>>> This patch verifies all partitions are removed by checking if there is >>>> /sys/block/loopX/loopXpY left. The expected number of partitions left is 0. >>> >>> Thanks for the test! A couple of comments below. >>> >>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >>>> --- >>>> tests/loop/001 | 5 +++++ >>>> tests/loop/001.out | 1 + >>>> 2 files changed, 6 insertions(+) >>>> >>>> diff --git a/tests/loop/001 b/tests/loop/001 >>>> index 47f760a..a0326b7 100755 >>>> --- a/tests/loop/001 >>>> +++ b/tests/loop/001 >>>> @@ -4,6 +4,9 @@ >>>> # >>>> # Test loop device partition scanning. Regression test for commit e02898b42380 >>>> # ("loop: fix LO_FLAGS_PARTSCAN hang"). >>>> +# >>>> +# Test loop device paritition scanning. Regression test for commit 758a58d0bc67 >>>> +# ("loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). >>> >>> These can just be combined to >>> >>> # Test loop device partition scanning. Regression test for commits e02898b42380 >>> # ("loop: fix LO_FLAGS_PARTSCAN hang") and 758a58d0bc67 ("loop: set >>> # GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). >>> >>>> . tests/loop/rc >>>> >>>> @@ -24,9 +27,11 @@ test() { >>>> mkpart primary 50% 100% >>>> >>>> loop_device="$(losetup -P -f --show "$TMPDIR/img")" >>>> + loop_name=${loop_device:5} >>>> lsblk -ln "$loop_device" | wc -l >>>> >>>> losetup -d "$loop_device" >>>> + ls /sys/block/$loop_name | grep loop | wc -l >>> >>> We can just repeat the same `lsblk -ln "$loop_device" | wc -l` from >>> earlier, right? That's a bit cleaner than the hardcoded string slicing >>> and ls. >> >> Seems 'lsblk' does not work here. >> >> step1: truncate -s 100M /tmp/tmp.raw >> step2: parted /tmp/tmp.raw --script mklabel msdos \ >> mkpart primary 0% 50% mkpart primary 50% 100% >> step3: losetup -P -f --show /tmp/tmp.raw >> >> Now we are able to see two loop partitions from 'lsblk' >> >> # lsblk -ln /dev/loop0 >> loop0 7:0 0 100M 0 loop >> loop0p1 259:0 0 50M 0 loop >> loop0p2 259:1 0 50M 0 loop >> >> >> step4: # losetup -d /dev/loop0 >> >> There is below syslog as partscan is failed. >> >> [ 261.181049] __loop_clr_fd: partition scan of loop0 failed (rc=-22) >> >> >> There are 2 partitions left: >> >> # ls /dev | grep loop0 >> loop0 >> loop0p1 >> loop0p2 >> >> # ls /sys/block/loop0 | grep loop >> loop0p1 >> loop0p2 >> >> >> However, 'lsblk -ln' does not report the orphan paritions: >> >> # lsblk -ln >> sr0 11:0 1 1024M 0 rom >> sda 8:0 0 20G 0 disk >> sda2 8:2 0 1K 0 part >> sda5 8:5 0 4.1G 0 part [SWAP] >> sda1 8:1 0 15.9G 0 part / >> >> >> Therefore, we would not be able to use 'lsblk' here. > > I see. I think we should check both lsblk and sysfs here. How about > something like > https://github.com/osandov/blktests/commit/6c1237cd358008024ece90bd915a67c23add8a2a? > It is good to me. Thank you very much for improve the patchset! I have tested it would pass with commit 758a58d0bc67 and not pass w/o 758a58d0bc67. To check both lsblk and sysfs may test the loop much more thoroughly. Just one programming question about loop_partition_sysfs(): +find_loop_partition_sysfs() { + find "$sysfs/" -mindepth 1 -maxdepth 1 -name "$1"'p*' -printf '%f\n' | + sed -n "s/^${1}p//p" | sort +} Why not something like: ls "$sysfs/" | grep $1 | sed -n "s/^${1}p//p" | sort Is that because it is better to use 'find' than 'ls' in bash programming? Thank you very much! Dongli Zhang
On Fri, Mar 22, 2019 at 10:23:01AM +0800, Dongli Zhang wrote: > > > On 3/22/19 7:26 AM, Omar Sandoval wrote: > > On Fri, Mar 15, 2019 at 10:00:27AM +0800, Dongli Zhang wrote: > >> > >> > >> On 3/15/19 1:55 AM, Omar Sandoval wrote: > >>> On Thu, Mar 14, 2019 at 07:45:17PM +0800, Dongli Zhang wrote: > >>>> loop/001 does not test whether all partitions are removed successfully > >>>> during loop device partition scanning. As a result, the regression > >>>> introduced by 0da03cab87e6 ("loop: Fix deadlock when calling > >>>> blkdev_reread_part()") can not be detected. > >>>> > >>>> The regression will generate below message in dmesg: > >>>> > >>>> [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > >>>> > >>>> and leave orphan partitions like below: > >>>> > >>>> - /dev/loop0p1 > >>>> - /sys/block/loop0/loop0p1 > >>>> > >>>> This patch verifies all partitions are removed by checking if there is > >>>> /sys/block/loopX/loopXpY left. The expected number of partitions left is 0. > >>> > >>> Thanks for the test! A couple of comments below. > >>> > >>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > >>>> --- > >>>> tests/loop/001 | 5 +++++ > >>>> tests/loop/001.out | 1 + > >>>> 2 files changed, 6 insertions(+) > >>>> > >>>> diff --git a/tests/loop/001 b/tests/loop/001 > >>>> index 47f760a..a0326b7 100755 > >>>> --- a/tests/loop/001 > >>>> +++ b/tests/loop/001 > >>>> @@ -4,6 +4,9 @@ > >>>> # > >>>> # Test loop device partition scanning. Regression test for commit e02898b42380 > >>>> # ("loop: fix LO_FLAGS_PARTSCAN hang"). > >>>> +# > >>>> +# Test loop device paritition scanning. Regression test for commit 758a58d0bc67 > >>>> +# ("loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > >>> > >>> These can just be combined to > >>> > >>> # Test loop device partition scanning. Regression test for commits e02898b42380 > >>> # ("loop: fix LO_FLAGS_PARTSCAN hang") and 758a58d0bc67 ("loop: set > >>> # GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). > >>> > >>>> . tests/loop/rc > >>>> > >>>> @@ -24,9 +27,11 @@ test() { > >>>> mkpart primary 50% 100% > >>>> > >>>> loop_device="$(losetup -P -f --show "$TMPDIR/img")" > >>>> + loop_name=${loop_device:5} > >>>> lsblk -ln "$loop_device" | wc -l > >>>> > >>>> losetup -d "$loop_device" > >>>> + ls /sys/block/$loop_name | grep loop | wc -l > >>> > >>> We can just repeat the same `lsblk -ln "$loop_device" | wc -l` from > >>> earlier, right? That's a bit cleaner than the hardcoded string slicing > >>> and ls. > >> > >> Seems 'lsblk' does not work here. > >> > >> step1: truncate -s 100M /tmp/tmp.raw > >> step2: parted /tmp/tmp.raw --script mklabel msdos \ > >> mkpart primary 0% 50% mkpart primary 50% 100% > >> step3: losetup -P -f --show /tmp/tmp.raw > >> > >> Now we are able to see two loop partitions from 'lsblk' > >> > >> # lsblk -ln /dev/loop0 > >> loop0 7:0 0 100M 0 loop > >> loop0p1 259:0 0 50M 0 loop > >> loop0p2 259:1 0 50M 0 loop > >> > >> > >> step4: # losetup -d /dev/loop0 > >> > >> There is below syslog as partscan is failed. > >> > >> [ 261.181049] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > >> > >> > >> There are 2 partitions left: > >> > >> # ls /dev | grep loop0 > >> loop0 > >> loop0p1 > >> loop0p2 > >> > >> # ls /sys/block/loop0 | grep loop > >> loop0p1 > >> loop0p2 > >> > >> > >> However, 'lsblk -ln' does not report the orphan paritions: > >> > >> # lsblk -ln > >> sr0 11:0 1 1024M 0 rom > >> sda 8:0 0 20G 0 disk > >> sda2 8:2 0 1K 0 part > >> sda5 8:5 0 4.1G 0 part [SWAP] > >> sda1 8:1 0 15.9G 0 part / > >> > >> > >> Therefore, we would not be able to use 'lsblk' here. > > > > I see. I think we should check both lsblk and sysfs here. How about > > something like > > https://github.com/osandov/blktests/commit/6c1237cd358008024ece90bd915a67c23add8a2a? > > > > It is good to me. Thank you very much for improve the patchset! I have tested it > would pass with commit 758a58d0bc67 and not pass w/o 758a58d0bc67. > > To check both lsblk and sysfs may test the loop much more thoroughly. Thanks, applied. > Just one programming question about loop_partition_sysfs(): > > +find_loop_partition_sysfs() { > + find "$sysfs/" -mindepth 1 -maxdepth 1 -name "$1"'p*' -printf '%f\n' | > + sed -n "s/^${1}p//p" | sort > +} > > Why not something like: > > ls "$sysfs/" | grep $1 | sed -n "s/^${1}p//p" | sort > > Is that because it is better to use 'find' than 'ls' in bash programming? I usually prefer find because you can control the output more exactly. ls probably would've been fine in this case.
diff --git a/tests/loop/001 b/tests/loop/001 index 47f760a..a0326b7 100755 --- a/tests/loop/001 +++ b/tests/loop/001 @@ -4,6 +4,9 @@ # # Test loop device partition scanning. Regression test for commit e02898b42380 # ("loop: fix LO_FLAGS_PARTSCAN hang"). +# +# Test loop device paritition scanning. Regression test for commit 758a58d0bc67 +# ("loop: set GENHD_FL_NO_PART_SCAN after blkdev_reread_part()"). . tests/loop/rc @@ -24,9 +27,11 @@ test() { mkpart primary 50% 100% loop_device="$(losetup -P -f --show "$TMPDIR/img")" + loop_name=${loop_device:5} lsblk -ln "$loop_device" | wc -l losetup -d "$loop_device" + ls /sys/block/$loop_name | grep loop | wc -l rm "$TMPDIR/img" echo "Test complete" } diff --git a/tests/loop/001.out b/tests/loop/001.out index 75979f0..8c4917f 100644 --- a/tests/loop/001.out +++ b/tests/loop/001.out @@ -1,3 +1,4 @@ Running loop/001 3 +0 Test complete
loop/001 does not test whether all partitions are removed successfully during loop device partition scanning. As a result, the regression introduced by 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") can not be detected. The regression will generate below message in dmesg: [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) and leave orphan partitions like below: - /dev/loop0p1 - /sys/block/loop0/loop0p1 This patch verifies all partitions are removed by checking if there is /sys/block/loopX/loopXpY left. The expected number of partitions left is 0. Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- tests/loop/001 | 5 +++++ tests/loop/001.out | 1 + 2 files changed, 6 insertions(+)