10:46 a.m.
This fixes the bug of treating volume allocation as an offset
from the start of the file instead of a total of allocated bytes.
The mysterious error message:
"invalid argument: can't shrink capacity below existing allocation"
is not addressed by this series
Ján Tomko (3):
use virFileAllocate in virStorageFileResize
virStorageFileResize: fallocate the whole capacity
Shrink volume even with ALLOCATE flag
src/storage/storage_util.c | 3 +--
src/util/virfile.c | 11 +++++++++++
src/util/virfile.h | 2 ++
src/util/virstoragefile.c | 33 ++++++++++++++++-----------------
src/util/virstoragefile.h | 1 -
5 files changed, 30 insertions(+), 20 deletions(-)
--
2.13.0
10:46 a.m.
New subject: [libvirt] [PATCH 1/3] use virFileAllocate in virStorageFileResize
Introduce a new function virFileAllocate that will call the
non-destructive variants of safezero, essentially reverting
my commit 1390c268
safezero: fall back to writing zeroes even when resizing
back to the state as of commit 18f0316
virstoragefile: Have virStorageFileResize use safezero
This means that _ALLOCATE flag will no longer work on platforms
without the allocate syscalls, but it will not overwrite data
either.
---
src/util/virfile.c | 11 +++++++++++
src/util/virfile.h | 2 ++
src/util/virstoragefile.c | 15 ++++++++++-----
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index a03a23fab..7ca60052d 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1216,6 +1216,17 @@ int safezero(int fd, off_t offset, off_t len)
return safezero_slow(fd, offset, len);
}
+int virFileAllocate(int fd, off_t offset, off_t len)
+{
+ int ret;
+
+ ret = safezero_posix_fallocate(fd, offset, len);
+ if (ret != -2)
+ return ret;
+
+ return safezero_sys_fallocate(fd, offset, len);
+}
+
#if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
/* search /proc/mounts for mount point of *type; return pointer to
* malloc'ed string of the path if found, otherwise return NULL
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 57ceb8072..21fb41b70 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -44,6 +44,8 @@ ssize_t safewrite(int fd, const void *buf, size_t count)
ATTRIBUTE_RETURN_CHECK;
int safezero(int fd, off_t offset, off_t len)
ATTRIBUTE_RETURN_CHECK;
+int virFileAllocate(int fd, off_t offset, off_t len)
+ ATTRIBUTE_RETURN_CHECK;
/* Don't call these directly - use the macros below */
int virFileClose(int *fdptr, virFileCloseFlags flags)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 484a5c806..b3da0a452 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1320,7 +1320,7 @@ virStorageFileResize(const char *path,
{
int fd = -1;
int ret = -1;
- int rc ATTRIBUTE_UNUSED;
+ int rc;
off_t offset ATTRIBUTE_UNUSED;
off_t len ATTRIBUTE_UNUSED;
@@ -1333,10 +1333,15 @@ virStorageFileResize(const char *path,
}
if (pre_allocate) {
- if (safezero(fd, offset, len) != 0) {
- virReportSystemError(errno,
- _("Failed to pre-allocate space for "
- "file '%s'"), path);
+ if ((rc = virFileAllocate(fd, offset, len)) != 0) {
+ if (rc == -2) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("preallocate is not supported on this
platform"));
+ } else {
+ virReportSystemError(errno,
+ _("Failed to pre-allocate space for "
+ "file '%s'"), path);
+ }
goto cleanup;
}
} else {
--
2.13.0
4:22 p.m.
New subject: [libvirt] [PATCH 1/3] use virFileAllocate in virStorageFileResize
On 09/25/2017 11:46 AM, Ján Tomko wrote:
Introduce a new function virFileAllocate that will call the
non-destructive variants of safezero, essentially reverting
my commit 1390c268
safezero: fall back to writing zeroes even when resizing
back to the state as of commit 18f0316
virstoragefile: Have virStorageFileResize use safezero
This means that _ALLOCATE flag will no longer work on platforms
without the allocate syscalls, but it will not overwrite data
either.
---
src/util/virfile.c | 11 +++++++++++
src/util/virfile.h | 2 ++
src/util/virstoragefile.c | 15 ++++++++++-----
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index a03a23fab..7ca60052d 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1216,6 +1216,17 @@ int safezero(int fd, off_t offset, off_t len)
return safezero_slow(fd, offset, len);
}
Two blank lines between functions...
+int virFileAllocate(int fd, off_t offset, off_t len)
int
virFileAllocation(int fd,
off_t offset,
off_t len)
Is the format more recently being used.
+{
+ int ret;
+
+ ret = safezero_posix_fallocate(fd, offset, len);
+ if (ret != -2)
+ return ret;
+
+ return safezero_sys_fallocate(fd, offset, len);
+}
+
Extra blank line here too.
> #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
> /* search /proc/mounts for mount point of *type; return pointer to
> * malloc'ed string of the path if found, otherwise return NULL
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 57ceb8072..21fb41b70 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -44,6 +44,8 @@ ssize_t safewrite(int fd, const void *buf, size_t count)
> ATTRIBUTE_RETURN_CHECK;
> int safezero(int fd, off_t offset, off_t len)
> ATTRIBUTE_RETURN_CHECK;
+int virFileAllocate(int fd, off_t offset, off_t len)
> +
ATTRIBUTE_RETURN_CHECK;
>
> /* Don't call these directly - use the macros below */
> int virFileClose(int *fdptr, virFileCloseFlags flags)
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 484a5c806..b3da0a452 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1320,7 +1320,7 @@ virStorageFileResize(const char *path,
> {
> int fd = -1;
> int ret = -1;
> - int rc ATTRIBUTE_UNUSED;
> + int rc;
> off_t offset ATTRIBUTE_UNUSED;
> off_t len ATTRIBUTE_UNUSED;
One would think the ATTRIBUTE_UNUSED are no longer necessary since
18f0316 too - what was I thinking then? But they're removed on the next
patch, so no big deal I guess, <sigh>.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
@@ -1333,10 +1333,15 @@ virStorageFileResize(const char *path,
}
if (pre_allocate) {
- if (safezero(fd, offset, len) != 0) {
- virReportSystemError(errno,
- _("Failed to pre-allocate space for "
- "file '%s'"), path);
+ if ((rc = virFileAllocate(fd, offset, len)) != 0) {
+ if (rc == -2) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("preallocate is not supported on this
platform"));
+ } else {
+ virReportSystemError(errno,
+ _("Failed to pre-allocate space for "
+ "file '%s'"), path);
+ }
goto cleanup;
}
} else {
10:46 a.m.
New subject: [libvirt] [PATCH 2/3] virStorageFileResize: fallocate the whole capacity
We have been trying to implement the ALLOCATE flag to mean
"the volume should be fully allocated after the resize".
Since commit b0579ed9 we do not allocate from the existing
capacity, but from the existing allocation value.
However this value is a total of all the allocated bytes,
not an offset.
For a sparsely allocated file:
$ perl -e 'print "x"x8192;' > vol1
$ fallocate -p -o 0 -l 4096 vol1
Treating allocation as an offset would result in an incompletely
allocated file:
$ virsh vol-resize vol1 --pool default 16384 --allocate
Capacity: 16.00 KiB
Allocation: 12.00 KiB
Call fallocate from zero on the whole requested capacity to fully
allocate the file.
---
src/storage/storage_util.c | 3 +--
src/util/virstoragefile.c | 8 +-------
src/util/virstoragefile.h | 1 -
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 07dba2222..b94b3f397 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -2329,8 +2329,7 @@ virStorageBackendVolResizeLocal(virConnectPtr conn
ATTRIBUTE_UNUSED,
bool pre_allocate = flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE;
if (vol->target.format == VIR_STORAGE_FILE_RAW) {
- return virStorageFileResize(vol->target.path, capacity,
- vol->target.allocation, pre_allocate);
+ return virStorageFileResize(vol->target.path, capacity, pre_allocate);
} else if (vol->target.format == VIR_STORAGE_FILE_PLOOP) {
return storagePloopResize(vol, capacity);
} else {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index b3da0a452..5df1ea0b8 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1315,17 +1315,11 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
int
virStorageFileResize(const char *path,
unsigned long long capacity,
- unsigned long long orig_capacity,
bool pre_allocate)
{
int fd = -1;
int ret = -1;
int rc;
- off_t offset ATTRIBUTE_UNUSED;
- off_t len ATTRIBUTE_UNUSED;
-
- offset = orig_capacity;
- len = capacity - orig_capacity;
if ((fd = open(path, O_RDWR)) < 0) {
virReportSystemError(errno, _("Unable to open '%s'"), path);
@@ -1333,7 +1327,7 @@ virStorageFileResize(const char *path,
}
if (pre_allocate) {
- if ((rc = virFileAllocate(fd, offset, len)) != 0) {
+ if ((rc = virFileAllocate(fd, 0, capacity)) != 0) {
if (rc == -2) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("preallocate is not supported on this
platform"));
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index f7e897f25..1eb1c6471 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -328,7 +328,6 @@ virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr
chain,
int virStorageFileResize(const char *path,
unsigned long long capacity,
- unsigned long long orig_capacity,
bool pre_allocate);
int virStorageFileIsClusterFS(const char *path);
--
2.13.0
5:44 p.m.
New subject: [libvirt] [PATCH 2/3] virStorageFileResize: fallocate the whole capacity
On 09/25/2017 11:46 AM, Ján Tomko wrote:
We have been trying to implement the ALLOCATE flag to mean
Is this the colloquial "we"? ;-)
"the volume should be fully allocated after the resize".
commit id 'aa2a4cff' added support for ALLOCATE, but it seems that code
was wrong, then?
The docs are somewhat vague regarding the ALLOCATE flag:
virStorageVolResize:
...
* Normally, the operation will attempt to affect capacity with a minimum
* impact on allocation (that is, the default operation favors a sparse
* resize). If @flags contains VIR_STORAGE_VOL_RESIZE_ALLOCATE, then the
* operation will ensure that allocation is sufficient for the new
* capacity; this may make the operation take noticeably longer.
and:
VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */
What really isn't clear is whether the new allocation wants to preserve
some sort of sparseness factor as well. IOW: If you have a sparse file
before you start, then shouldn't you have a sparse file when you're done?
Since commit b0579ed9 we do not allocate from the existing
capacity, but from the existing allocation value.
However this value is a total of all the allocated bytes,
not an offset.
For a sparsely allocated file:
$ perl -e 'print "x"x8192;' > vol1
$ fallocate -p -o 0 -l 4096 vol1
Treating allocation as an offset would result in an incompletely
allocated file:
$ virsh vol-resize vol1 --pool default 16384 --allocate
Capacity: 16.00 KiB
Allocation: 12.00 KiB
Yep, would have expected an allocation of 16KiB here...
Call fallocate from zero on the whole requested capacity to fully
allocate the file.
FWIW: complete output/example because I'm a visual learner...
Before:
$ perl -e 'print "x"x8192;' > /path/to/default/vol1
$ virsh pool-refresh default
$ virsh vol-info default vol1
Capacity: 8.00 KiB
Allocation: 8.00 KiB
$ fallocate -p -o 0 -l 4096 /path/to/default/vol1
$ virsh vol-info vol1 default
Capacity: 8.00 KiB
Allocation: 4.00 KiB
$ virsh vol-resize vol1 --pool default 16384 --allocate
$ virsh vol-info vol1 default
Capacity: 16.00 KiB
Allocation: 12.00 KiB
After the patch:
Only the vol-resize output changes to:
Capacity: 16.00 KiB
Allocation: 16.00 KiB
Which would seem to be correct. Still I wonder if the initial changes
got things "wrong" by trying to be cute and adding even though DELTA is
dealt with much earlier.
The question being - should the original code adjusted both capacity and
allocation getting the difference between the two as the additional
capacity in order to preserve whatever sparseness may have existed
previously or in this case a capacity=24KiB and Allocation=20KiB. Oh
well we may never know...
---
src/storage/storage_util.c | 3 +--
src/util/virstoragefile.c | 8 +-------
src/util/virstoragefile.h | 1 -
3 files changed, 2 insertions(+), 10 deletions(-)
I guess it's a long way of me giving:
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
But I needed to convince myself first... and if you feel the need to
alter the commit message a bit to pull in more details that's even
better (for me), but I suppose the current one works too.
John
7:55 a.m.
New subject: [libvirt] [PATCH 2/3] virStorageFileResize: fallocate the whole capacity
On Tue, Sep 26, 2017 at 06:44:55PM -0400, John Ferlan wrote:
On 09/25/2017 11:46 AM, Ján Tomko wrote:
> We have been trying to implement the ALLOCATE flag to mean
Is this the colloquial "we"? ;-)
We believe so.
> "the volume should be fully allocated after the
resize".
commit id 'aa2a4cff' added support for ALLOCATE, but it seems that code
was wrong, then?
The docs are somewhat vague regarding the ALLOCATE flag:
virStorageVolResize:
...
* Normally, the operation will attempt to affect capacity with a minimum
* impact on allocation (that is, the default operation favors a sparse
* resize). If @flags contains VIR_STORAGE_VOL_RESIZE_ALLOCATE, then the
* operation will ensure that allocation is sufficient for the new
* capacity; this may make the operation take noticeably longer.
and:
VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */
What really isn't clear is whether the new allocation wants to preserve
some sort of sparseness factor as well. IOW: If you have a sparse file
before you start, then shouldn't you have a sparse file when you're done?
Mixing sparseness in the original part and allocation in the new part
does not seem that useful and it does not "ensure that the allocation is
sufficient for the new capacity".
>
> Since commit b0579ed9 we do not allocate from the existing
> capacity, but from the existing allocation value.
> However this value is a total of all the allocated bytes,
> not an offset.
>
> For a sparsely allocated file:
> $ perl -e 'print "x"x8192;' > vol1
> $ fallocate -p -o 0 -l 4096 vol1
>
> Treating allocation as an offset would result in an incompletely
> allocated file:
> $ virsh vol-resize vol1 --pool default 16384 --allocate
> Capacity: 16.00 KiB
> Allocation: 12.00 KiB
Yep, would have expected an allocation of 16KiB here...
>
> Call fallocate from zero on the whole requested capacity to fully
> allocate the file.
FWIW: complete output/example because I'm a visual learner...
Before:
$ perl -e 'print "x"x8192;' > /path/to/default/vol1
$ virsh pool-refresh default
$ virsh vol-info default vol1
Capacity: 8.00 KiB
Allocation: 8.00 KiB
$ fallocate -p -o 0 -l 4096 /path/to/default/vol1
$ virsh vol-info vol1 default
Capacity: 8.00 KiB
Allocation: 4.00 KiB
$ virsh vol-resize vol1 --pool default 16384 --allocate
$ virsh vol-info vol1 default
Capacity: 16.00 KiB
Allocation: 12.00 KiB
After the patch:
Only the vol-resize output changes to:
Capacity: 16.00 KiB
Allocation: 16.00 KiB
Which would seem to be correct. Still I wonder if the initial changes
got things "wrong" by trying to be cute and adding even though DELTA is
dealt with much earlier.
The question being - should the original code adjusted both capacity and
allocation getting the difference between the two as the additional
capacity in order to preserve whatever sparseness may have existed
previously or in this case a capacity=24KiB and Allocation=20KiB. Oh
well we may never know...
> ---
> src/storage/storage_util.c | 3 +--
> src/util/virstoragefile.c | 8 +-------
> src/util/virstoragefile.h | 1 -
> 3 files changed, 2 insertions(+), 10 deletions(-)
>
I guess it's a long way of me giving:
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
But I needed to convince myself first... and if you feel the need to
alter the commit message a bit to pull in more details that's even
better (for me), but I suppose the current one works too.
I have added some more examples to the commit message and pushed the
series
Thanks!
Jan
John
10:46 a.m.
New subject: [libvirt] [PATCH 3/3] Shrink volume even with ALLOCATE flag
Calling fallocate on the new (smaller) capacity ensures
that the whole file is allocated, but it does not reduce
the file size.
Also call ftruncate after fallocate.
https://bugzilla.redhat.com/show_bug.cgi?id=1366446
---
src/util/virstoragefile.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5df1ea0b8..80a33b1a6 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1338,12 +1338,12 @@ virStorageFileResize(const char *path,
}
goto cleanup;
}
- } else {
- if (ftruncate(fd, capacity) < 0) {
- virReportSystemError(errno,
- _("Failed to truncate file '%s'"),
path);
- goto cleanup;
- }
+ }
+
+ if (ftruncate(fd, capacity) < 0) {
+ virReportSystemError(errno,
+ _("Failed to truncate file '%s'"), path);
+ goto cleanup;
}
if (VIR_CLOSE(fd) < 0) {
--
2.13.0
6:37 p.m.
New subject: [libvirt] [PATCH 3/3] Shrink volume even with ALLOCATE flag
On 09/25/2017 11:46 AM, Ján Tomko wrote:
Calling fallocate on the new (smaller) capacity ensures
that the whole file is allocated, but it does not reduce
the file size.
Also call ftruncate after fallocate.
https://bugzilla.redhat.com/show_bug.cgi?id=1366446
---
src/util/virstoragefile.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2613
days inactive
2615
days old
7 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko