From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicholas Miell Subject: Re: [RFC][PATCH] Implement SEEK_HOLE/SEEK_DATA Date: Wed, 28 Nov 2007 15:50:40 -0800 Message-ID: <1196293840.18231.24.camel@entropy> References: <20071128200206.GB3977@dhcp243-37.rdu.redhat.com> <1196290614.18231.17.camel@entropy> <20071128233327.GF3977@dhcp243-37.rdu.redhat.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, Andreas Dilger , Christoph Hellwig To: Josef Bacik Return-path: Received: from qmta07.westchester.pa.mail.comcast.net ([76.96.62.64]:46844 "EHLO QMTA07.westchester.pa.mail.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755659AbXK1Xuo (ORCPT ); Wed, 28 Nov 2007 18:50:44 -0500 In-Reply-To: <20071128233327.GF3977@dhcp243-37.rdu.redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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