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

[Bob Peterson <rpeterso@redhat.com>]

----- 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? Or else split it
   into begin->main->end? I was kinda hoping to pass in the iomap_actor
   somehow. 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.
3. I had to do something like this to get "make menuconfig" to work:
index 276fcfb..daa129c 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -11,7 +11,7 @@ config DCACHE_WORD_ACCESS
 if BLOCK
 
 config FS_IOMAP
-	bool
+	bool "File IOMAP Support"

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).

Regards,

Bob Peterson
Red Hat File Systems