Re: [PATCH 2/2] xfs: reduce ilock acquisitions in xfs_file_fsync

[Dave Chinner <david@fromorbit.com>]

On Fri, Jan 22, 2021 at 05:46:43PM +0100, Christoph Hellwig wrote:
> If the inode is not pinned by the time fsync is called we don't need the
> ilock to protect against concurrent clearing of ili_fsync_fields as the
> inode won't need a log flush or clearing of these fields.  Not taking
> the iolock allows for full concurrency of fsync and thus O_DSYNC
> completions with io_uring/aio write submissions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Code looks good, so

Reviewed-by: Dave Chinner <dchinner@redhat.com>

But it makes me wonder...

That is, we already elide the call to generic_write_sync() in direct
IO in the case that the device supports FUA and it's a pure
overwrite with no dirty metadata on the inode. Hence for a lot of
storage and AIO/io_uring+DIO w/ O_DSYNC workloads we're already
eliding this fsync-based lock cycle.

In the case where we can't do a REQ_FUA IO because it is not
supported by the device, then don't we really only need a cache
flush at IO completion rather than the full generic_write_sync()
call path? That would provide this optimisation to all the
filesystems using iomap_dio_rw(), not just XFS....

In fact, I wonder if we need to do anything other than just use
REQ_FUA unconditionally in iomap for this situation, as the block
layer will translate REQ_FUA to a write+post-flush if the device
doesn't support FUA writes directly.

You're thoughts on that, Christoph?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com