Hi Shinichiro,
> -----Original Message-----
> From: Shinichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> Sent: Wednesday, June 26, 2024 7:58 AM
> To: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>
> Cc: linux-block@xxxxxxxxxxxxxxx; Chaitanya Kulkarni <kch@xxxxxxxxxx>
> 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@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.
I agree.
Reviewed-by: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>
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
> >
> >
