Re: [PATCH] generic/409: reflink concurrent operations

[Eryu Guan <eguan@redhat.com>]

[Didn't see my reply hit the list, resend]

On Mon, Feb 27, 2017 at 04:26:23PM +0200, nave vardy wrote:
> On Fri, 2017-02-24 at 17:47 +0800, Eryu Guan wrote:
> > On Thu, Feb 23, 2017 at 02:30:54PM +0200, Nave Vardy wrote:
> > > 
> > > reflink: concurrent operations test
> > > 
> > > perform read operation on the target file while
> > > doing write or fpunch operations on the reflinks.
> > I'm having the same question as Darrick's, is this test reproducing
> > some
> > kind of bugs for you?
> Maybe it would be better if I'll first explain my motivation for
> sending the patch. I am working at Plexistor a company that develops a
> new file-system for persistent memory. The file-system supports cow,
> and we wrote some fstests, we thought to contribute for the community.
> For the test itself, it helped us to found a bug in the development of
> cow, no other generic test currently exists did.

Thanks a lot for writing new tests!

> > 
> > > 
> > > 
> > > Signed-off-by: Nave Vardy <nave.vardy@plexistor.com>
> > > ---
> > >  tests/generic/409     | 97
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/409.out |  2 ++
> > >  tests/generic/group   |  1 +
> > >  3 files changed, 100 insertions(+)
> > >  create mode 100644 tests/generic/409
> > 0755 file mode for new test. This is done automatically if you use
> > "./new generic" to setup new test template.
> > 
> In the next version of the patch I'll use this.
> > > 
> > >  create mode 100644 tests/generic/409.out
> > > 
> > > diff --git a/tests/generic/409 b/tests/generic/409
> > > new file mode 100644
> > > index 0000000..eb1da13
> > > --- /dev/null
> > > +++ b/tests/generic/409
> > > @@ -0,0 +1,97 @@
> > > +#!/bin/bash
> > > +# FS QA Test No. 409
> > > +#
> > > +# test for races between write or fpunch operations on reflinked
> > > files
> > > +# to read operations on the targed file
> > > +#
> > > +#-----------------------------------------------------------------
> > > ------
> > > +#
> > > +# Copyright (c) 2017 Plexistor Ltd. All Rights Reserved.
> > > +#
> > > +# This program is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU General Public License as
> > > +# published by the Free Software Foundation.
> > > +#
> > > +# This program is distributed in the hope that it would be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public
> > > License
> > > +# along with this program; if not, write the Free Software
> > > Foundation,
> > > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > > +#
> > > +#-----------------------------------------------------------------
> > > ------
> > > +
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +status=0    # success is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/reflink
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# real QA test starts here
> > > +_supported_fs generic
> > > +_supported_os Linux
> > > +_require_scratch_reflink
> > > +_require_cp_reflink
> > > +
> > > +_scratch_mkfs_sized $((400 * 1024 ** 2)) >> $seqres.full 2>&1 ||
> > > _fail "mkfs failed"
> > Same question on the filesystem size.
> > 
> I used _scratch_mkfs_sized to avoid enospace in this scenario. 
> I took a lager margin than I needed, which I can narrow down.
> The targted file is 10mb and I used 20 reflinks, because the tests
> writes the reflinks, in the end each will take place of max 10mb (it is
> not accurate because I use fpunch for every second block), so we have
> 21 files sum to max size of 210mb, so I think 250mb will be good
> enough.

Then I think, as Darrick suggested, _require_fs_space() is what you need.

> > > 
> > > +_scratch_mount || _fail "mount failed"
> > > +
> > > +echo "Silence is golden"
> > > +
> > > +workfile=${SCRATCH_MNT}/workfile
> > > +light_clone=${SCRATCH_MNT}/light_clone
> > > +
> > > +file_size=$((10 * 1024 * 1024))
> > > +bs=4096
> I used 4096 because this is our file-system copy-on-write granularity,
> this is in order to get maximum effect. I'll be able to use
> _get_file_block_size().
> > > +block_num=$((file_size / bs))
> > > +reflinks_num=20
> > > +
> > > +$XFS_IO_PROG -f -c "pwrite 0 $file_size" $workfile >> $seqres.full
> > > +
> > > +#create all reflinkfs
> > > +for ((i=1; i<reflinks_num; i++));
> > > +do
> > > +	cp --reflink $workfile ${light_clone}_$i
> I'll change this to use _cp_reflink()
> > > +done
> > > +
> > > +#for each block simultaneously write to all reflinks
> > > +for ((block_index=0; block_index<block_num; block_index++));
> > > +do
> > > +	for ((i=0; i<reflinks_num; i++));
> > > +	do
> > > +		if [ $i -eq 0 ]; then
> > > +			$XFS_IO_PROG -c "pread $((block_index *
> > > bs)) $bs" \
> > > +						$workfile >>
> > > $seqres.full &
> > > +		elif [ $((block_index % 2)) -eq 0 ]; then
> > > +			$XFS_IO_PROG -c "pwrite $((block_index *
> > > bs)) $bs" \
> > > +					${light_clone}_$i  >>
> > > $seqres.full &
> > > +		else
> > > +			$XFS_IO_PROG -c "fpunch $((block_index *
> > > bs)) $bs" \
> > > +					${light_clone}_$i >>
> > > $seqres.full &
> > > +		fi
> > I don't see much concurrency in this loop, for each block_index
> > there's
> > only one pread process and reflinks_num processes doing pwrite or
> > fpunch. Seems only pwrites are racing with pwrites and fpunchs are
> > racing with fpunchs. How about:
> > 
> > for ((block_index=0; block_index<block_num; block_index++)); do
> > 	for ((i=0; i<reflinks_num; i++)); do
> > 		$XFS_IO_PROG -c "pread ..." ... &
> > 		if [ $((i % 2)) -eq 0 ]; then
> > 			$XFS_IO_PROG -c "pwrite ..." ... &
> > 		else
> > 			$XFS_IO_PROG -c "fpunch ..." ... &
> > 		fi
> > 	done
> > 	wait
> > done
> > 
> > So for every pwrite or fpunch there's a pread racing with it. But
> > because I don't know if you're reproducing some specific bug with
> > this
> > loop, I'm not sure if my version still works.
> > 
> I understand your comment and your suggestion, we interntionally didn't
> won't to modify the workfile so we just read it and race the pread with
> the pwrites of the reflinks (or the fpunchs). we wanted that all
> reflinks simultaneously will perform pwrite and in the next iteration
> fpunch (and not half reflinks pwrite and the other half fpunch - which
> is fine but describes another scenario). I think we can combine your
> suggestion with my version, to get more concurrency like this:

Looks reasonable, please add some comments to describe this workload.

> 
> for ((block_index=0; block_index<block_num; block_index++)); do
>      for ((i=0; i<reflinks_num; i++)); do

Perhaps we can start with i=1 then remove the "$i -ne 0" check?

Thanks,
Eryu

>           $XFS_IO_PROG -c "pread $((block_index * 	bs)) $bs" \
> 					$workfile >> $seqres.full &
>           if [ $i -ne 0 ]; then
>               if [ $((block_index % 2)) -eq 0 ]; then
> 		  $XFS_IO_PROG -c "pwrite $((block_index * bs)) $bs" \        
>                             ${light_clone}_$i >> $seqres.full &
>               else
>                   $XFS_IO_PROG -c "fpunch $((block_index * bs)) $bs" \
>                                     ${light_clone}_$i >> $seqres.full &
>               fi
>            fi
>        wait
>        done
> done		 
> > > +	wait
> > > +done
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/409.out b/tests/generic/409.out
> > > new file mode 100644
> > > index 0000000..6f11537
> > > --- /dev/null
> > > +++ b/tests/generic/409.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 409
> > > +Silence is golden
> > > diff --git a/tests/generic/group b/tests/generic/group
> > > index d14f221..27ff229 100644
> > > --- a/tests/generic/group
> > > +++ b/tests/generic/group
> > > @@ -411,3 +411,4 @@
> > >  406 auto quick dangerous
> > >  407 auto quick clone metadata
> > >  408 auto quick clone dedupe metadata
> > > +409 auto quick clone
> > If you decide to increase the concurrency/workload, probably the test
> > will not be quick enough to fit in quick group :)
> > 
> It is not too important if it would only be in clone and auto
> > Thanks,
> > Eryu
> > 
> > >