| Message ID | 20240529171516.54111-1-gulam.mohamed@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,blktests] loop: Detect a race condition between loop detach and open | expand |
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@oracle.com> thanks for the test, there seems to be an issue with the formatting of the patch ? > --- > 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 .. > +} > + > +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 ? > + 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 ... > + losetup -d "${loop_device}" > /dev/null 2>&1 > + same here ... > + 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 ... > + 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 .. > + 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@nvidia.com> -ck
Hi Chaitanya, Thanks for the review, > -----Original Message----- > From: Chaitanya Kulkarni <chaitanyak@nvidia.com> > Sent: Thursday, May 30, 2024 12:44 AM > To: Gulam Mohamed <gulam.mohamed@oracle.com> > Cc: shinichiro.kawasaki@wdc.com; linux-block@vger.kernel.org > 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@oracle.com> > > 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@nvidia.com> > > -ck > Addressed all the comments above. Can you please let me know the next step? Regards, Gulam Mohamed.
>> apart from formatting and nit comments it looks good ... Reviewed-by: >> Chaitanya Kulkarni <kch@nvidia.com> -ck > Addressed all the comments above. Can you please let me know the next > step? where ? I don't see the v3 on the list did you send it already ? -ck
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 +} + +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 + 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 + + losetup -d "${loop_device}" > /dev/null 2>&1 + + 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 + if _dmesg_since_test_start | grep -q "partition scan of loop0 failed (rc=-16)"; then + 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
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@oracle.com> --- 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