Hi Chaitanya,
Thanks for the review,
> -----Original Message-----
> From: Chaitanya Kulkarni <chaitanyak@xxxxxxxxxx>
> Sent: Thursday, May 30, 2024 12:44 AM
> To: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>
> Cc: shinichiro.kawasaki@xxxxxxx; linux-block@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2 blktests] loop: Detect a race condition between loop
> detach and open
>
> On 5/29/24 10:15, Gulam Mohamed wrote:
> > When one process opens a loop device partition and another process
> > detaches it, there will be a race condition due to which stale loop
> > partitions are created causing IO errors. This test will detect the
> > race
> >
> > Signed-off-by: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>
>
> thanks for the test, there seems to be an issue with the formatting of the
> patch ?
There were indentation/space errors. Corrected and ran "make check" and found no errors/warnings in this script
>
> > ---
> > tests/loop/010 | 75
> ++++++++++++++++++++++++++++++++++++++++++++++
> > tests/loop/010.out | 2 ++
> > 2 files changed, 77 insertions(+)
> > create mode 100755 tests/loop/010
> > create mode 100644 tests/loop/010.out
> >
> > diff --git a/tests/loop/010 b/tests/loop/010 new file mode 100755
> > index 000000000000..aa9c1d33bdb9
> > --- /dev/null
> > +++ b/tests/loop/010
> > @@ -0,0 +1,75 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-3.0+
> > +# Copyright (C) 2024, Oracle and/or its affiliates.
> > +#
> > +# Test to detect a race between loop detach and loop open which
> > +creates # stale loop partitions when one process opens the loop
> > +partition and # another process detaches the loop device # .
> > +tests/loop/rc DESCRIPTION="check stale loop partition"
> > +TIMED=1
> > +
> > +requires() {
> > + _have_program parted
> > + _have_program mkfs.xfs
>
> nit:-
> do we need to add blkid/udevadm ? if not ignore this comment ..
Yes, blkid and udevadm commands are needed. Included them
>
> > +}
> > +
> > +image_file="$TMPDIR/loopImg"
> > +
> > +create_loop()
> > +{
> > + while true
> > + do
> > + loop_device="$(losetup -P -f --show "${image_file}")"
> > + blkid /dev/loop0p1 >> /dev/null 2>&1
> > + done
> > +}
> > +
> > +detach_loop()
> > +{
> > + while true
> > + do
> > + if [ -e /dev/loop0 ]; then
> > + losetup -d /dev/loop0 > /dev/null 2>&1
>
> nit:-
> do we want to record this somewhere for debugging purpose instead of
> /dev/null ?
This is not needed
>
> > + fi
> > + done
> > +}
> > +
> > +test() {
> > + echo "Running ${TEST_NAME}"
> > + local loop_device
> > +
> > + truncate -s 1G "${image_file}"
> > + parted -a none -s "${image_file}" mklabel gpt
> > + loop_device="$(losetup -P -f --show "${image_file}")"
> > + parted -a none -s "${loop_device}" mkpart primary 64s 109051s
> > +
> > + udevadm settle
> > + if [ ! -e "${loop_device}" ]; then
> > + return 1
> > + fi
> > +
> > + mkfs.xfs -f "${loop_device}p1" > /dev/null 2>&1
> > +
>
> same here ...
Not needed as we have sufficient space for xfs and we are using force option also
>
> > + losetup -d "${loop_device}" > /dev/null 2>&1
> > +
>
> same here ...
Not needed
>
> > + create_loop &
> > + create_pid=$!
> > + detach_loop &
> > + detach_pid=$!
> > +
> > + sleep "${TIMEOUT:-90}"
> > + {
> > + kill -9 $create_pid
> > + kill -9 $detach_pid
> > + wait
> > + sleep 1
> > + } 2>/dev/null
> > +
> > + losetup -D > /dev/null 2>&1
>
> same here ...
Not needed
>
> > + if _dmesg_since_test_start | grep -q "partition scan of loop0 failed
> > +(rc=-16)"; then
>
> do we want to keep the error message short and achieve same result ?
> just a thought feel free to ignore this comment ..
Kept this message so that its enough to distinguish with other messages
>
> > + echo "Fail"
> > + fi
> > + echo "Test complete"
> > +}
> > diff --git a/tests/loop/010.out b/tests/loop/010.out new file mode
> > 100644 index 000000000000..64a6aee00b8a
> > --- /dev/null
> > +++ b/tests/loop/010.out
> > @@ -0,0 +1,2 @@
> > +Running loop/010
> > +Test complete
>
> apart from formatting and nit comments it looks good ...
>
> Reviewed-by: Chaitanya Kulkarni <kch@xxxxxxxxxx>
>
> -ck
>
Addressed all the comments above. Can you please let me know the next step?
Regards,
Gulam Mohamed.
