Re: [PATCH 1/6] iomap: Use a IOMAP_COW/srcmap for a read-modify-write I/O

[Christoph Hellwig <hch@lst.de>]

On Tue, Jun 25, 2019 at 02:14:42PM -0500, Goldwyn Rodrigues wrote:
> > I can't say I'm a huge fan of this two iomaps in one method call
> > approach.  I always though two separate iomap iterations would be nicer,
> > but compared to that even the older hack with just the additional
> > src_addr seems a little better.
> 
> I am just expanding on your idea of using multiple iterations for the Cow case
> in the hope we can come out of a good design:
> 
> 1. iomap_file_buffered_write calls iomap_apply with IOMAP_WRITE flag.
>    which calls iomap_begin() for the respective filesystem.
> 2. btrfs_iomap_begin() sets up iomap->type as IOMAP_COW and fills iomap
>    struct with read addr information.
> 3. iomap_apply() conditionally for IOMAP_COW calls do_cow(new function)
>    and calls ops->iomap_begin() with flag IOMAP_COW_READ_DONE(new flag).
> 4. btrfs_iomap_begin() fills up iomap structure with write information.
> 
> Step 3 seems out of place because iomap_apply should be iomap.type agnostic.
> Right?
> Should we be adding another flag IOMAP_COW_DONE, just to figure out that
> this is the "real" write for iomap_begin to fill iomap?
> 
> If this is not how you imagined, could you elaborate on the dual iteration
> sequence?

Here are my thoughts from dealing with this from a while ago, all
XFS based of course.

If iomap_file_buffered_write is called on a page that is inside a COW
extent we have the following options:

 a) the page is updatodate or entirely overwritten.  We cn just allocate
    new COW blocks and return them, and we are done
 b) the page is not/partially uptodate and not entirely overwritten.

The latter case is the interesting one.  My thought was that iff the
IOMAP_F_SHARED flag is set __iomap_write_begin / iomap_read_page_sync
will then have to retreive the source information in some form.

My original plan was to just do a nested iomap_apply call, which would
need a special nested flag to not duplicate any locking the file
system might be holding between ->iomap_begin and ->iomap_end.

The upside here is that there is no additional overhead for the non-COW
path and the architecture looks relatively clean.  The downside is that
at least for XFS we usually have to look up the source information
anyway before allocating the COW destination extent, so we'd have to
cache that information somewhere or redo it, which would be rather
pointless.  At that point the idea of a srcaddr in the iomap becomes
interesting again - while it looks a little ugly from the architectural
POV it actually ends up having very practical benefits.
> 
> 
> -- 
> Goldwyn
---end quoted text---