[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]