[libvirt] [PATCH RFC] storage: perform btrfs clone if possible
Martin Kletzander
mkletzan at redhat.com
Mon Nov 24 07:09:00 UTC 2014
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>
More information about the libvir-list
mailing list