Re: [PATCH v2 2/6] ovl: respect FIEMAP_FLAG_SYNC flag

[Dave Chinner <david@fromorbit.com>]

On Sun, Aug 26, 2018 at 07:25:13PM +0300, Amir Goldstein wrote:
> Stacked overlayfs fiemap operation broke xfstests that test delayed
> allocation (with "_test_generic_punch -d"), because ovl_fiemap()
> failed to write dirty pages when requested.
> 
> Fixes: 9e142c4102db ("ovl: add ovl_fiemap()")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/inode.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index e0bb217c01e2..5014749fd4b4 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -467,6 +467,10 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  		return -EOPNOTSUPP;
>  
>  	old_cred = ovl_override_creds(inode->i_sb);
> +
> +	if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
> +		filemap_write_and_wait(realinode->i_mapping);
> +
>  	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);


Where's the fiemap_check_flags() call in the overlay code to
indicate to userspace what functionality ovl supports?

And, further, you can't take action on FIEMAP_FLAG_SYNC for the
lower filesystem file because the lower filesystem first has to
validate the fiemap flags passed in.

So if you need to process FIEMAP_FLAG_SYNC here for the lower
filesystem, that implies that there is a bug in the filesystem
implementations and/or the VFS fiemap behaviour.

e.g. in XFS we call iomap_fiemap(), and it does:

        ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
        if (ret)
                return ret;

        if (fi->fi_flags & FIEMAP_FLAG_SYNC) {
                ret = filemap_write_and_wait(inode->i_mapping);
                if (ret)
                        return ret;
        }

That means you wouldn't have seen this bug on XFS. Ext4 does not do
this, so it would appear not to observe the FIEMAP_FLAG_SYNC
behaviour as it was asked to perform.

Ah, I see - the problem is ioctl_fiemap() - it assumes that it can
run the flush without first allowing the filesystem to check if that
flag is supported.

So, shouldn't the correct fix be to move the FIEMAP_FLAG_SYNC from
the VFS down into the filesystem implementations after they have
checked the flags field for supported functionality? That way ovl
doesn't need special case hacks to replicate VFS behaviour...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com