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@xxxxxxxxxx>
>>>> ---
>>>> 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
