5:06 a.m.
This patch changes zerobuf variable from array to VIR_ALLOC_N().
Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp>
---
src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++---------
1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 2eede74..3a22da4 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -36,6 +36,10 @@
#include <sys/stat.h>
#include <sys/param.h>
#include <dirent.h>
+#ifdef __linux__
+#include <sys/ioctl.h>
+#include <linux/fs.h>
+#endif
#if HAVE_SELINUX
# include <selinux/selinux.h>
@@ -108,6 +112,9 @@ enum {
TOOL_QCOW_CREATE,
};
+#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024)
+#define WRITE_BLOCK_SIZE_DEFAULT (512)
+
static int ATTRIBUTE_NONNULL (2)
virStorageBackendCopyToFD(virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
@@ -119,8 +126,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
int amtread = -1;
int ret = 0;
unsigned long long remain;
- size_t bytes = 1024 * 1024;
- char zerobuf[512];
+ size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
+ size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ int interval;
+ char *zerobuf;
char *buf = NULL;
if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) {
@@ -131,9 +140,19 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
goto cleanup;
}
- bzero(&zerobuf, sizeof(zerobuf));
+#ifdef __linux__
+ if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
+ wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ }
+#endif
+
+ if (VIR_ALLOC_N(zerobuf, wbytes) < 0) {
+ ret = -errno;
+ virReportOOMError();
+ goto cleanup;
+ }
- if (VIR_ALLOC_N(buf, bytes) < 0) {
+ if (VIR_ALLOC_N(buf, rbytes) < 0) {
ret = -errno;
virReportOOMError();
goto cleanup;
@@ -144,10 +163,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
while (amtread != 0) {
int amtleft;
- if (remain < bytes)
- bytes = remain;
+ if (remain < rbytes)
+ rbytes = remain;
- if ((amtread = saferead(inputfd, buf, bytes)) < 0) {
+ if ((amtread = saferead(inputfd, buf, rbytes)) < 0) {
ret = -errno;
virReportSystemError(errno,
_("failed reading from file '%s'"),
@@ -160,7 +179,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
* blocks */
amtleft = amtread;
do {
- int interval = ((512 > amtleft) ? amtleft : 512);
+ interval = ((wbytes > amtleft) ? amtleft : wbytes);
int offset = amtread - amtleft;
if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) {
@@ -179,7 +198,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
goto cleanup;
}
- } while ((amtleft -= 512) > 0);
+ } while ((amtleft -= interval) > 0);
}
if (VIR_CLOSE(inputfd) < 0) {
@@ -196,6 +215,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
cleanup:
VIR_FORCE_CLOSE(inputfd);
+ VIR_FREE(zerobuf);
VIR_FREE(buf);
return ret;
--
Minoru Usui <usui(a)mxm.nes.nec.co.jp>
10:04 p.m.
On 03/08/2011 04:06 AM, Minoru Usui wrote:
This patch changes zerobuf variable from array to VIR_ALLOC_N().
Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp>
Thanks for contributing! This looks like your first patch in, so I
updated AUTHORS; let me know if I need to adjust any spelling.
---
src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++---------
1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 2eede74..3a22da4 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -36,6 +36,10 @@
#include <sys/stat.h>
#include <sys/param.h>
#include <dirent.h>
+#ifdef __linux__
+#include <sys/ioctl.h>
+#include <linux/fs.h>
+#endif
This needs indentation tweaking to keep 'make syntax-check' happy when
cppi is installed.
+#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024)
+#define WRITE_BLOCK_SIZE_DEFAULT (512)
Technically, you don't need () around a single integer constant, but it
didn't hurt to leave them in. Why isn't this 4k, though?
+#ifdef __linux__
+ if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
+ wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ }
+#endif
What's wrong with fstat(fd) and using st.st_blksize on all other
platforms where the ioctl doesn't exist? That may still be too small on
some platforms, but it's more likely to be write than hard-coding things
at 512 (for example, on Cygwin, st.st_blksize is 64k, which is indeed
the optimal size for NTFS drives). Also, I don't know of any file
system that supports sparse files with holes smaller than st_blksize.
Here's what I squashed in before pushing:
diff --git i/AUTHORS w/AUTHORS
index c0c1780..0e56468 100644
--- i/AUTHORS
+++ w/AUTHORS
diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c
index b3eba10..fc08c68 100644
--- i/src/storage/storage_backend.c
+++ w/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
/*
* storage_backend.c: internal storage driver backend contract
*
- * Copyright (C) 2007-2010 Red Hat, Inc.
+ * Copyright (C) 2007-2011 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -37,8 +37,8 @@
#include <sys/param.h>
#include <dirent.h>
#ifdef __linux__
-#include <sys/ioctl.h>
-#include <linux/fs.h>
+# include <sys/ioctl.h>
+# include <linux/fs.h>
#endif
#if HAVE_SELINUX
@@ -113,7 +113,7 @@ enum {
};
#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024)
-#define WRITE_BLOCK_SIZE_DEFAULT (512)
+#define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024)
static int ATTRIBUTE_NONNULL (2)
virStorageBackendCopyToFD(virStorageVolDefPtr vol,
@@ -127,10 +127,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
int ret = 0;
unsigned long long remain;
size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
- size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ size_t wbytes = 0;
int interval;
char *zerobuf;
char *buf = NULL;
+ struct stat st;
if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) {
ret = -errno;
@@ -142,9 +143,13 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
#ifdef __linux__
if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
- wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ wbytes = 0;
}
#endif
+ if ((wbytes == 0) && fstat(fd, &st) == 0)
+ wbytes = st.st_blksize;
+ if (wbytes < WRITE_BLOCK_SIZE_DEFAULT)
+ wbytes = WRITE_BLOCK_SIZE_DEFAULT;
if (VIR_ALLOC_N(zerobuf, wbytes) < 0) {
ret = -errno;
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:30 p.m.
Hi, Eric.
Thank you for merging my patch.
On Mon, 14 Mar 2011 21:04:26 -0600
Eric Blake <eblake(a)redhat.com> wrote:
On 03/08/2011 04:06 AM, Minoru Usui wrote:
> This patch changes zerobuf variable from array to VIR_ALLOC_N().
>
> Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp>
Thanks for contributing! This looks like your first patch in, so I
updated AUTHORS; let me know if I need to adjust any spelling.
Thanks for updating AUTHORS.
Above spelling of Signed-off-by is correct.
But..., I sent another patch to you and libvirt-devel before.
Please check it, if you have any time.
http://www.mail-archive.com/libvir-list@redhat.com/msg32801.html
> ---
> src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++---------
> 1 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 2eede74..3a22da4 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -36,6 +36,10 @@
> #include <sys/stat.h>
> #include <sys/param.h>
> #include <dirent.h>
> +#ifdef __linux__
> +#include <sys/ioctl.h>
> +#include <linux/fs.h>
> +#endif
This needs indentation tweaking to keep 'make syntax-check' happy when
cppi is installed.
Thank you for telling me about it.
I agree.
> +#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024)
> +#define WRITE_BLOCK_SIZE_DEFAULT (512)
Technically, you don't need () around a single integer constant, but it
didn't hurt to leave them in. Why isn't this 4k, though?
I don't know 4k is appropriate value for all platform.
So I don't change 512.
If 4k is best, I think default value changes from 512 to 4k, too.
> +#ifdef __linux__
> + if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
> + wbytes = WRITE_BLOCK_SIZE_DEFAULT;
> + }
> +#endif
What's wrong with fstat(fd) and using st.st_blksize on all other
platforms where the ioctl doesn't exist? That may still be too small on
some platforms, but it's more likely to be write than hard-coding things
at 512 (for example, on Cygwin, st.st_blksize is 64k, which is indeed
the optimal size for NTFS drives). Also, I don't know of any file
system that supports sparse files with holes smaller than st_blksize.
I agree with you.
Your code looks good for me.
Here's what I squashed in before pushing:
diff --git i/AUTHORS w/AUTHORS
index c0c1780..0e56468 100644
--- i/AUTHORS
+++ w/AUTHORS
diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c
index b3eba10..fc08c68 100644
--- i/src/storage/storage_backend.c
+++ w/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
/*
* storage_backend.c: internal storage driver backend contract
*
- * Copyright (C) 2007-2010 Red Hat, Inc.
+ * Copyright (C) 2007-2011 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -37,8 +37,8 @@
#include <sys/param.h>
#include <dirent.h>
#ifdef __linux__
-#include <sys/ioctl.h>
-#include <linux/fs.h>
+# include <sys/ioctl.h>
+# include <linux/fs.h>
#endif
#if HAVE_SELINUX
@@ -113,7 +113,7 @@ enum {
};
#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024)
-#define WRITE_BLOCK_SIZE_DEFAULT (512)
+#define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024)
static int ATTRIBUTE_NONNULL (2)
virStorageBackendCopyToFD(virStorageVolDefPtr vol,
@@ -127,10 +127,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
int ret = 0;
unsigned long long remain;
size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
- size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ size_t wbytes = 0;
int interval;
char *zerobuf;
char *buf = NULL;
+ struct stat st;
if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) {
ret = -errno;
@@ -142,9 +143,13 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
#ifdef __linux__
if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
- wbytes = WRITE_BLOCK_SIZE_DEFAULT;
+ wbytes = 0;
}
#endif
+ if ((wbytes == 0) && fstat(fd, &st) == 0)
+ wbytes = st.st_blksize;
+ if (wbytes < WRITE_BLOCK_SIZE_DEFAULT)
+ wbytes = WRITE_BLOCK_SIZE_DEFAULT;
if (VIR_ALLOC_N(zerobuf, wbytes) < 0) {
ret = -errno;
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
Minoru Usui <usui(a)mxm.nes.nec.co.jp>
5002
days inactive
5009
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Minoru Usui