Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Dave Chinner <david@fromorbit.com>]

On Fri, Dec 01, 2017 at 09:21:15AM +0200, Amir Goldstein wrote:
> On Fri, Dec 1, 2017 at 8:38 AM, Chengguang Xu <cgxu@mykernel.net> wrote:
> >
> >> 在 2017年12月1日，下午12:13，Eryu Guan <eguan@redhat.com> 写道：
> >>
> >> On Fri, Dec 01, 2017 at 12:04:44PM +0800, Chengguang Xu wrote:
> >>> Hi Eryu,
> >>>
> >>> Actually, in my another test case generic/470 will need to check whether fs supports syncfs or not.
> >>> I make shared infrastructure for checking that, and because it is common component
> >>> I post as an individual patch instead of including in the case of generic/470.
> >>
> >> I think "_require_xfs_io_command syncfs" should be fine, there's no need
> >> & not encouraged to add new binary & usage like this. If you want to run
> >> syncfs(2) to make sure the kernel actually supports it, you can add a
> >> new 'syncfs' switch case in _require_xfs_io_command.
> >>
> >
> > Failure of _require_xfs_io_command check leads to notrun, if we have several
> > sync patterns(combination of fsync/fdatasync/syncfs/sync) in an actual test case,
> > the case will lose downward compatibility for old kernel.
> > In this situation, we have to split test case though they look similar.
> >
> 
> You have 2 options:
> Easy - split 2 tests - 1 requires syncfs and tests syncfs, 1 does not
> require and does not test syncfs
> Work - re-factor  _require_xfs_io_command to _check_xfs_io_command
> that returns the error
> message but does not _notrun. use that hepler in your test to
> conditionally test syncfs.
> 
> Anyway, I see people are not so fond of the delalloc "canary test".
> Perhaps a still simple and quick test would be to do small buffered
> write; sync; small direct io read?

No, because the direct IO read can flush dirty buffers across the
range the IO is being done on. IOWs, you don't know if the sync
actually flushed it, the direct IO read flushed it, or the direct IO
read fell back to buffered IO and simply read the in memory copy.

Let's step back a moment: What bug/regression are you actually
trying to expose/reproduce here? Why is it not already covered by at
least one of the other generic sync/fsync tests we already have?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com