6:08 p.m.
Both of these problems can occur only if a new storage volume is
created by copying it from an existing volume. The first is a problem
if the new volume is longer than the old (the new volume won't be
zero-filled at the end), and the second happens if the new and old
volumes are the same length (fsync wouldn't be called at the end of
writing the new volume) (actually, before the 1st fix is applied, that
problem would exist for *all* new volumes created as copies of
existing volumes).
6:08 p.m.
New subject: [libvirt] [PATCH 1/2] Don't skip zero'ing end of volume file when inputvol is shorter than newvol
A missing set of braces around an error condition caused us to skip
zero'ing out the remainder of a new volume file if the new volume was
longer than the original (the goto was supposed to be taken only in
the case of error, but was always being taken).
---
src/storage/storage_backend.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index c185693..1eb8e33 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -288,9 +288,10 @@ static int createRawFileOpHook(int fd, void *data) {
if (hdata->inputvol) {
int res = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol,
fd, &remain, 1);
- if (res < 0)
+ if (res < 0) {
ret = -res;
goto cleanup;
+ }
}
if (remain) {
--
1.7.1.1
6:15 p.m.
New subject: [libvirt] [PATCH 1/2] Don't skip zero'ing end of volume file when inputvol is shorter than newvol
On 07/19/2010 05:08 PM, Laine Stump wrote:
A missing set of braces around an error condition caused us to skip
zero'ing out the remainder of a new volume file if the new volume was
longer than the original (the goto was supposed to be taken only in
the case of error, but was always being taken).
Ouch. Running a re-indenter would have spotted this, if we didn't have
too many other false-positive reindentations to sift through...
---
src/storage/storage_backend.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index c185693..1eb8e33 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -288,9 +288,10 @@ static int createRawFileOpHook(int fd, void *data) {
if (hdata->inputvol) {
int res = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol,
fd, &remain, 1);
- if (res < 0)
+ if (res < 0) {
ret = -res;
goto cleanup;
+ }
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:28 a.m.
New subject: [libvirt] [PATCH 1/2] Don't skip zero'ing end of volume file when inputvol is shorter than newvol
On 07/19/2010 07:15 PM, Eric Blake wrote:
On 07/19/2010 05:08 PM, Laine Stump wrote:
> A missing set of braces around an error condition caused us to skip
> zero'ing out the remainder of a new volume file if the new volume was
> longer than the original (the goto was supposed to be taken only in
> the case of error, but was always being taken).
Ouch. Running a re-indenter would have spotted this, if we didn't have
too many other false-positive reindentations to sift through...
> ---
> src/storage/storage_backend.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index c185693..1eb8e33 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -288,9 +288,10 @@ static int createRawFileOpHook(int fd, void *data) {
> if (hdata->inputvol) {
> int res = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol,
> fd,&remain, 1);
> - if (res< 0)
> + if (res< 0) {
> ret = -res;
> goto cleanup;
> + }
ACK.
Thanks, pushed.
6:08 p.m.
New subject: [libvirt] [PATCH 2/2] fsync new storage volumes even if new volume was copied.
Originally the storage volume files were opened with O_DSYNC to make
sure they were flushed to disk immediately. It turned out that this
was extremely slow in some cases, so the O_DSYNC was removed in favor
of just calling fsync() after all the data had been written. However,
this call to fsync was inside the block that is executed to zero-fill
the end of the volume file. In cases where the new volume is copied
from an old volume, and they are the same length, this fsync would
never take place.
Now the fsync is *always* done, unless there is an error (in which
case it isn't important, and is most likely inappropriate.
---
src/storage/storage_backend.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 1eb8e33..730ca7b 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -324,12 +324,13 @@ static int createRawFileOpHook(int fd, void *data) {
}
}
- if (fsync(fd) < 0) {
- ret = errno;
- virReportSystemError(errno, _("cannot sync data to file
'%s'"),
- hdata->vol->target.path);
- goto cleanup;
- }
+ }
+
+ if (fsync(fd) < 0) {
+ ret = errno;
+ virReportSystemError(errno, _("cannot sync data to file
'%s'"),
+ hdata->vol->target.path);
+ goto cleanup;
}
cleanup:
--
1.7.1.1
6:16 p.m.
New subject: [libvirt] [PATCH 2/2] fsync new storage volumes even if new volume was copied.
On 07/19/2010 05:08 PM, Laine Stump wrote:
Originally the storage volume files were opened with O_DSYNC to make
sure they were flushed to disk immediately. It turned out that this
was extremely slow in some cases, so the O_DSYNC was removed in favor
of just calling fsync() after all the data had been written. However,
this call to fsync was inside the block that is executed to zero-fill
the end of the volume file. In cases where the new volume is copied
from an old volume, and they are the same length, this fsync would
never take place.
Now the fsync is *always* done, unless there is an error (in which
case it isn't important, and is most likely inappropriate.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:28 a.m.
New subject: [libvirt] [PATCH 2/2] fsync new storage volumes even if new volume was copied.
On 07/19/2010 07:16 PM, Eric Blake wrote:
On 07/19/2010 05:08 PM, Laine Stump wrote:
> Originally the storage volume files were opened with O_DSYNC to make
> sure they were flushed to disk immediately. It turned out that this
> was extremely slow in some cases, so the O_DSYNC was removed in favor
> of just calling fsync() after all the data had been written. However,
> this call to fsync was inside the block that is executed to zero-fill
> the end of the volume file. In cases where the new volume is copied
> from an old volume, and they are the same length, this fsync would
> never take place.
>
> Now the fsync is *always* done, unless there is an error (in which
> case it isn't important, and is most likely inappropriate.
ACK.
Thanks. Pushed.
5243
days inactive
5244
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump