Re: [PATCH 09/15] fs: iomap based fiemap implementation

[Dave Chinner <david@fromorbit.com>]

On Thu, May 26, 2016 at 02:19:57PM -0400, Bob Peterson wrote:
> ----- Original Message -----
> | On Mon, May 23, 2016 at 04:09:26PM -0400, Bob Peterson wrote:
> | > I've been looking at this again. Where are the calls to the fs-specific
> | > bits
> | > for fiemap?
> | 
> | In the iomap_ops structure passed to iomap_fiemap.
> | 
> | > It looks like iomap_fiemap calls iomap_apply, which calls
> | > iomap_fiemap_actor, but that doesn't call any ops->iomap_get_iomap or
> | > similar.
> | > It calls the iomap_begin (which BTW has a comment that says "Execute a
> | > iomap
> | > write" which is probably wrong and should be more generic, as for cases
> | > like
> | > fiemap) and it calls iomap_end. But it never calls an fs-specific actor
> | > anywhere. Am I missing something? My earlier version passed in the actor
> | > function, as per Dave Chinner's request, but yours doesn't.
> | 
> | The iomap_begin callback is where you do the mapping.  the iomap_end
> | callback does any required cleanup, which in case of GFS2 probably
> | would be dropping the cluster lock protecting the mapping.
> | 
> 
> Okay, got it. So a couple things:
> 
> 1. I verified that the vfs bits of the patch set work properly for GFS2
>    using a modified iomap-based fiemap. And it's fast.
> 2. I'm not sure I like the fact that instead of begin->main->end it is
>    essentially begin->end with begin doing all the work.  It works, and
>    it's better than we have today. But I'd prefer either renaming the
>    first function from "iomap_begin" to something that indicates it's
>    more than just a precursor to the actual function?

<shrug> Same can be said for the existing write_begin/end interface
we have for block mapping right now. I suppose iomap_prepare/finish
or iomap_get/put might be more obvious. Pink or purple? :P

>    Or else split it
>    into begin->main->end? I was kinda hoping to pass in the iomap_actor
>    somehow.

Can you explain what for? What are you wanting to put in the fiemap
that you can't pass though the iomap as a map flag?

>    It's not a tragic loss, but the way I've got the gfs2
>    function coded, the begin function does: locking plus the main
>    functionality, and all the fiemap_end function basically does is unlock.

Yup, that's pretty much how it was been intended to work.

> 4. I don't suppose you could split this patch set up so that the vfs
>    bits are independent, so someone like Al Viro could grab them
>    rather than getting pulled along size the xfs-specific bits?
>    (Which is essentially what I've done for GFS2; I did not drag in
>    all the xfs bits).

Once the merge window is over and the VFS bits are stable and
reviewed, I'll put the code into two branches in the XFS tree - the
first will just have the VFS iomap bits and the second will contain
the XFS bits. Once committed, they'll be stable branches, so you
should be able to merge it into the GFS2 tree and work from there.
The duplication of the branch in different trees will resolve
automatically when Linux pulls the trees. That way I can carry the
XFS changes in the XFS tree, and you don't have to see any of them
in the GFS2 tree...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com