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@xxxxxxxxxxxxxx>
---
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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list