Re: cp/reflink support for OCFS2

[Pádraig Brady]

On 05/03/2011 03:47 PM, jeff.liu wrote:
> Hello All,
> 
> I'd like to introduce the ocfs2 reflink support to cp(1) when it was called 
> with
> "--reflink=[WHEN]".
> With this patch, `cp' will try OCFS2 reflink first, if it fails with EEXIST, 
> IMHO, it definitely
> means the user is intended to perform reflink on OCFS2, but the destination 
> file is already exists,
> so set the retval = false and return, or try Btrfs clone again.
> 
> I have done some tests, includes reflink on OCFS2, reflink to an existing 
> file, reflink files
> cross-filesystems, and reflink attributes only, all works fine.
> 
> For the test automation, the existing reflink test are presume the tests 
> running on either file
> systems with Cow support IMO, maybe we can improve them with real filesystems 
> on loop device?
> 
> Also, the old bug ticket for this topic will be closed at:
> http://lists.gnu.org/archive/html/bug-coreutils/2010-04/msg00185.html

Thanks for doing this.

It's a pity we've to try different file system specific CoW methods.
Is there any news on the reflink system call?
https://lkml.org/lkml/2009/9/14/532

I notice that xattrs are copied with REFLINK_ATTR_NONE.
Is that also the case for BTRFS clone?
In any case that's a change I'm not sure is desired, or worth noting at least.

I don't understand this bit...

> +      if (reflink_ok)
> +        {
             ...
> +        }
> +      else
> +        {
> +          /* When the destination file is aready exists on OCFS2, the
> +             above reflink should fails with EEXIST.  If that happens,
> +             we need not try btrfs clone again since it means the user
> +             is definitely want a OCFS2 reflink.  */
> +          if (errno == EEXIST)

Should this not be errno != ENOTSUP
Or why not try to unlink() (dependent on -f, -n etc.).
At least shouldn't we be falling back to a regular copy when --reflink=auto

> +            {
> +              error (0, errno, _("failed to reflink ..."));
> +              return_val = false;
> +              goto close_src_desc;
> +            }

cheers,
Pádraig.