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

-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Monday, November 24, 2014 3:09 PM To: Chen, Hanxiao/陈 晗霄 Cc: libvir-list(a)redhat.com Subject: 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(a)cn.fujitsu.com&gt; >--- > 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.