Re: BTRFS file clone support for cp

[Pádraig Brady]

Giuseppe Scrivano wrote:
> Hi,
> 
> I cleaned a bit the Pádraig's example in a new test case.

tests, great!
comments below.

> The second patch fixes a problem that I introduced with the commit
> e81c4d88c2fce526c02693d539e22c7468dc452b.

I would have posted that patch in the other "tail" thread.
On momentary review it seems fine.


> diff --git a/tests/cp/file-clone b/tests/cp/file-clone

> +require_root_
> +require_sparse_support_

We probably want expensive_ for the moment
since we're moving 500M of data around

> +
> +fail=0
> +
> +mkfs.btrfs --version || skip_test_ "btrfs userland tools not installed"

Here, I'd add the comment:

# 300MB seems to be the minimum size for a btrfs with default parameters.
# FIXME: use `truncate --allocate` when it becomes available, which
# may allow unmarking this as an expensive test.

> +dd bs=1M count=300 if=/dev/zero of=btrfs.img  || framework_failure

Perhaps we could shrink everything by tweaking some
of the parameters to mkfs.btrfs ?

> +
> +mkfs.btrfs btrfs.img  || framework_failure
> +
> +mkdir btrfs || framework_failure
> +
> +mount -t btrfs -o loop btrfs.img btrfs || framework_failure
> +
> +dd bs=1M count=200 if=/dev/zero of=btrfs/alloc.test || (umount btrfs;
> +                                                       framework_failure)

No need for a subshell above. The following is better I think:

dd bs=1M count=200 if=/dev/zero of=btrfs/alloc.test ||
  { umount btrfs; framework_failure; }

> +# If the file is cloned, only additional space for metadata is required.
> +# Two 200Mb files can be present even if the total file system space is 
> 300Mb.

I usually use big B for Bytes and little b for bits.

cheers,
Pádraig.