Re: Supporting reflinked/cow files in coreutils

[Jim Meyering]

Philipp Thomas wrote:
> Jim,
>
> * Jeff liu (address@hidden) [20110418 15:54]:
>
>> FYI, I once implemented a similar patch to teach du(1) to show the disk
>> footprint with reflinked stuff for OCFS2 only.
>>
>> It is make use of rb-tree to figure out the shared extents and
>> calculate the exactly used space eventually.
>> http://oss.oracle.com/pipermail/ocfs2-devel/2010-September/007293.html
>> http://oss.oracle.com/pipermail/ocfs2-devel/2010-September/007288.html
>> http://oss.oracle.com/pipermail/ocfs2-devel/2010-September/007287.html
>>
>> I have not submited the patch set to Coreutils since I guess it is hard to
>> accept by upstream as reflink and FIEMAP_EXTENT_SHARED flag are not spread
>> over the general filesystems.
>
> in this thread Jeff asked you on your opinions regarding such a patch set
> but somehow you never responded. Could you possibly do so now as I would
> still like something like this to be included upstream.

Hi Philipp,

Glancing through the patch in the first message,

    http://oss.oracle.com/pipermail/ocfs2-devel/2010-September/007293.html

I have a hard time justifying the addition of so much code for a
relatively small feature that will be useful only for OCFS2 file systems:

  coreutils-6.9/src/du.c |  479 +++++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 473 insertions(+), 6 deletions(-)

That's a lot of new always-compiled code that will be used only by a
very small fraction of users.  And to add insult to injury, it extends
the ugly tentacles of FIEMAP into du.c.  It was bad enough to have to
use it in copy.c, and we're all impatient for something better, but
SEEK_HOLE and SEEK_DATA aren't yet widely available.

That's a lot of new code, not even counting these:

  * lib/rbtree.c: Source file of rbtree.
  * lib/rbtree.h: Header file of rbtree.
  * lib/Makefile.am (libcoreutils_a_SOURCES): Add both of them.
  ---
   coreutils-6.9/lib/Makefile.am |    3 +-
   coreutils-6.9/lib/Makefile.in |    4 +-
   coreutils-6.9/lib/rbtree.c    |  403 
+++++++++++++++++++++++++++++++++++++++++
   coreutils-6.9/lib/rbtree.h    |  143 +++++++++++++++
   4 files changed, 550 insertions(+), 3 deletions(-)

Besides which, there is no reason to add new rbtree.[ch] code
when there is already plenty of rbtree code in gnulib.

If it can be made to work cleanly with BTRFS as well, and preferably
*without* FIEMAP (if possible, though I fear that FIEMAP is the only
interface we have for now), that would be even better.

If you and/or Jeff are motivated enough, a good first step would be to
post a rebased patch to bug-coreutils, preferably after you've switched
out that rbtree.[ch] for gnulib's code.  I hesitate even to suggest
that you rebase, because we might end up deciding it's not yet worth
the cost, but let's hear what others have to say.  Maybe enough people
use du and are bothered because the space used by their reflinked files
is over-reported.

A final question: why use an rbtree rather than a hash table?
du is already using hash tables at the required scale, and new code
would be required at all.