On Wed, Jun 26, 2019 at 11:10:17AM -0500, Goldwyn Rodrigues wrote:
> 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.
I think it's less complicated to pass both mappings out in a single
->iomap_begin call rather than have this dance where the fs tells iomap
to call back for the read mapping and then iomap calls back for the read
mapping with a special "don't take locks" flag.
For XFS specifically this means we can serve both mappings with a single
ILOCK cycle.
> So, do we move back to the design of adding an extra field of srcaddr?
TLDR: Please no.
> Honestly, I find the design of using an extra field srcaddr in iomap better
> and simpler versus passing additional iomap srcmap or multiple iterations.
Putting my long-range planning hat on, the usage story (from the fs'
perspective) here is:
"iomap wants to know how a file write should map to a disk write. If
we're doing a straight overwrite of disk blocks then I should send back
the relevant mapping. Sometimes I might need the write to go to a
totally different location than where the data is currently stored, so I
need to send back a second mapping."
Because iomap is now a general-purpose API, we need to think about the
read mapping for a moment:
- For all disk-based filesystems we'll want the source address for the
read mapping.
- For filesystems that support "inline data" (which really just means
the fs maintains its own buffers to file data) we'll also need the
inline_data pointer.
- For filesystems that support multiple devices (like btrfs) we'll also
need a pointer to a block_device because we could be writing to a
different device than the one that stores the data. The prime
example I can think of is reading data from disk A in some RAID
stripe and writing to disk B in a different RAID stripe to solve the
RAID5 hole... but you could just be lazy-migrating file data to less
full or newer drives or whatever.
- If we ever encounter a filesystem that supports multiple dax devices
then we'll need a pointer to the dax_device too. (That would be
btrfs, since I thought your larger goal was to enable dax there...)
- We're probably going to need the ability to pass flags for the read
mapping at some point or another, so we need that too.
>From this, you can see that we need half the fields in the existing
struct iomap, and that's how I arrived at the idea of passing to
iomap_begin pointers to two iomaps instead of bolting these five fields
into struct iomap.
In XFS parlance (where the data fork stores mappings for on-disk data
and the cow fork stores mappings for), this means that our iomap_begin
data paths remain fairly straightforward:
xfs_bmapi_read(ip, offset, XFS_DATA_FORK, &imap...);
xfs_bmapi_read(ip, offset, XFS_COW_FORK, &cmap...);
xfs_bmbt_to_iomap(ip, iomap, &imap...);
iomap->type = IOMAP_COW;
xfs_bmbt_to_iomap(ip, srcmap, &cmap...);
(It's more complicated than that, but that's approximately what we do
now.)
> 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.
I think they need to remain distinct types because the IOMAP_F_SHARED
flag means that the storage is shared amongst multiple owners (which
iomap_fiemap reports to userspace), whereas the IOMAP_COW type means
that RMW is required for unaligned writes.
btrfs has a strong tendency to require copy writes, but that doesn't
mean the extent is necessarily shared.
> Dave/Darrick, what are your thoughts?
I liked where the first 3 patches of this series were heading. :)
--D
>
> --
> Goldwyn
