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

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

On  8:39 26/06, Christoph Hellwig wrote:
> 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.


So, do we move back to the design of adding an extra field of srcaddr?
Honestly, I find the design of using an extra field srcaddr in iomap better
and simpler versus passing additional iomap srcmap or multiple iterations.

Also, should we add another iomap type IOMAP_COW, or (re)use the flag
IOMAP_F_SHARED during writes? IOW iomap type vs iomap flag.

Dave/Darrick, what are your thoughts?

-- 
Goldwyn