| Message ID | 20240625112011.409282-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests,v2] loop/010: do not assume /dev/loop0 | expand |
Hi Shinichiro, > -----Original Message----- > From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Sent: Tuesday, June 25, 2024 4:50 PM > To: linux-block@vger.kernel.org > Cc: Gulam Mohamed <gulam.mohamed@oracle.com>; Chaitanya Kulkarni > <kch@nvidia.com>; Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Subject: [PATCH blktests v2] loop/010: do not assume /dev/loop0 > > The current implementation of the test case loop/010 assumes that the > prepared loop device is /dev/loop0, which is not always true. When other > loop devices are set up before the test case run, the assumption is > wrong and the test case fails. > > To avoid the failure, use the prepared loop device name stored in > $loop_device instead of /dev/loop0. Adjust the grep string to meet the > device name. Also use "losetup --detach" instead of > "losetup --detach-all" to not detach the loop devices which existed > before the test case runs. > > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and > open") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > Changes from v1: > * Replaced the losetup --find option with the loop device name > * Added the missing "p1" postfix to the blkid argument > > tests/loop/010 | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/tests/loop/010 b/tests/loop/010 > index ea396ec..ade8044 100755 > --- a/tests/loop/010 > +++ b/tests/loop/010 > @@ -16,18 +16,23 @@ requires() { > } > > create_loop() { > + local dev=$1 > + > while true > do > - loop_device="$(losetup --partscan --find --show > "${image_file}")" > - blkid /dev/loop0p1 >& /dev/null > + if losetup --partscan "$dev" "${image_file}" &> /dev/null; > then > + blkid "$dev"p1 >& /dev/null > + fi > done > } > > detach_loop() { > + local dev=$1 > + > while true > do > - if [ -e /dev/loop0 ]; then > - losetup --detach /dev/loop0 >& /dev/null > + if [[ -e "$dev" ]]; then > + losetup --detach "$dev" >& /dev/null > fi > done > } > @@ -38,6 +43,7 @@ test() { > local create_pid > local detach_pid > local image_file="$TMPDIR/loopImg" > + local grep_str > > truncate --size 1G "${image_file}" > parted --align none --script "${image_file}" mklabel gpt > @@ -53,9 +59,9 @@ test() { > mkfs.xfs --force "${loop_device}p1" >& /dev/null > losetup --detach "${loop_device}" >& /dev/null > > - create_loop & > + create_loop "${loop_device}" & > create_pid=$! > - detach_loop & > + detach_loop "${loop_device}" & > detach_pid=$! > > sleep "${TIMEOUT:-90}" > @@ -66,8 +72,9 @@ test() { > sleep 1 > } 2>/dev/null > > - losetup --detach-all >& /dev/null > - if _dmesg_since_test_start | grep --quiet "partition scan of loop0 > failed (rc=-16)"; then > + losetup --detach "${loop_device}" >& /dev/null > + grep_str="partition scan of ${loop_device##*/} failed (rc=-16)" Can you please add this also "__loop_clr_fd: " to the grep_str (please note the "space" after ":")? So that the grep string looks like this: "__loop_clr_fd: partition scan of loop0 failed (rc=-16)" Other than this, it looks good to me. Regards, Gulam Mohamed. > + if _dmesg_since_test_start | grep --quiet "$grep_str"; then > echo "Fail" > fi > echo "Test complete" > -- > 2.45.0
On Jun 25, 2024 / 14:10, Gulam Mohamed wrote: > Hi Shinichiro, > > > -----Original Message----- > > From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Sent: Tuesday, June 25, 2024 4:50 PM > > To: linux-block@vger.kernel.org > > Cc: Gulam Mohamed <gulam.mohamed@oracle.com>; Chaitanya Kulkarni > > <kch@nvidia.com>; Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Subject: [PATCH blktests v2] loop/010: do not assume /dev/loop0 > > > > The current implementation of the test case loop/010 assumes that the > > prepared loop device is /dev/loop0, which is not always true. When other > > loop devices are set up before the test case run, the assumption is > > wrong and the test case fails. > > > > To avoid the failure, use the prepared loop device name stored in > > $loop_device instead of /dev/loop0. Adjust the grep string to meet the > > device name. Also use "losetup --detach" instead of > > "losetup --detach-all" to not detach the loop devices which existed > > before the test case runs. > > > > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and > > open") > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > --- > > Changes from v1: > > * Replaced the losetup --find option with the loop device name > > * Added the missing "p1" postfix to the blkid argument > > > > tests/loop/010 | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/tests/loop/010 b/tests/loop/010 > > index ea396ec..ade8044 100755 > > --- a/tests/loop/010 > > +++ b/tests/loop/010 > > @@ -16,18 +16,23 @@ requires() { > > } > > > > create_loop() { > > + local dev=$1 > > + > > while true > > do > > - loop_device="$(losetup --partscan --find --show > > "${image_file}")" > > - blkid /dev/loop0p1 >& /dev/null > > + if losetup --partscan "$dev" "${image_file}" &> /dev/null; > > then > > + blkid "$dev"p1 >& /dev/null > > + fi > > done > > } > > > > detach_loop() { > > + local dev=$1 > > + > > while true > > do > > - if [ -e /dev/loop0 ]; then > > - losetup --detach /dev/loop0 >& /dev/null > > + if [[ -e "$dev" ]]; then > > + losetup --detach "$dev" >& /dev/null > > fi > > done > > } > > @@ -38,6 +43,7 @@ test() { > > local create_pid > > local detach_pid > > local image_file="$TMPDIR/loopImg" > > + local grep_str > > > > truncate --size 1G "${image_file}" > > parted --align none --script "${image_file}" mklabel gpt > > @@ -53,9 +59,9 @@ test() { > > mkfs.xfs --force "${loop_device}p1" >& /dev/null > > losetup --detach "${loop_device}" >& /dev/null > > > > - create_loop & > > + create_loop "${loop_device}" & > > create_pid=$! > > - detach_loop & > > + detach_loop "${loop_device}" & > > detach_pid=$! > > > > sleep "${TIMEOUT:-90}" > > @@ -66,8 +72,9 @@ test() { > > sleep 1 > > } 2>/dev/null > > > > - losetup --detach-all >& /dev/null > > - if _dmesg_since_test_start | grep --quiet "partition scan of loop0 > > failed (rc=-16)"; then > > + losetup --detach "${loop_device}" >& /dev/null > > + grep_str="partition scan of ${loop_device##*/} failed (rc=-16)" > > Can you please add this also "__loop_clr_fd: " to the grep_str (please note the "space" after ":")? So that the grep string looks like this: > > "__loop_clr_fd: partition scan of loop0 failed (rc=-16)" I'm not sure if this change is a good one. The added string "__loop_clr_fd: " will distinguish which function reported the error message: _loop_clr_fd() or loop_reread_partitions(). This will make the test more accurate, probably. Having said that, the change will make the test fragile against the future function name changes. Once the function name changes in the kernel side, blktests side change will be required. I would like to reduce the possibility of such work. > > Other than this, it looks good to me. > > Regards, > Gulam Mohamed. > > > + if _dmesg_since_test_start | grep --quiet "$grep_str"; then > > echo "Fail" > > fi > > echo "Test complete" > > -- > > 2.45.0 > >
Hi Shinichiro, > -----Original Message----- > From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Sent: Wednesday, June 26, 2024 7:58 AM > To: Gulam Mohamed <gulam.mohamed@oracle.com> > Cc: linux-block@vger.kernel.org; Chaitanya Kulkarni <kch@nvidia.com> > Subject: Re: [PATCH blktests v2] loop/010: do not assume /dev/loop0 > > On Jun 25, 2024 / 14:10, Gulam Mohamed wrote: > > Hi Shinichiro, > > > > > -----Original Message----- > > > From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > Sent: Tuesday, June 25, 2024 4:50 PM > > > To: linux-block@vger.kernel.org > > > Cc: Gulam Mohamed <gulam.mohamed@oracle.com>; Chaitanya > Kulkarni > > > <kch@nvidia.com>; Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > Subject: [PATCH blktests v2] loop/010: do not assume /dev/loop0 > > > > > > The current implementation of the test case loop/010 assumes that > > > the prepared loop device is /dev/loop0, which is not always true. > > > When other loop devices are set up before the test case run, the > > > assumption is wrong and the test case fails. > > > > > > To avoid the failure, use the prepared loop device name stored in > > > $loop_device instead of /dev/loop0. Adjust the grep string to meet > > > the device name. Also use "losetup --detach" instead of "losetup > > > --detach-all" to not detach the loop devices which existed before > > > the test case runs. > > > > > > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop > > > detach and > > > open") > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > --- > > > Changes from v1: > > > * Replaced the losetup --find option with the loop device name > > > * Added the missing "p1" postfix to the blkid argument > > > > > > tests/loop/010 | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/tests/loop/010 b/tests/loop/010 index ea396ec..ade8044 > > > 100755 > > > --- a/tests/loop/010 > > > +++ b/tests/loop/010 > > > @@ -16,18 +16,23 @@ requires() { > > > } > > > > > > create_loop() { > > > + local dev=$1 > > > + > > > while true > > > do > > > - loop_device="$(losetup --partscan --find --show > > > "${image_file}")" > > > - blkid /dev/loop0p1 >& /dev/null > > > + if losetup --partscan "$dev" "${image_file}" &> /dev/null; > > > then > > > + blkid "$dev"p1 >& /dev/null > > > + fi > > > done > > > } > > > > > > detach_loop() { > > > + local dev=$1 > > > + > > > while true > > > do > > > - if [ -e /dev/loop0 ]; then > > > - losetup --detach /dev/loop0 >& /dev/null > > > + if [[ -e "$dev" ]]; then > > > + losetup --detach "$dev" >& /dev/null > > > fi > > > done > > > } > > > @@ -38,6 +43,7 @@ test() { > > > local create_pid > > > local detach_pid > > > local image_file="$TMPDIR/loopImg" > > > + local grep_str > > > > > > truncate --size 1G "${image_file}" > > > parted --align none --script "${image_file}" mklabel gpt @@ -53,9 > > > +59,9 @@ test() { > > > mkfs.xfs --force "${loop_device}p1" >& /dev/null > > > losetup --detach "${loop_device}" >& /dev/null > > > > > > - create_loop & > > > + create_loop "${loop_device}" & > > > create_pid=$! > > > - detach_loop & > > > + detach_loop "${loop_device}" & > > > detach_pid=$! > > > > > > sleep "${TIMEOUT:-90}" > > > @@ -66,8 +72,9 @@ test() { > > > sleep 1 > > > } 2>/dev/null > > > > > > - losetup --detach-all >& /dev/null > > > - if _dmesg_since_test_start | grep --quiet "partition scan of loop0 > > > failed (rc=-16)"; then > > > + losetup --detach "${loop_device}" >& /dev/null > > > + grep_str="partition scan of ${loop_device##*/} failed (rc=-16)" > > > > Can you please add this also "__loop_clr_fd: " to the grep_str (please note > the "space" after ":")? So that the grep string looks like this: > > > > "__loop_clr_fd: partition scan of loop0 failed (rc=-16)" > > I'm not sure if this change is a good one. The added string "__loop_clr_fd: " > will distinguish which function reported the error message: _loop_clr_fd() or > loop_reread_partitions(). This will make the test more accurate, probably. > Having said that, the change will make the test fragile against the future > function name changes. Once the function name changes in the kernel side, > blktests side change will be required. I would like to reduce the possibility of > such work. I agree. Reviewed-by: Gulam Mohamed <gulam.mohamed@oracle.com> Regards, Gulam Mohamed. > > > > > Other than this, it looks good to me. > > > > Regards, > > Gulam Mohamed. > > > > > + if _dmesg_since_test_start | grep --quiet "$grep_str"; then > > > echo "Fail" > > > fi > > > echo "Test complete" > > > -- > > > 2.45.0 > > > >
On 6/25/24 04:20, Shin'ichiro Kawasaki wrote: > The current implementation of the test case loop/010 assumes that the > prepared loop device is /dev/loop0, which is not always true. When other > loop devices are set up before the test case run, the assumption is > wrong and the test case fails. > > To avoid the failure, use the prepared loop device name stored in > $loop_device instead of /dev/loop0. Adjust the grep string to meet the > device name. Also use "losetup --detach" instead of > "losetup --detach-all" to not detach the loop devices which existed > before the test case runs. > > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and open") > Signed-off-by: Shin'ichiro Kawasaki<shinichiro.kawasaki@wdc.com> > --- I've sent a similar test with hard-coded DUT, always ends up in trouble. I'll keep in mind from next time when reviewing the tests. Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
On Jun 25, 2024 / 20:20, Shin'ichiro Kawasaki wrote: > The current implementation of the test case loop/010 assumes that the > prepared loop device is /dev/loop0, which is not always true. When other > loop devices are set up before the test case run, the assumption is > wrong and the test case fails. > > To avoid the failure, use the prepared loop device name stored in > $loop_device instead of /dev/loop0. Adjust the grep string to meet the > device name. Also use "losetup --detach" instead of > "losetup --detach-all" to not detach the loop devices which existed > before the test case runs. > > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and open") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> FYI, this fix has got applied.
Hi Shinichiro, Thank you very much for working on improving the test case. Regards, Gulam Mohamed. > -----Original Message----- > From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Sent: Friday, June 28, 2024 3:41 PM > To: linux-block@vger.kernel.org > Cc: Gulam Mohamed <gulam.mohamed@oracle.com>; Chaitanya Kulkarni > <kch@nvidia.com> > Subject: Re: [PATCH blktests v2] loop/010: do not assume /dev/loop0 > > On Jun 25, 2024 / 20:20, Shin'ichiro Kawasaki wrote: > > The current implementation of the test case loop/010 assumes that the > > prepared loop device is /dev/loop0, which is not always true. When > > other loop devices are set up before the test case run, the assumption > > is wrong and the test case fails. > > > > To avoid the failure, use the prepared loop device name stored in > > $loop_device instead of /dev/loop0. Adjust the grep string to meet the > > device name. Also use "losetup --detach" instead of "losetup > > --detach-all" to not detach the loop devices which existed before the > > test case runs. > > > > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop > > detach and open") > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > FYI, this fix has got applied.
diff --git a/tests/loop/010 b/tests/loop/010 index ea396ec..ade8044 100755 --- a/tests/loop/010 +++ b/tests/loop/010 @@ -16,18 +16,23 @@ requires() { } create_loop() { + local dev=$1 + while true do - loop_device="$(losetup --partscan --find --show "${image_file}")" - blkid /dev/loop0p1 >& /dev/null + if losetup --partscan "$dev" "${image_file}" &> /dev/null; then + blkid "$dev"p1 >& /dev/null + fi done } detach_loop() { + local dev=$1 + while true do - if [ -e /dev/loop0 ]; then - losetup --detach /dev/loop0 >& /dev/null + if [[ -e "$dev" ]]; then + losetup --detach "$dev" >& /dev/null fi done } @@ -38,6 +43,7 @@ test() { local create_pid local detach_pid local image_file="$TMPDIR/loopImg" + local grep_str truncate --size 1G "${image_file}" parted --align none --script "${image_file}" mklabel gpt @@ -53,9 +59,9 @@ test() { mkfs.xfs --force "${loop_device}p1" >& /dev/null losetup --detach "${loop_device}" >& /dev/null - create_loop & + create_loop "${loop_device}" & create_pid=$! - detach_loop & + detach_loop "${loop_device}" & detach_pid=$! sleep "${TIMEOUT:-90}" @@ -66,8 +72,9 @@ test() { sleep 1 } 2>/dev/null - losetup --detach-all >& /dev/null - if _dmesg_since_test_start | grep --quiet "partition scan of loop0 failed (rc=-16)"; then + losetup --detach "${loop_device}" >& /dev/null + grep_str="partition scan of ${loop_device##*/} failed (rc=-16)" + if _dmesg_since_test_start | grep --quiet "$grep_str"; then echo "Fail" fi echo "Test complete"
The current implementation of the test case loop/010 assumes that the prepared loop device is /dev/loop0, which is not always true. When other loop devices are set up before the test case run, the assumption is wrong and the test case fails. To avoid the failure, use the prepared loop device name stored in $loop_device instead of /dev/loop0. Adjust the grep string to meet the device name. Also use "losetup --detach" instead of "losetup --detach-all" to not detach the loop devices which existed before the test case runs. Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and open") Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- Changes from v1: * Replaced the losetup --find option with the loop device name * Added the missing "p1" postfix to the blkid argument tests/loop/010 | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)