Re: [PATCH] generic/470: Add check for different sync modes

Re: [PATCH] generic/470: Add check for different sync modes

[Amir Goldstein]

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?

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.
>

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.

> 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"
> +_require_command "$FILEFRAG_PROG" filefrag
> +
> +PREFIX=$TEST_DIR/${seq}-testfile
> +TESTFILES=$TEST_DIR/${seq}-testfile-*
> +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
> +       if [ $? -ne 0 ]; then
> +               _notrun "This test requires delayed allocation support!"
> +       fi
> +}
> +
> +sync_data()
> +{
> +       case $1 in
> +       sync)
> +               $XFS_IO_PROG -c "sync" >/dev/null 2>&1
> +               ;;
> +       syncfs)
> +               $XFS_IO_PROG -c "syncfs" ${PREFIX}-${FCNT} >/dev/null 2>&1
> +               ;;
> +       fsync)
> +               for i in `seq 1 $FCNT`; do
> +                       $XFS_IO_PROG -c "fsync" ${PREFIX}-${i} >/dev/null 2>&1
> +               done
> +               ;;
> +       fdatasync)
> +               for i in `seq 1 $FCNT`; do
> +                       $XFS_IO_PROG -c "fdatasync" ${PREFIX}-${i} >/dev/null 2>&1
> +               done
> +               ;;
> +       *)
> +               ;;
> +       esac
> +}
> +


Technically, you don't need all those cases.
This command will do almost the same for all cases and more efficiently
then the for loop.

$XFS_IO_PROG -c $sync_mode ${PREFIX}-* >/dev/null 2>&1

sync happens only once, but files are being opened for no reason.
syncfs happens FCNT times, but that won't change test result.

So up to you if you use this hack for all modes, but I would certainly
change fsync and fdatasync to not use the for loop as xfs_io already
has a built in file iterator.

Thanks!
Amir.

Re: [PATCH] generic/470: Add check for different sync modes

[Eryu Guan]

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.
> 
> 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.

> 
> > 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.

> > +_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.

Thanks,
Eryu

> > +       if [ $? -ne 0 ]; then
> > +               _notrun "This test requires delayed allocation support!"
> > +       fi
> > +}
> > +
> > +sync_data()
> > +{
> > +       case $1 in
> > +       sync)
> > +               $XFS_IO_PROG -c "sync" >/dev/null 2>&1
> > +               ;;
> > +       syncfs)
> > +               $XFS_IO_PROG -c "syncfs" ${PREFIX}-${FCNT} >/dev/null 2>&1
> > +               ;;
> > +       fsync)
> > +               for i in `seq 1 $FCNT`; do
> > +                       $XFS_IO_PROG -c "fsync" ${PREFIX}-${i} >/dev/null 2>&1
> > +               done
> > +               ;;
> > +       fdatasync)
> > +               for i in `seq 1 $FCNT`; do
> > +                       $XFS_IO_PROG -c "fdatasync" ${PREFIX}-${i} >/dev/null 2>&1
> > +               done
> > +               ;;
> > +       *)
> > +               ;;
> > +       esac
> > +}
> > +
> 
> 
> Technically, you don't need all those cases.
> This command will do almost the same for all cases and more efficiently
> then the for loop.
> 
> $XFS_IO_PROG -c $sync_mode ${PREFIX}-* >/dev/null 2>&1
> 
> sync happens only once, but files are being opened for no reason.
> syncfs happens FCNT times, but that won't change test result.
> 
> So up to you if you use this hack for all modes, but I would certainly
> change fsync and fdatasync to not use the for loop as xfs_io already
> has a built in file iterator.
> 
> Thanks!
> Amir.

Re: [PATCH] generic/470: Add check for different sync modes

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 6886 bytes --]

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.

[-- Attachment #2: fstests-delalloc-sync.patch --]
[-- Type: text/x-patch, Size: 7313 bytes --]

Delivered-To: amir73il@gmail.com
Received: by 10.129.112.10 with SMTP id l10csp24825ywc;
        Wed, 29 Nov 2017 18:44:42 -0800 (PST)
X-Google-Smtp-Source: AGs4zMbX/qSN/eyGboOJkhtoHTJt3l/Ii55gW7N0VQ3fOJaXBjqHTLXn5kwxSb+0teafYnGXQ+9r
X-Received: by 10.157.25.239 with SMTP id k102mr4093048otk.342.1512009882171;
        Wed, 29 Nov 2017 18:44:42 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; t=1512009882; cv=none;
        d=google.com; s=arc-20160816;
        b=LC2Z4qejlWW2COdWVpvTOfkakd2Pqw1KnwfLkYtKNFLRRUxd5/OimJ98gOjLtYTvmt
         1VaOmmtnzi91R2v4GKpL4H4JgR9QqJskKnjOPYJs5Dtm0YFPXtOB+fIeXlK0vzjV+MIb
         sGGJFz1LHQZyi0IiUP7UoeBdg9FEsb7BV1Yxf73F9tAXSGF8uEk2/pgbCBcb6cDMu9yL
         2V3mIHfloBELzVy8hp/QcpmHjgUrEMaOe11lhPEgY9chO9CorBdshsBIMkDjISzAqTYl
         m5Hm9UiMc3RdnzeRNG1+v1KO5drREJwUCTGao5OY1b1/ivwg+qTF5Ar5pg7sHiP11AFM
         9oIg==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816;
        h=message-id:date:subject:cc:to:from:arc-authentication-results;
        bh=BlUBjaWAmCh7MM80UPI5kM9k18JyIHhfon/Q6bu5aHE=;
        b=T5wjisJCa0cdeBWs12b3Tvtrg2+WNopL3AMxzPhZip61d3gr1ligibvWYs9xF9vT2J
         twuLv5eAIWwuckasg/OJcpDLE3+GNLbz3KmTRW10YlMigmV4qff3nALNul9kE9JaFK5G
         q3Tf6gfRQItzl4UHBpJAbH++jL0JvBi9h+OhEIpROetZldgU9JiZUk41bOKfOWZj0cV6
         sYMlvJJSuIud8tdQk/cQX0wIiN8BJL7hTknjFXAYQKsS+WXq5DGKYFtVCfPQsCfj8qjM
         Sg/73N18y6GYyERY5jmOBPvzg6Ux7hRrxTb9Kio8QSnr+jZlMdQj4YdzJ/w7GB5VWU0n
         kbVQ==
ARC-Authentication-Results: i=1; mx.google.com;
       spf=neutral (google.com: 122.225.81.134 is neither permitted nor denied by best guess record for domain of cgxu@juanniu018037.ss.mogujie.org) smtp.mailfrom=cgxu@juanniu018037.ss.mogujie.org
Return-Path: <cgxu@juanniu018037.ss.mogujie.org>
Received: from juanniu018037.ss.mogujie.org ([122.225.81.134])
        by mx.google.com with ESMTPS id u144si949957oif.519.2017.11.29.18.44.38
        for <amir73il@gmail.com>
        (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
        Wed, 29 Nov 2017 18:44:41 -0800 (PST)
Received-SPF: neutral (google.com: 122.225.81.134 is neither permitted nor denied by best guess record for domain of cgxu@juanniu018037.ss.mogujie.org) client-ip=122.225.81.134;
Authentication-Results: mx.google.com;
       spf=neutral (google.com: 122.225.81.134 is neither permitted nor denied by best guess record for domain of cgxu@juanniu018037.ss.mogujie.org) smtp.mailfrom=cgxu@juanniu018037.ss.mogujie.org
Received: from juanniu018037.ss.mogujie.org (localhost [127.0.0.1])
	by juanniu018037.ss.mogujie.org (8.14.7/8.14.7) with ESMTP id vAU2hWoo011563;
	Thu, 30 Nov 2017 10:43:32 +0800
Received: (from cgxu@localhost)
	by juanniu018037.ss.mogujie.org (8.14.7/8.14.7/Submit) id vAU2hVHE011562;
	Thu, 30 Nov 2017 10:43:31 +0800
From: Chengguang Xu <cgxu@mykernel.net>
To: amir73il@gmail.com
Cc: linux-unionfs@vger.kernel.org, Chengguang Xu <cgxu@mykernel.net>
Subject: [PATCH] generic/470: Add check for different sync modes
Date: Thu, 30 Nov 2017 10:43:21 +0800
Message-Id: <1512009801-11466-1-git-send-email-cgxu@mykernel.net>
X-Mailer: git-send-email 1.8.3.1

generic/470 should be skipped when delalloc is not supported.

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"
+_require_command "$FILEFRAG_PROG" filefrag
+
+PREFIX=$TEST_DIR/${seq}-testfile
+TESTFILES=$TEST_DIR/${seq}-testfile-*
+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
+	if [ $? -ne 0 ]; then
+		_notrun "This test requires delayed allocation support!"
+	fi
+}
+
+sync_data()
+{
+	case $1 in
+	sync)
+		$XFS_IO_PROG -c "sync" >/dev/null 2>&1
+		;;
+	syncfs)
+		$XFS_IO_PROG -c "syncfs" ${PREFIX}-${FCNT} >/dev/null 2>&1
+		;;
+	fsync)
+		for i in `seq 1 $FCNT`; do
+			$XFS_IO_PROG -c "fsync" ${PREFIX}-${i} >/dev/null 2>&1
+		done
+		;;
+	fdatasync)
+		for i in `seq 1 $FCNT`; do
+			$XFS_IO_PROG -c "fdatasync" ${PREFIX}-${i} >/dev/null 2>&1
+		done
+		;;
+	*)
+		;;
+	esac
+}
+
+check_data()
+{
+	$FILEFRAG_PROG -e $TESTFILES | grep -w delalloc 2>/dev/null
+	if [ $? -eq 0 ]; then
+		status=1
+	fi
+}
+
+test_sync()
+{
+	local sync_mode=$1
+
+	if [ $sync_mode = "sync" ]; then
+		echo 3 > /proc/sys/vm/drop_caches
+	fi
+
+	write_data
+	sync_data $sync_mode
+	check_data
+}
+
+check_delalloc_support
+
+for i in fsync fdatasync syncfs sync; do
+	test_sync $i
+done
+
+rm -f $TESTFILES >/dev/null 2>&1
+
+echo "Silence is golden"
+exit $status
diff --git a/tests/generic/470.out b/tests/generic/470.out
new file mode 100644
index 0000000..79fb532
--- /dev/null
+++ b/tests/generic/470.out
@@ -0,0 +1,2 @@
+QA output created by 470
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 6c3bb03..23e325c 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -472,3 +472,4 @@
 467 auto quick exportfs
 468 shutdown auto quick metadata
 469 auto quick
+470 auto quick sync
-- 
1.8.3.1

Re: [PATCH] generic/470: Add check for different sync modes

[Dave Chinner]

On Thu, Nov 30, 2017 at 10:08:34AM +0200, Amir Goldstein wrote:
> 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.

FWIW, this just seems like an unreliable hack to me.

It doesn't actually validate any of the guarantees that sync/fsync
is supposed to provide, just that "there aren't any delalloc
extents". That, in itself, is a racy check, because other
events can cause the filesystem to convert delalloc extents to
something else without the intervention of sync. e.g. dirty
background writeback kicking in, journal checkpoint on
ext4 in ordered mode kicking data writeback, etc.

And it isn't actually testing data integrity, and it's not even
checking that the correct data has been written.

So why would we want this as a generic test? It doesn't add any
additional test coverage - all it does is increase the runtime of
filesystem testing. I don't see any benefit here...

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

That's also sufficient grounds to say "bad test, don't merge".

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com