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@xxxxxxxxxxx> - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
