[libvirt] [PATCH] fix vol-create when target allocation=0
Regression is introduced by commit e30297b0. After that, it will report "Cannot fill file" error with following xml file: <volume> <name>virtinst-vmlinuz-xen.tP1NHh</name> <capacity>4343792</capacity> <allocation>0</allocation> <target> <format type="raw"/> <nocow/> </target> </volume> Because of this, installing xen pv guest with virt-manager fails. Reason is: posix_fallocate(int fd, off_t offset, off_t len) will return EINVAL when len is equal to 0. So, this patch adds a check before calling safezero(). Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index db49739..2a265af 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -455,7 +455,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, pos = inputvol->target.capacity - remain; } - if (need_alloc) { + if (need_alloc && (vol->target.allocation > pos)) { if (safezero(fd, pos, vol->target.allocation - pos) < 0) { ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), -- 2.1.4
On Mon, Aug 24, 2015 at 01:50:41PM +0800, Chunyan Liu wrote:
Regression is introduced by commit e30297b0. After that, it will report "Cannot fill file" error with following xml file: <volume> <name>virtinst-vmlinuz-xen.tP1NHh</name> <capacity>4343792</capacity> <allocation>0</allocation> <target> <format type="raw"/> <nocow/> </target> </volume> Because of this, installing xen pv guest with virt-manager fails.
Reason is: posix_fallocate(int fd, off_t offset, off_t len) will return EINVAL when len is equal to 0. So, this patch adds a check before calling safezero().
Oh, god that you've found out the root cause, although the same patch is already on the list: https://www.redhat.com/archives/libvir-list/2015-August/msg00808.html Although it would be nice if it had the root cause (or at least the causing commit) in its commit message as well. Cc'ing the author of the previous patch in case he hasn't pushed yet. Martin
While a zero allocation in safezero should be fine it isn't when we use posix_fallocate which returns EINVAL on a zero allocation. While we could skip the zero allocation in safezero_posix_fallocate it's an optimization to do it for all allocations. This fixes vm installation via virtinst for me which otherwise aborts like: Starting install... Retrieving file linux... | 5.9 MB 00:01 ... Retrieving file initrd.gz... | 29 MB 00:07 ... ERROR Couldn't create storage volume 'virtinst-linux.sBgds4': 'cannot fill file '/var/lib/libvirt/boot/virtinst-linux.sBgds4': Invalid argument' The error was introduced by e30297b0 as spotted by Chunyan Liu --- Only the commit message got adjusted. src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index db49739..0418473 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -455,7 +455,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, pos = inputvol->target.capacity - remain; } - if (need_alloc) { + if (need_alloc && (vol->target.allocation - pos > 0)) { if (safezero(fd, pos, vol->target.allocation - pos) < 0) { ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), -- 2.1.4
On Mon, Aug 24, 2015 at 12:55:22PM +0200, Guido Günther wrote:
While a zero allocation in safezero should be fine it isn't when we use posix_fallocate which returns EINVAL on a zero allocation.
While we could skip the zero allocation in safezero_posix_fallocate it's an optimization to do it for all allocations.
This fixes vm installation via virtinst for me which otherwise aborts like:
Starting install... Retrieving file linux... | 5.9 MB 00:01 ... Retrieving file initrd.gz... | 29 MB 00:07 ... ERROR Couldn't create storage volume 'virtinst-linux.sBgds4': 'cannot fill file '/var/lib/libvirt/boot/virtinst-linux.sBgds4': Invalid argument'
The error was introduced by e30297b0 as spotted by Chunyan Liu --- Only the commit message got adjusted.
ACK, that's a possible one.
src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index db49739..0418473 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -455,7 +455,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, pos = inputvol->target.capacity - remain; }
- if (need_alloc) { + if (need_alloc && (vol->target.allocation - pos > 0)) { if (safezero(fd, pos, vol->target.allocation - pos) < 0) { ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), -- 2.1.4
Hi, On Mon, Aug 24, 2015 at 01:27:42PM +0200, Martin Kletzander wrote:
On Mon, Aug 24, 2015 at 12:55:22PM +0200, Guido Günther wrote:
While a zero allocation in safezero should be fine it isn't when we use posix_fallocate which returns EINVAL on a zero allocation.
While we could skip the zero allocation in safezero_posix_fallocate it's an optimization to do it for all allocations.
This fixes vm installation via virtinst for me which otherwise aborts like:
Starting install... Retrieving file linux... | 5.9 MB 00:01 ... Retrieving file initrd.gz... | 29 MB 00:07 ... ERROR Couldn't create storage volume 'virtinst-linux.sBgds4': 'cannot fill file '/var/lib/libvirt/boot/virtinst-linux.sBgds4': Invalid argument'
The error was introduced by e30297b0 as spotted by Chunyan Liu --- Only the commit message got adjusted.
ACK, that's a possible one.
Pushed now. Thanks! -- Guido
src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index db49739..0418473 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -455,7 +455,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, pos = inputvol->target.capacity - remain; }
- if (need_alloc) { + if (need_alloc && (vol->target.allocation - pos > 0)) { if (safezero(fd, pos, vol->target.allocation - pos) < 0) { ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), -- 2.1.4
On 8/24/2015 at 05:47 PM, in message <20150824094707.GD22373@wheatley>, Martin Kletzander <mkletzan@redhat.com> wrote: On Mon, Aug 24, 2015 at 01:50:41PM +0800, Chunyan Liu wrote: Regression is introduced by commit e30297b0. After that, it will report "Cannot fill file" error with following xml file: <volume> <name>virtinst-vmlinuz-xen.tP1NHh</name> <capacity>4343792</capacity> <allocation>0</allocation> <target> <format type="raw"/> <nocow/> </target> </volume> Because of this, installing xen pv guest with virt-manager fails.
Reason is: posix_fallocate(int fd, off_t offset, off_t len) will return EINVAL when len is equal to 0. So, this patch adds a check before calling safezero().
Oh, god that you've found out the root cause, although the same patch is already on the list:
https://www.redhat.com/archives/libvir-list/2015-August/msg00808.html
Oh yes, it's the same patch. Sorry for not reading the message in time. Really coincidental sending same patch from different person one after another :) Chunyan
Although it would be nice if it had the root cause (or at least the causing commit) in its commit message as well. Cc'ing the author of the previous patch in case he hasn't pushed yet.
Martin
participants (4)
-
Chun Yan Liu -
Chunyan Liu
-
Guido Günther -
Martin Kletzander