Re: [coreutils] cp/reflink-perm fails on btrfs (and probably on ocfs2, too)

[Erik Auerswald]

Hi,

On Tue, Oct 26, 2010 at 12:57:08PM +0200, Jim Meyering wrote:
> Erik Auerswald wrote:
> > On Tue, Oct 26, 2010 at 11:39:58AM +0200, Jim Meyering wrote:
> >> The cp/reflink-perm test fails on a btrfs file system:
> >>
> >>     echo > file2 # file with data
> >>     cp --reflink=auto --preserve --attributes-only file2 empty_copy || 
> >> fail=1
> >>     test -s empty_copy && fail=1
> >>
> >> because with --reflink=auto, the file contents are copied, too,
> >
> > Is this intentional? I would have expected that no data is copied, because
> > this is stated in the documentation. The --reflink option should change
> > behaviour of a data copy only, IMHO.
> 
> Thanks for the feedback.
> 
> I wondered the same thing and looked at this part of copy.c,
> but I didn't look carefully enough.
> 
> [data_copy_required defaults to true, but is set to false
>  with --attributes-only ]
> 
>   if (x->reflink_mode)
>     {
>       bool clone_ok = clone_file (dest_desc, source_desc) == 0;
>       if (clone_ok || x->reflink_mode == REFLINK_ALWAYS)
>         {
>           if (!clone_ok)
>             {
>               error (0, errno, _("failed to clone %s"), quote (dst_name));
>               return_val = false;
>               goto close_src_and_dst_desc;
>             }
>           data_copy_required = false;
>         }
>     }
> 
> Now, I think you're right.
> Perhaps, with --attributes-only cp should not even attempt
> the reflink copy, like this:

That's what I would expect.

> diff --git a/src/copy.c b/src/copy.c
> index df31592..f49722f 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -622,7 +622,7 @@ copy_reg (char const *src_name, char const *dst_name,
>        goto close_src_and_dst_desc;
>      }
> 
> -  if (x->reflink_mode)
> +  if (data_copy_required && x->reflink_mode)
>      {
>        bool clone_ok = clone_file (dest_desc, source_desc) == 0;
>        if (clone_ok || x->reflink_mode == REFLINK_ALWAYS)
> 
> Writing that makes me think that the combination
> of --attributes-only and --reflink=ANYTHING (or --reflink)
> should elicit an error.

This should be warning, to allow something like "alias cp=cp --reflink=never"
to work with --attributes-only. When --attributes-only is specified, all
modifiers for data copying (--sparse, --reflink) should be ignored and
might elicit a warning. A combination of --attributes-only with --link or
--symbolic-link could result in an error or be treated the same as --sparse
and --reflink.

I did not check how this is currently implemented, so no review of the
patch above...

Erik
-- 
Thanks to the virtue of me personally not caring one whit about
virtualization, I can stand back and just watch the fireworks.
                        -- Linus Torvalds