Re: [PATCH 08/10] iomap: New iomap_written operation

[Dave Chinner <david@fromorbit.com>]

On Thu, Jan 11, 2018 at 10:15:02PM +0100, Andreas Gruenbacher wrote:
> Add a callback to iomap_file_buffered_write that's called whenever
> writing to a page has completed.  This is needed for implementing data
> journaling: in the data journaling case, pages are written into the
> journal before being written back to their proper on-disk locations.
> 
> (So far, the only user of iomap_file_buffered_write is xfs, which
> doesn't do data journaling.)
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/iomap.c            | 21 +++++++++++++++++++--
>  include/linux/iomap.h |  9 +++++++++
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 47d29ccffaef..98903be66c35 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -149,11 +149,21 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
>  	return ret;
>  }
>  
> +struct iomap_write_data {
> +	struct iov_iter *iter;
> +	const struct iomap_ops *ops;
> +};
> +
>  static loff_t
>  iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>  		struct iomap *iomap)
>  {
> -	struct iov_iter *i = data;
> +	struct iomap_write_data *d = data;
> +	struct iov_iter *i = d->iter;
> +	void (*iomap_written)(struct inode *inode, struct page *page,
> +			      unsigned int offset, unsigned int length,
> +			      unsigned int written) =
> +		d->ops->iomap_written;
>  	long status = 0;
>  	ssize_t written = 0;
>  	unsigned int flags = AOP_FLAG_NOFS;
> @@ -198,6 +208,9 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>  
>  		flush_dcache_page(page);
>  
> +		if (iomap_written)
> +			iomap_written(inode, page, offset, bytes, copied);
> +
>  		status = iomap_write_end(inode, pos, bytes, copied, page);
>  		if (unlikely(status < 0))
>  			break;

This looks like it should replace iomap_write_end() as it has
pretty much the same parameters are passed to it.

i.e. it seems to me like a better solution would be to add
->write_begin/->write_end ops to the struct iomap_ops and have
existing implementations set them up as iomap_write_begin()/
iomap_write_end(). Then gfs2 can do it's special little extra bit
and then call iomap_write_end() in the one call...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com