On Jun 25, 2024 / 14:10, Gulam Mohamed wrote:
> Hi Shinichiro,
>
> > -----Original Message-----
> > From: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > Sent: Tuesday, June 25, 2024 4:50 PM
> > To: linux-block@xxxxxxxxxxxxxxx
> > Cc: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>; Chaitanya Kulkarni
> > <kch@xxxxxxxxxx>; Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > 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@xxxxxxx>
> > ---
> > 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
>
>
