generic/470: Add check for different sync modes

On Thu, Nov 30, 2017 at 9:22 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Thu, Nov 30, 2017 at 07:30:41AM +0200, Amir Goldstein wrote:
>> Adding fstests list and maintainer in CC, because this is where this
>> patch in meant to go.
>>
>> Eryu,
>>
>> This test is expected to fail with overlayfs on current upstream and
>> any past version
>> AFAIK.
>> Do you this it qualifies for a specific overlay/* regression test? or
>> that generic/* test
>> would be sufficient to cover the overlayfs issue?
>
> If it reproduces the overlay bug without any special overlay setup, a
> generic test would be good, other filesystems could benefit from this
> test too.
>
> And Chengguang, can you please send the full version of the test to
> fstests@vger.kernel.org again? I can only see and comment on the trimmed
> version now.

Yeh, sorry about that. Attaching patch here until v2 is posted to fstests.

>>
>> On Thu, Nov 30, 2017 at 4:43 AM, Chengguang Xu <cgxu@mykernel.net> wrote:
>> > generic/470 should be skipped when delalloc is not supported.
>
> What happens if there's no delalloc support? If test fails due to that's
> not a valid setup or wrong usage of overlay (I highly doubt it :), I
> agree we should _notrun in this case. If test passes without reproducing
> the bug, I'd prefer continuing run the test to get better test coverage,
> just write comments to describe this case.
>
>> >
>>
>> Test looks very good. One minor nit below.
>>
>>
>> Not sure why you choose the minor detail in the line above as the
>> commit description?
>> Seems like the text in the top comment of the test would have been also
>> appropriate for commit description.
>
> Yeah, please describe the test purpose in commit log, if you're testing
> a specific overlay bug, describe that bug too and refering to the patch
> that fixed the bug would be good. And it's worth mentioning the test
> behavior when delalloc is not supported.
>

Test is not_run when delalloc is not supported.
I should explain.
This test is inspired by a simple test script I wrote
https://github.com/amir73il/overlayfs/blob/master/tests/xfs_syncfs.sh

At the time when I *thought* I fixed syncfs, I couldn't figure out a way
to write a generic test so I left it at that.

The delalloc trick, which I recently added to the script is just a very cheap
way of testing that inode pages are flushed.
If fs doesn't support delalloc, then test would have to use dm-flakey or a like
and compare size/md5sum on disk to expected.
That is a much more heavy weight test and I can't think of how to combine
dm-flakey setup with overlay setup.

It is just too much of a hustle considering that the delalloc trick is so cheap
and works on several major fs.

>>
>> > Signed-off-by: Chengguang Xu <cgxu@mykernel.net>
>> > ---
>> >  tests/generic/470     | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  tests/generic/470.out |   2 +
>> >  tests/generic/group   |   1 +
>> >  3 files changed, 136 insertions(+)
>> >  create mode 100755 tests/generic/470
>> >  create mode 100644 tests/generic/470.out
>> >
>> > diff --git a/tests/generic/470 b/tests/generic/470
>> > new file mode 100755
>> > index 0000000..a43fb91
>> > --- /dev/null
>> > +++ b/tests/generic/470
>> > @@ -0,0 +1,133 @@
>> > +#! /bin/bash
>> > +# FS QA Test No. 470
>> > +#
>> > +# Run fsync & fdatasync & syncfs & sync to test sync result.
>> > +#
>> > +#-----------------------------------------------------------------------
>> > +# Copyright (c) 2017 Chengguang Xu <cgxu@mykernel.net>. 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
>> > +trap "_cleanup; exit \$status" 0 1 2 3 15
>> > +
>> > +_cleanup()
>> > +{
>> > +       cd /
>> > +       rm -f $tmp.*
>> > +}
>> > +
>> > +# get standard environment, filters and checks
>> > +. ./common/rc
>> > +. ./common/filter
>> > +
>> > +# remove previous $seqres.full before test
>> > +rm -f $seqres.full
>> > +
>> > +# real QA test starts here
>> > +
>> > +# Modify as appropriate.
>> > +_supported_fs generic
>> > +_supported_os Linux
>> > +_require_test
>> > +_require_xfs_io_command "fsync"
>> > +_require_xfs_io_command "fdatasync"
>> > +_require_xfs_io_command "syncfs"
>> > +_require_xfs_io_command "sync"
>
> Seems not necessary except the "syncfs" case, as older kernel doesn't
> support syncfs(2), but I think there's nothing wrong with having them
> checked explicitly.
>

Actually, it is quite not nice not to run the test on old kernels.
Better check for availability of syncfs and if it exists add it to
SYNC_MODES to be tested.

>> > +_require_command "$FILEFRAG_PROG" filefrag
>> > +
>> > +PREFIX=$TEST_DIR/${seq}-testfile
>> > +TESTFILES=$TEST_DIR/${seq}-testfile-*
>
> Why not just use $PREFIX-*?
>
>> > +FCNT=1000
>> > +
>> > +write_data()
>> > +{
>> > +       rm -f $TESTFILES >/dev/null 2>&1
>> > +       for i in `seq 1 $FCNT`; do
>> > +               $XFS_IO_PROG -f -c "pwrite 1K 1K" \
>> > +                               ${PREFIX}-$i >/dev/null 2>&1
>> > +       done
>> > +}
>> > +
>> > +check_delalloc_support()
>> > +{
>> > +       write_data
>> > +       $FILEFRAG_PROG -e $TESTFILES | grep -w delalloc >/dev/null 2>&1
>
> This looks fragile and requires all the files are not written back to
> disk, it's unlikely but still possible someone flushed the files before
> filefrag run.
>
> Anyway, I think this function can be dropped if we decide continue to
> run test if delalloc is not supported.
>

This test does not guaranty failure on buggy fs, but it does provide
very very high
probability of failure on buggy fs. Anything else would be much more
complicated.
Anything else would also not be as quick, because it will require writing large
amounts of data and waiting for sync/fsync/syncfs to flush them after
every write.

I suggest to merge this test as is and add commentary about the possibility
of false negative.

Chengguang may continue to work on another test that is more reliable and
doesn't require delalloc support if he chooses to.

Thanks,
Amir.

generic/470: Add check for different sync modes

Commit Message

Comments

Patch