[libvirt] [PATCH RFC] storage: perform btrfs clone if possible

[Martin Kletzander]

On Mon, Nov 24, 2014 at 02:11:47PM +0800, Chen Hanxiao wrote:
>We already had nocow flags in virStorageSource.
>But when creating RAW file, we don't take advantage
>of clone of btrfs.
>This file introduce btrfs_clone_file function,
>and try to use it when !nocow.
>

I'm not sure we want to do this, but I have nothing against that
either.  So I'll just review the code without any other comments.

>Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
>---
> src/storage/storage_backend.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
>diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>index 98720f6..f5ea34c 100644
>--- a/src/storage/storage_backend.c
>+++ b/src/storage/storage_backend.c
>@@ -156,6 +156,27 @@ enum {
> #define READ_BLOCK_SIZE_DEFAULT  (1024 * 1024)
> #define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024)
>
>+/*
>+ * Perform the O(1) btrfs clone operation, if possible.
>+ * Upon success, return 0.  Otherwise, return -1 and set errno.
>+ */
>+static inline int
>+btrfs_clone_file(int dest_fd, int src_fd)

All the functions in this file use camelCase, this one might too.

>+{
>+#ifdef __linux__
>+# undef BTRFS_IOCTL_MAGICi

s/i$// ?

>+# define BTRFS_IOCTL_MAGIC 0x94
>+# undef BTRFS_IOC_CLONE
>+# define BTRFS_IOC_CLONE _IOW (BTRFS_IOCTL_MAGIC, 9, int)

Are you redefining those just in case they are not defined, but the
support exists?  I'm always afraid of creating incompatibilities and
would prefer #if for that and if anything is not defined, just don't
use it.

>+    return ioctl(dest_fd, BTRFS_IOC_CLONE, src_fd);
>+#else
>+    (void) dest_fd;
>+    (void) src_fd;

we use ignore_value(), but you don't need that if you do what's
preferred ...

>+    errno = ENOTSUP;
>+    return -1;
>+#endif
>+}
>+

... we prefer to split the whole definitions for functions to a
working variant and a stub, in that case you can mark unused
parameters in the stub function.  From a subjective point of view,
it's more readable, also (you see right before the definition what you
need for the function to work):

#if defined(__linux__) && defined(BTRFS_IOC_CLONE)

static inline int
btrfs_clone_file(int dest_fd, int src_fd)
{
    return ioctl(dest_fd, BTRFS_IOC_CLONE, src_fd);
}

#else

static inline int
btrfs_clone_file(int dest_fd, int src_fd)
{
    errno = ENOTSUP;
    return -1;
}

#endif

>@@ -200,6 +221,16 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>         goto cleanup;
>     }
>
>+    if (!vol->target.nocow) {
>+        if (btrfs_clone_file(fd, inputfd) == -1) {
>+            if (errno == ENOTSUP)
>+                VIR_DEBUG("btrfs clone not supported, try another way.");
>+        } else {
>+            VIR_DEBUG("btrfs clone findished.");

s/findished/finished/

As I said, I'm not commenting on whether we want this in or not, so
for that you should wait for someone's response.  I bet there's a
(good) reason behind libvirt not using some lvm/zfs/btrfs features,
but I am too lazy to search for it since it'd be inaccurate anyway.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141124/1b67f632/attachment-0001.sig>