On Mon, May 30, 2022 at 09:20:27AM +0800, Qu Wenruo wrote:
>
>
> On 2022/5/30 08:23, Anand Jain wrote:
> > On 5/27/22 13:49, Christoph Hellwig wrote:
> > > Add a few helpers to consolidate code for btrfs read repair testing:
> > >
> > > - _btrfs_get_first_logical() gets the btrfs logical address for the
> > > first extent in a file
> > > - _btrfs_get_device_path and _btrfs_get_physical use the
> > > btrfs-map-logical tool to find the device path and physical address
> > > for btrfs logical address for a specific mirror
> > > - _btrfs_direct_read_on_mirror and _btrfs_buffered_read_on_mirror
> > > read the data from a specific mirror
> > >
> > > These will be used to consolidate the read repair tests and avoid
> > > duplication for new tests.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > > Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> > > ---
> > > common/btrfs | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > common/config | 1 +
> > > 2 files changed, 76 insertions(+)
> > >
> > > diff --git a/common/btrfs b/common/btrfs
> > > index ac597ca4..b69feeee 100644
> > > --- a/common/btrfs
> > > +++ b/common/btrfs
> > > @@ -505,3 +505,78 @@ _btrfs_metadump()
> > > $BTRFS_IMAGE_PROG "$device" "$dumpfile"
> > > [ -n "$DUMP_COMPRESSOR" ] && $DUMP_COMPRESSOR -f "$dumpfile" &>
> > > /dev/null
> > > }
> > > +
> > > +# Return the btrfs logical address for the first block in a file
> > > +_btrfs_get_first_logical()
> > > +{
> > > + local file=$1
> > > + _require_command "$FILEFRAG_PROG" filefrag
> > > +
> > > + ${FILEFRAG_PROG} -v $SCRATCH_MNT/foobar >> $seqres.full
> > > + ${FILEFRAG_PROG} -v $file | _filter_filefrag | cut -d '#' -f 1
> > > +}
> > > +
> > > +# Find the device path for a btrfs logical offset
> > > +_btrfs_get_device_path()
> > > +{
> > > + local logical=$1
> > > + local stripe=$2
> > > +
> > > + _require_command "$BTRFS_MAP_LOGICAL_PROG" btrfs-map-logical
> > > +
> > > + $BTRFS_MAP_LOGICAL_PROG -l $logical $SCRATCH_DEV | \
> > > + $AWK_PROG "(\$1 ~ /mirror/ && \$2 ~ /$stripe/) { print \$8 }"
> > > +}
> > > +
> > > +
> > > +# Find the device physical sector for a btrfs logical offset
> > > +_btrfs_get_physical()
> > > +{
> > > + local logical=$1
> > > + local stripe=$2
> > > +
> > > + _require_command "$BTRFS_MAP_LOGICAL_PROG" btrfs-map-logical
> > > +
> > > + $BTRFS_MAP_LOGICAL_PROG -b -l $logical $SCRATCH_DEV >>
> > > $seqres.full 2>&1
> > > + $BTRFS_MAP_LOGICAL_PROG -l $logical $SCRATCH_DEV | \
> > > + $AWK_PROG "(\$1 ~ /mirror/ && \$2 ~ /$stripe/) { print \$6 }"
> > > +}
> > > +
> > > +# Read from a specific stripe to test read recovery that corrupted a
> > > specific
> > > +# stripe. Btrfs uses the PID to select the mirror, so keep reading
> > > until the
> > > +# xfs_io process that performed the read was executed with a PID that
> > > ends up
> > > +# on the intended mirror.
> > > +_btrfs_direct_read_on_mirror()
> > > +{
> > > + local mirror=$1
> > > + local nr_mirrors=$2
> > > + local file=$3
> > > + local offset=$4
> > > + local size=$5
> > > +
> > > + while [[ -z $( (( BASHPID % nr_mirrors == mirror )) &&
> > > + exec $XFS_IO_PROG -d \
> > > + -c "pread -b $size $offset $size" $file) ]]; do
> > > + :
> > > + done
> > > +}
> > > +
> > > +# Read from a specific stripe to test read recovery that corrupted a
> > > specific
> > > +# stripe. Btrfs uses the PID to select the mirror, so keep reading
> > > until the
> > > +# xfs_io process that performed the read was executed with a PID that
> > > ends up
> > > +# on the intended mirror.
> > > +_btrfs_buffered_read_on_mirror()
> > > +{
> > > + local mirror=$1
> > > + local nr_mirrors=$2
> > > + local file=$3
> > > + local offset=$4
> > > + local size=$5
> > > +
> > > + echo 3 > /proc/sys/vm/drop_caches
> >
> >
> > > + while [[ -z $( (( BASHPID % nr_mirrors == mirror )) &&
> > > + exec $XFS_IO_PROG \
> > > + -c "pread -b $size $offset $size" $file) ]]; do
> >
> > I am confused if it should be BASHPID or PID?
> >
> > Next, it is ok if the xfs_io_prog fails and returns != 0.
> > (Part of the test).
> > But then we will continue in the while loop. No?
>
> The loop goes like this:
>
> If "BASHPID % nr_mirrors" isn't mirror, then the whole condition inside
> the $() part is already 0, no need to exec the xfs_io command.
>
> Then -z 0 return true, we continue the while loop (aka, doing nothing).
>
> Only when (BASHPID % nr_mirrors == mirror) is true, we will try to exec
> c XFS_IO_PROGS in the same PID, and normally we will exit.
>
>
> Some of your concern are valid.
> Like if some code bug makes the pread failed, then we will continue the
> loop, possibly lead to an infinite loop.
>
> The pid-check-then-exec idea is pretty good, it allows us to only
> initiate the read, without doing some unnecessary read then check the pid.
>
> But putting the whole exec into the pid check is indeed hacky and can
> lead to problems.
>
> Can we make it less hacky?
Hi Anand and Wenruo,
This patchset has been merged into fstests release v2022.05.29 last weekend.
If you feel something isn't good enough, feel free to send new patches to
fix/improve it.
Thanks,
Zorro
>
> Thanks,
> Qu
>
> Thanks,
> Qu
>
> >
> > Sorry, I am sceptical about this. Could you please clarify
> > how this works?
> >
> >
> > Thanks, Anand
> >
> >
> >
> > > + :
> > > + done
> >
> >
> >
> >
> > > +}
> > > diff --git a/common/config b/common/config
> > > index c6428f90..df20afc1 100644
> > > --- a/common/config
> > > +++ b/common/config
> > > @@ -228,6 +228,7 @@ export E2IMAGE_PROG="$(type -P e2image)"
> > > export BLKZONE_PROG="$(type -P blkzone)"
> > > export GZIP_PROG="$(type -P gzip)"
> > > export BTRFS_IMAGE_PROG="$(type -P btrfs-image)"
> > > +export BTRFS_MAP_LOGICAL_PROG=$(type -P btrfs-map-logical)
> > > # use 'udevadm settle' or 'udevsettle' to wait for lv to be settled.
> > > # newer systems have udevadm command but older systems like RHEL5
> > > don't.
> >
>
