Re: [RFC][PATCH] Implement SEEK_HOLE/SEEK_DATA

[Nicholas Miell <nmiell@comcast.net>]

On Wed, 2007-11-28 at 18:33 -0500, Josef Bacik wrote:
> On Wed, Nov 28, 2007 at 02:56:54PM -0800, Nicholas Miell wrote:
> > 
> > On Wed, 2007-11-28 at 15:02 -0500, Josef Bacik wrote:
> > > Hello,
> > > 
> > > This is my first pass at implementing SEEK_HOLE/SEEK_DATA.  This has been in
> > > solaris for about a year now, and is described here
> > > 
> > > http://docs.sun.com/app/docs/doc/819-2241/lseek-2?l=en&a=view&q=SEEK_HOLE
> > > http://blogs.sun.com/roller/page/bonwick?entry=seek_hole_and_seek_data
> > > 
> > > I've added a file operation to allow filesystems to override the default
> > > seek_hole_data function, which just loops through bmap looking for either a hole
> > > or data.  I've tested this and it seems to work well.  I ran my testcase on a
> > > solaris box to make sure I got consistent results (I just ran my test script on
> > > the solaris box, I haven't looked at any of their code in case thats a concern).
> > > All comments welcome.  Thank you,
> > > 
> > > Josef
> > 
> > I stand by my belief that SEEK_HOLE/SEEK_DATA is a lousy interface.
> > 
> > It abuses the seek operation to become a query operation, it requires a
> > total number of system calls proportional to the number holes+data and
> > it isn't general enough for other similar uses (e.g. total number of
> > contiguous extents, compressed extents, offline extents, extents
> > currently shared with other inodes, extents embedded in the inode
> > (tails), etc.)
> > 
> > Something like the following would be much better:
> > 
> > int getfilextents(int fd, off_t offset, int type, size_t *length, struct
> > extent *extents)
> > 
> > with
> > 
> > int fd: open file
> > 
> > offset: offset in file to start reporting extents
> > 
> > type: one of EXTENT_TYPE_HOLE, EXTENT_TYPE_DATA, EXTENT_TYPE_EXTENTS,
> > EXTENT_TYPE_COMPRESSED, EXTENT_TYPE_UNCOMPRESSED etc.
> > 
> > length: in/out parameter, on entry contains length of extents array, on
> > exit contains number of valid entries in the extents array or total
> > number of extents remaining in the file, whichever is larger
> > 
> > extents: array of struct extent { off_t offset; off_t length }, only
> > updated if non-NULL
> > 
> > Making the type parameter a bitmask and adding a type member to struct
> > extent could be useful so that multiple types of extents could be
> > reported at once could be useful, too. (But you end up with weird cases
> > like data extents overlapping with compressed extents.)
> > 
> > Actually, now that I've searched my mailbox, Andreas Dilger's FIEMAP
> > proposal is pretty much what I suggest here and is certainly superior to
> > Sun's SEEK_HOLE/SEEK_DATA.
> >
> 
> Agreed, however in speaking hch and others the consensus was FIEMAP was good,
> however there was no reason why SEEK_HOLE/SEEK_DATA shouldn't also be
> implemented, and then at some point down the road when a generic FIEMAP is in
> place either change the SEEK_HOLE/SEEK_DATA implementation to try to use FIEMAP
> by default and then fall back on bmap if it has to, or some other such
> operation.  I'm cool with passing on this implementation in preference for
> FIEMAP, but given the discussion I had earlier this week with some of the other
> fs people the general thought was go ahead and do this for now.
> 

Well, there's no demand specifically for SEEK_HOLE/SEEK_DATA[1] and the
interface is ugly, so killing it before it spreads beyond Solaris seems
like a good idea to me. OTOH, if you implement SEEK_HOLE/SEEK_DATA,
nobody is going to bother using the good interface if SEEK_HOLE/
SEEK_DATA is the only portable interface.



[1] The only user appears to be Joerg Schilling.
http://www.google.com/codesearch?hl=en&q=+%5CWSEEK_(DATA%7CHOLE)&sa=N&as_case=y

-- 
Nicholas Miell <nmiell@comcast.net>