On 8/12/13 10:10 PM, Dave Chinner wrote:

On Sat, Aug 03, 2013 at 09:27:40PM -0400, Theodore Ts'o wrote:

quoted

On Tue, Jul 30, 2013 at 01:08:07PM +1000, Dave Chinner wrote:

quoted

But Ted's case is not a "hint" - it's a direct command to fetch the
extent map from disk. You can do that already with FIEMAP, so no new
code or interfaces are needed. fadvise() is not the proper interface
for manipulating filesystem metadata behaviour, and fiemap can
already do what you need. There is no need for any new interfaces
here.

I've been looking at the definition of fiemap, and I'm not convinced.
To quote from the fiemap.txt:

   The fiemap ioctl is an efficient method for userspace to get file
   extent mappings.

Ted - 

Changing fiemap.txt is easy, if that's the only problem... :)

quoted

That's not what is going on here.  We are pre-caching them into kernel
memory, not in user-space.  In addition, we're also setting a flag to
keep these extents preferentially in memory compared to other entries
in the extent cache.

Reading extents via fiemap almost certainly moves that metadata into
kernel cache, simply by the act of reading the block device to get them.

It doesn't set any caching preference, but how needed is that, really,
in practice?

quoted

I agree that posix_fadvise() isn't really a good match, either:

   "posix_fadvise - predeclare an access pattern for file data"

How about this?  FIEMAP is an ioctl, anyway.  How about if we just
declare this as a new fs-independent ioctl, much like FS_IOC_FIEMAP?

#define FS_IOC_PRECACHE_EXTENTS    _IO('f', 18)

This is, of course, assuming that other file systems are interested in
implementing this functionality.  If not, we can just keep it as
EXT4_IOC_PRECACHE_EXTENTS, and just call it a day.  (We can always add
a definition of FS_IOC_PRECACHE_EXTENTS set to ext4 ioctl's code
point, at some later point, if people change their minds.)

We *don't need to add any code* to the kernel to read extents into
the kernel cache. The FIEMAP interface as it exists today -without
modification- fulfils your stated requirement. 

I do no see any reason for adding a new, duplicated interface that
we have to maintain and hook up to all the relevant filesystems,
write test code for and then support forever more. It just makes no
sense at all.

I see Dave's point that we _do_ have an interface today to read
all file extents into cache.  We don't mark them as particularly sticky,
however.

This seems pretty clearly driven by a Google workload need; something you
can probably test.  Does FIEMAP do the job for you or not?  If not, why not?

-Eric

Cheers,

Dave.