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
