[libvirt PATCH 0/4] storage: support controlling COW attribute for pool
We already support a "nocow" flag for storage volumes, but this requires extra work by the mgmt app or user when creating images on btrfs. We want to "do the right thing" out of the box for btrfs. We achieve this by changint the storage pool code so that when creating a storage pool we always try to disable COW on btrfs filesystems. We then add an <cow state="yes|no"/> feature in the pool XML to let apps override the default logic. The COW setting on the pool is inherited by any volumes. The main thing not solved here is that the default directory at /var/lib/libvirt/images is created by the RPM itself, not by a normal "pool-build" command. Fortunately it appears that virt-install will explicitly invoke a storage pool build even if the directory already exists. Daniel P. Berrangé (4): util: add a helper method for controlling the COW flag on btrfs storage: convert to use virFileSetCOW storage: attempt to disable COW by default conf: add control over COW for storage pool directories docs/formatstorage.html.in | 25 +++++++ docs/schemas/storagepool.rng | 30 ++++++++ src/conf/storage_conf.c | 49 +++++++++++++ src/conf/storage_conf.h | 8 +++ src/libvirt_private.syms | 1 + src/storage/storage_util.c | 46 +++++------- src/util/virfile.c | 76 ++++++++++++++++++++ src/util/virfile.h | 3 + tests/storagepoolxml2xmlin/pool-dir-cow.xml | 10 +++ tests/storagepoolxml2xmlout/pool-dir-cow.xml | 15 ++++ tests/storagepoolxml2xmltest.c | 1 + 11 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-cow.xml create mode 100644 tests/storagepoolxml2xmlout/pool-dir-cow.xml -- 2.26.2
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used. The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73b72c9e10..eea31a736d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2125,6 +2125,7 @@ virFileRewrite; virFileRewriteStr; virFileSanitizePath; virFileSetACLs; +virFileSetCOW; virFileSetupDev; virFileSetXAttr; virFileTouch; diff --git a/src/util/virfile.c b/src/util/virfile.c index 213acdbcaa..5b169b3d11 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -71,6 +71,7 @@ # endif # include <sys/ioctl.h> # include <linux/cdrom.h> +# include <linux/fs.h> #endif #if HAVE_LIBATTR @@ -4504,3 +4505,78 @@ virFileDataSync(int fd) return fdatasync(fd); #endif } + + +int +virFileSetCOW(const char *path, + virTristateBool state) +{ +#if __linux__ + int val = 0; + struct statfs buf; + VIR_AUTOCLOSE fd = -1; + + VIR_DEBUG("Setting COW flag on '%s' to '%s'", + path, virTristateBoolTypeToString(state)); + + fd = open(path, O_RDONLY|O_NONBLOCK|O_LARGEFILE); + if (fd < 0) { + virReportSystemError(errno, _("unable to open '%s'"), + path); + return -1; + } + + if (fstatfs(fd, &buf) < 0) { + virReportSystemError(errno, _("unable query filesystem type on '%s'"), + path); + return -1; + } + + if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) { + virReportSystemError(ENOSYS, + _("unable to control COW flag on '%s', not btrfs"), + path); + return -1; + } + return 0; + } + + if (ioctl(fd, FS_IOC_GETFLAGS, &val) < 0) { + virReportSystemError(errno, _("unable get directory flags on '%s'"), + path); + return -1; + } + + VIR_DEBUG("Current flags on '%s' are 0x%x", path, val); + if (state == VIR_TRISTATE_BOOL_YES) { + val &= ~FS_NOCOW_FL; + } else { + val |= FS_NOCOW_FL; + } + + VIR_DEBUG("New flags on '%s' will be 0x%x", path, val); + if (ioctl(fd, FS_IOC_SETFLAGS, &val) < 0) { + int saved_err = errno; + VIR_DEBUG("Failed to set flags on '%s': %s", path, g_strerror(saved_err)); + if (state != VIR_TRISTATE_BOOL_ABSENT) { + virReportSystemError(saved_err, + _("unable control COW flag on '%s'"), + path); + return -1; + } else { + VIR_DEBUG("Ignoring failure to set COW"); + } + } + + return 0; +#else /* ! __linux__ */ + if (state != VIR_TRISTATE_BOOL_ABSENT) { + virReportSystemError(ENOSYS, + _("Unable to set copy-on-write state on '%s' to '%s'"), + path, virTristateBoolTypeToString(state)); + return -1; + } + return 0; +#endif /* ! __linux__ */ +} diff --git a/src/util/virfile.h b/src/util/virfile.h index 7a92364a5c..cb0e586a7d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -374,3 +374,6 @@ int virFileRemoveXAttr(const char *path, G_GNUC_NO_INLINE; int virFileDataSync(int fd); + +int virFileSetCOW(const char *path, + virTristateBool state); -- 2.26.2
On Mon, Jul 20, 2020 at 18:33:19 +0100, Daniel Berrange wrote:
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73b72c9e10..eea31a736d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2125,6 +2125,7 @@ virFileRewrite; virFileRewriteStr; virFileSanitizePath; virFileSetACLs; +virFileSetCOW; virFileSetupDev; virFileSetXAttr; virFileTouch; diff --git a/src/util/virfile.c b/src/util/virfile.c index 213acdbcaa..5b169b3d11 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -71,6 +71,7 @@ # endif # include <sys/ioctl.h> # include <linux/cdrom.h> +# include <linux/fs.h> #endif
#if HAVE_LIBATTR @@ -4504,3 +4505,78 @@ virFileDataSync(int fd) return fdatasync(fd); #endif } + + +int +virFileSetCOW(const char *path, + virTristateBool state) +{ +#if __linux__ + int val = 0; + struct statfs buf; + VIR_AUTOCLOSE fd = -1; + + VIR_DEBUG("Setting COW flag on '%s' to '%s'", + path, virTristateBoolTypeToString(state)); + + fd = open(path, O_RDONLY|O_NONBLOCK|O_LARGEFILE); + if (fd < 0) { + virReportSystemError(errno, _("unable to open '%s'"), + path); + return -1; + } + + if (fstatfs(fd, &buf) < 0) { + virReportSystemError(errno, _("unable query filesystem type on '%s'"), + path); + return -1; + } + + if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) {
Can't we handle the _ABSENT case before even attempting to open the file?
On Thu, Jul 23, 2020 at 02:57:32PM +0200, Peter Krempa wrote:
On Mon, Jul 20, 2020 at 18:33:19 +0100, Daniel Berrange wrote:
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73b72c9e10..eea31a736d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2125,6 +2125,7 @@ virFileRewrite; virFileRewriteStr; virFileSanitizePath; virFileSetACLs; +virFileSetCOW; virFileSetupDev; virFileSetXAttr; virFileTouch; diff --git a/src/util/virfile.c b/src/util/virfile.c index 213acdbcaa..5b169b3d11 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -71,6 +71,7 @@ # endif # include <sys/ioctl.h> # include <linux/cdrom.h> +# include <linux/fs.h> #endif
#if HAVE_LIBATTR @@ -4504,3 +4505,78 @@ virFileDataSync(int fd) return fdatasync(fd); #endif } + + +int +virFileSetCOW(const char *path, + virTristateBool state) +{ +#if __linux__ + int val = 0; + struct statfs buf; + VIR_AUTOCLOSE fd = -1; + + VIR_DEBUG("Setting COW flag on '%s' to '%s'", + path, virTristateBoolTypeToString(state)); + + fd = open(path, O_RDONLY|O_NONBLOCK|O_LARGEFILE); + if (fd < 0) { + virReportSystemError(errno, _("unable to open '%s'"), + path); + return -1; + } + + if (fstatfs(fd, &buf) < 0) { + virReportSystemError(errno, _("unable query filesystem type on '%s'"), + path); + return -1; + } + + if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) {
Can't we handle the _ABSENT case before even attempting to open the file?
This would require us to use statfs() instad of fstatfs() in order to check the super magic. I'm not seeing that improves things. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jul 23, 2020 at 14:00:40 +0100, Daniel Berrange wrote:
On Thu, Jul 23, 2020 at 02:57:32PM +0200, Peter Krempa wrote:
On Mon, Jul 20, 2020 at 18:33:19 +0100, Daniel Berrange wrote:
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+)
[...]
+ if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) {
Can't we handle the _ABSENT case before even attempting to open the file?
This would require us to use statfs() instad of fstatfs() in order to check the super magic. I'm not seeing that improves things.
I definitely agree. But adding a function which does non-obvious things without any comment pointing to the non-obvious behaviour isn't good practice either.
On Thu, Jul 23, 2020 at 03:08:41PM +0200, Peter Krempa wrote:
On Thu, Jul 23, 2020 at 14:00:40 +0100, Daniel Berrange wrote:
On Thu, Jul 23, 2020 at 02:57:32PM +0200, Peter Krempa wrote:
On Mon, Jul 20, 2020 at 18:33:19 +0100, Daniel Berrange wrote:
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+)
[...]
+ if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) {
Can't we handle the _ABSENT case before even attempting to open the file?
This would require us to use statfs() instad of fstatfs() in order to check the super magic. I'm not seeing that improves things.
I definitely agree. But adding a function which does non-obvious things without any comment pointing to the non-obvious behaviour isn't good practice either.
I'll add this /** * virFileSetCow: * @path: file or directory to control the COW flag on * @state: the desired state of the COW flag * * When @state is VIR_TRISTATE_BOOL_ABSENT, some helpful * default logic will be used. Specifically if the filesystem * containing @path is 'btrfs', then it will attempt to * disable the COW flag, but errors will be ignored. For * any other filesystem no change will be made. * * When @state is VIR_TRISTATE_BOOL_YES or VIR_TRISTATE_BOOL_NO, * it will attempt to set the COW flag state to that explicit * value, and always return an error if it fails. Note this * means it will always return error if the filesystem is not * 'btrfs'. */ Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 7/23/20 8:59 AM, Daniel P. Berrangé wrote:
On Thu, Jul 23, 2020 at 03:08:41PM +0200, Peter Krempa wrote:
On Thu, Jul 23, 2020 at 14:00:40 +0100, Daniel Berrange wrote:
On Thu, Jul 23, 2020 at 02:57:32PM +0200, Peter Krempa wrote:
On Mon, Jul 20, 2020 at 18:33:19 +0100, Daniel Berrange wrote:
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+)
[...]
+ if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) {
Can't we handle the _ABSENT case before even attempting to open the file?
This would require us to use statfs() instad of fstatfs() in order to check the super magic. I'm not seeing that improves things.
I definitely agree. But adding a function which does non-obvious things without any comment pointing to the non-obvious behaviour isn't good practice either.
I was reviewing this series Monday before getting distracted with other things and had to read this function a few times to get my head around it.
I'll add this
/** * virFileSetCow: * @path: file or directory to control the COW flag on * @state: the desired state of the COW flag * * When @state is VIR_TRISTATE_BOOL_ABSENT, some helpful * default logic will be used. Specifically if the filesystem * containing @path is 'btrfs', then it will attempt to * disable the COW flag, but errors will be ignored. For * any other filesystem no change will be made. * * When @state is VIR_TRISTATE_BOOL_YES or VIR_TRISTATE_BOOL_NO, * it will attempt to set the COW flag state to that explicit * value, and always return an error if it fails. Note this * means it will always return error if the filesystem is not * 'btrfs'. */
But that definitely helps! Thanks for adding it. Regards, Jim
On Thu, Jul 23, 2020 at 14:57:38 +0200, Peter Krempa wrote:
On Mon, Jul 20, 2020 at 18:33:19 +0100, Daniel Berrange wrote:
btrfs defaults to performing copy-on-write for files. This is often undesirable for VM images, so we need to be able to control whether this behaviour is used.
The virFileSetCOW() will allow for this. We use a tristate, since out of the box, we want the default behaviour attempt to disable cow, but only on btrfs, silently do nothing on non-btrfs. If someone explicitly asks to disable/enable cow, then we want to raise a hard error on non-btrfs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 76 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 80 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73b72c9e10..eea31a736d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2125,6 +2125,7 @@ virFileRewrite; virFileRewriteStr; virFileSanitizePath; virFileSetACLs; +virFileSetCOW; virFileSetupDev; virFileSetXAttr; virFileTouch; diff --git a/src/util/virfile.c b/src/util/virfile.c index 213acdbcaa..5b169b3d11 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -71,6 +71,7 @@ # endif # include <sys/ioctl.h> # include <linux/cdrom.h> +# include <linux/fs.h> #endif
#if HAVE_LIBATTR @@ -4504,3 +4505,78 @@ virFileDataSync(int fd) return fdatasync(fd); #endif } + + +int +virFileSetCOW(const char *path, + virTristateBool state) +{ +#if __linux__ + int val = 0; + struct statfs buf; + VIR_AUTOCLOSE fd = -1; + + VIR_DEBUG("Setting COW flag on '%s' to '%s'", + path, virTristateBoolTypeToString(state)); + + fd = open(path, O_RDONLY|O_NONBLOCK|O_LARGEFILE); + if (fd < 0) { + virReportSystemError(errno, _("unable to open '%s'"), + path); + return -1; + } + + if (fstatfs(fd, &buf) < 0) { + virReportSystemError(errno, _("unable query filesystem type on '%s'"), + path); + return -1; + } + + if (buf.f_type != BTRFS_SUPER_MAGIC) { + if (state == VIR_TRISTATE_BOOL_ABSENT) {
Can't we handle the _ABSENT case before even attempting to open the file?
I see now. This function actually attempts to disable CoW also when VIR_TRISTATE_BOOL_ABSENT is passed. That definitely is not obvious from the function name. Please add a comment explaing what it actually does.
When disabling COW on individual files, we now use the virFileSetCOW method. Note that this change has a slight semantic difference to the old implementation. The original code reported errors but returned success when disabling COW failed. With this new code, we will always report an error if the user requested disabling of COW and we could not honour it, either because btrfs returned an error, or because the filesystem is not btrfs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/storage_util.c | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index ee048f02fe..2595162a61 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -28,9 +28,6 @@ #ifdef __linux__ # include <sys/ioctl.h> # include <linux/fs.h> -# ifndef FS_NOCOW_FL -# define FS_NOCOW_FL 0x00800000 /* Do not cow file */ -# endif # define default_mount_opts "nodev,nosuid,noexec" #elif defined(__FreeBSD__) # define default_mount_opts "nosuid,noexec" @@ -456,25 +453,11 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, } created = true; - if (vol->target.nocow) { -#ifdef __linux__ - int attr; - - /* Set NOCOW flag. This is an optimisation for btrfs. - * The FS_IOC_SETFLAGS ioctl return value will be ignored since any - * failure of this operation should not block the volume creation. - */ - if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0) { - virReportSystemError(errno, "%s", _("Failed to get fs flags")); - } else { - attr |= FS_NOCOW_FL; - if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0) { - virReportSystemError(errno, "%s", - _("Failed to set NOCOW flag")); - } - } -#endif - } + /* NB, COW flag can only be toggled when the file is zero-size, + * so must go before the createRawFile call allocates payload */ + if (vol->target.nocow && + virFileSetCOW(vol->target.path, VIR_TRISTATE_BOOL_NO) < 0) + goto cleanup; if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) /* createRawFile already reported the exact error. */ -- 2.26.2
This calls virFileSetCOW when building a pool with a request to attempt, but not require, COW to be disabled. The effect is that nothing changes on non-btrfs filesystems, but btrfs will get COW disabled on the directory. This setting is then inherited by all newly created files in the pool, avoiding the need for mgmt app to set "nocow" on a per-volume basis. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/storage_util.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 2595162a61..80b49bd1cf 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2712,6 +2712,7 @@ virStorageBackendBuildLocal(virStoragePoolObjPtr pool) bool needs_create_as_uid; unsigned int dir_create_flags; g_autofree char *parent = NULL; + int ret; parent = g_strdup(def->target.path); if (!(p = strrchr(parent, '/'))) { @@ -2745,11 +2746,19 @@ virStorageBackendBuildLocal(virStoragePoolObjPtr pool) /* Now create the final dir in the path with the uid/gid/mode * requested in the config. If the dir already exists, just set * the perms. */ - return virDirCreate(def->target.path, - mode, - def->target.perms.uid, - def->target.perms.gid, - dir_create_flags); + ret = virDirCreate(def->target.path, + mode, + def->target.perms.uid, + def->target.perms.gid, + dir_create_flags); + if (ret < 0) + return -1; + + if (virFileSetCOW(def->target.path, + VIR_TRISTATE_BOOL_ABSENT) < 0) + return -1; + + return 0; } -- 2.26.2
The storage pool code now attempts to disable COW by default on btrfs, but management applications may wish to override this behaviour. Thus we introduce a concept of storage pool features: <features> <cow state='yes|no'/> </features> If the <cow> feature policy is set, it will be enforced. It will always return an hard error if COW cannot be explicitly set or unset. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatstorage.html.in | 25 ++++++++++ docs/schemas/storagepool.rng | 30 ++++++++++++ src/conf/storage_conf.c | 49 ++++++++++++++++++++ src/conf/storage_conf.h | 8 ++++ src/storage/storage_util.c | 2 +- tests/storagepoolxml2xmlin/pool-dir-cow.xml | 10 ++++ tests/storagepoolxml2xmlout/pool-dir-cow.xml | 15 ++++++ tests/storagepoolxml2xmltest.c | 1 + 8 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-cow.xml create mode 100644 tests/storagepoolxml2xmlout/pool-dir-cow.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 2a7604d136..7493714c5c 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -67,6 +67,31 @@ pool. <span class="since">Since 0.4.1</span></dd> </dl> + <h3><a id="StoragePoolFeatures">Features</a></h3> + + <p> + Some pools support optional features: + </p> + + <pre> +... +<features> + <cow state='no'> +</features> +...</pre> + + <p> + Valid features are: + </p> + <ul> + <dd><code>cow</code></dd> + <dt>Controls whether the filesystem performs copy-on-write (COW) for + images in the pool. This may only be set for directory / filesystem + pools on the <code>btrfs</code> filesystem. If not set then libvirt + will attempt to disable COW on any btrfs filesystems. + <span class="since">Since 6.6.0</span>.</dt> + </ul> + <h3><a id="StoragePoolSource">Source elements</a></h3> <p> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index ff0d3c836c..f5cf6769c8 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -37,6 +37,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcedir'/> <ref name='target'/> </interleave> @@ -49,6 +50,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcefs'/> <ref name='target'/> </interleave> @@ -64,6 +66,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcenetfs'/> <ref name='target'/> </interleave> @@ -79,6 +82,7 @@ <interleave> <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcelogical'/> <ref name='targetlogical'/> </interleave> @@ -91,6 +95,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcedisk'/> <ref name='target'/> </interleave> @@ -103,6 +108,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourceiscsi'/> <ref name='target'/> </interleave> @@ -117,6 +123,7 @@ <optional> <ref name='sizing'/> </optional> + <ref name='features'/> <ref name='sourceiscsidirect'/> </interleave> </define> @@ -128,6 +135,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcescsi'/> <ref name='target'/> </interleave> @@ -140,6 +148,7 @@ <interleave> <ref name='commonmetadata'/> <ref name='sizing'/> + <ref name='features'/> <optional> <ref name='sourcempath'/> </optional> @@ -154,6 +163,7 @@ <interleave> <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcerbd'/> <ref name='refresh'/> </interleave> @@ -169,6 +179,7 @@ <interleave> <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcesheepdog'/> </interleave> </define> @@ -180,6 +191,7 @@ <interleave> <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcegluster'/> </interleave> </define> @@ -191,6 +203,7 @@ <interleave> <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcezfs'/> <optional> <ref name='target'/> @@ -205,6 +218,7 @@ <interleave> <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> + <ref name='features'/> <ref name='sourcevstorage'/> <ref name='target'/> </interleave> @@ -277,6 +291,22 @@ </interleave> </define> + <define name='features'> + <optional> + <element name='features'> + <interleave> + <optional> + <element name='cow'> + <attribute name="state"> + <ref name='virYesNo'/> + </attribute> + </element> + </optional> + </interleave> + </element> + </optional> + </define> + <define name='target'> <element name='target'> <interleave> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 65d9b33049..4e63865b39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -839,6 +839,33 @@ virStoragePoolDefRefreshFormat(virBufferPtr buf, } +static int +virStoragePoolDefParseFeatures(virStoragePoolDefPtr def, + xmlXPathContextPtr ctxt) +{ + g_autofree char *cow = virXPathString("string(./features/cow/@state)", ctxt); + + if (cow) { + int val; + if (def->type != VIR_STORAGE_POOL_FS && + def->type != VIR_STORAGE_POOL_DIR) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cow feature may only be used for 'fs' and 'dir' pools")); + return -1; + } + if ((val = virTristateBoolTypeFromString(cow)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid storage pool cow feature state '%s'"), + cow); + return -1; + } + def->features.cow = val; + } + + return 0; +} + + virStoragePoolDefPtr virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { @@ -910,6 +937,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } } + if (virStoragePoolDefParseFeatures(def, ctxt) < 0) + return NULL; + if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { if (!def->source.nhost) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -1131,6 +1161,23 @@ virStoragePoolSourceFormat(virBufferPtr buf, } +static void +virStoragePoolDefFormatFeatures(virBufferPtr buf, + virStoragePoolDefPtr def) +{ + if (!def->features.cow) + return; + + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); + if (def->features.cow) + virBufferAsprintf(buf, "<cow state='%s'/>\n", + virTristateBoolTypeToString(def->features.cow)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); +} + + static int virStoragePoolDefFormatBuf(virBufferPtr buf, virStoragePoolDefPtr def) @@ -1166,6 +1213,8 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferAsprintf(buf, "<available unit='bytes'>%llu</available>\n", def->available); + virStoragePoolDefFormatFeatures(buf, def); + if (virStoragePoolSourceFormat(buf, options, &def->source) < 0) return -1; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index daa21a127b..ffd406e093 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -180,6 +180,13 @@ struct _virStoragePoolSourceDevice { } geometry; }; +typedef struct _virStoragePoolFeatures virStoragePoolFeatures; +typedef virStoragePoolFeatures *virStoragePoolFeaturesPtr; +struct _virStoragePoolFeatures { + virTristateBool cow; +}; + + typedef struct _virStoragePoolSource virStoragePoolSource; typedef virStoragePoolSource *virStoragePoolSourcePtr; struct _virStoragePoolSource { @@ -256,6 +263,7 @@ struct _virStoragePoolDef { unsigned long long capacity; /* bytes */ unsigned long long available; /* bytes */ + virStoragePoolFeatures features; virStoragePoolSource source; virStoragePoolTarget target; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 80b49bd1cf..f7c09e3375 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2755,7 +2755,7 @@ virStorageBackendBuildLocal(virStoragePoolObjPtr pool) return -1; if (virFileSetCOW(def->target.path, - VIR_TRISTATE_BOOL_ABSENT) < 0) + def->features.cow) < 0) return -1; return 0; diff --git a/tests/storagepoolxml2xmlin/pool-dir-cow.xml b/tests/storagepoolxml2xmlin/pool-dir-cow.xml new file mode 100644 index 0000000000..2217f2b8e3 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-dir-cow.xml @@ -0,0 +1,10 @@ +<pool type='dir'> + <name>vms</name> + <uuid>751f8e7e-d2e9-463d-8ffe-d38f5e13a19b</uuid> + <features> + <cow state="yes"/> + </features> + <target> + <path>/i/cant/believe/its/not/btrfs</path> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-dir-cow.xml b/tests/storagepoolxml2xmlout/pool-dir-cow.xml new file mode 100644 index 0000000000..2f3fe1f909 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-dir-cow.xml @@ -0,0 +1,15 @@ +<pool type='dir'> + <name>vms</name> + <uuid>751f8e7e-d2e9-463d-8ffe-d38f5e13a19b</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <features> + <cow state='yes'/> + </features> + <source> + </source> + <target> + <path>/i/cant/believe/its/not/btrfs</path> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 382a7c659f..f21f20357a 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -62,6 +62,7 @@ mymain(void) DO_TEST("pool-dir"); DO_TEST("pool-dir-naming"); + DO_TEST("pool-dir-cow"); DO_TEST("pool-fs"); DO_TEST("pool-logical"); DO_TEST("pool-logical-nopath"); -- 2.26.2
On Mon, Jul 20, 2020 at 18:33:22 +0100, Daniel Berrange wrote:
The storage pool code now attempts to disable COW by default on btrfs, but management applications may wish to override this behaviour. Thus we introduce a concept of storage pool features:
<features> <cow state='yes|no'/> </features>
If the <cow> feature policy is set, it will be enforced. It will always return an hard error if COW cannot be explicitly set or unset.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatstorage.html.in | 25 ++++++++++ docs/schemas/storagepool.rng | 30 ++++++++++++ src/conf/storage_conf.c | 49 ++++++++++++++++++++ src/conf/storage_conf.h | 8 ++++ src/storage/storage_util.c | 2 +- tests/storagepoolxml2xmlin/pool-dir-cow.xml | 10 ++++ tests/storagepoolxml2xmlout/pool-dir-cow.xml | 15 ++++++ tests/storagepoolxml2xmltest.c | 1 + 8 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-cow.xml create mode 100644 tests/storagepoolxml2xmlout/pool-dir-cow.xml
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 65d9b33049..4e63865b39 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -839,6 +839,33 @@ virStoragePoolDefRefreshFormat(virBufferPtr buf, }
+static int +virStoragePoolDefParseFeatures(virStoragePoolDefPtr def, + xmlXPathContextPtr ctxt) +{ + g_autofree char *cow = virXPathString("string(./features/cow/@state)", ctxt); + + if (cow) { + int val; + if (def->type != VIR_STORAGE_POOL_FS && + def->type != VIR_STORAGE_POOL_DIR) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cow feature may only be used for 'fs' and 'dir' pools")); + return -1; + } + if ((val = virTristateBoolTypeFromString(cow)) < 0) {
<= 0
+ virReportError(VIR_ERR_XML_ERROR, + _("invalid storage pool cow feature state '%s'"), + cow); + return -1; + } + def->features.cow = val; + } + + return 0; +} + + virStoragePoolDefPtr virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { @@ -910,6 +937,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } }
+ if (virStoragePoolDefParseFeatures(def, ctxt) < 0) + return NULL; + if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { if (!def->source.nhost) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -1131,6 +1161,23 @@ virStoragePoolSourceFormat(virBufferPtr buf, }
+static void +virStoragePoolDefFormatFeatures(virBufferPtr buf, + virStoragePoolDefPtr def) +{ + if (!def->features.cow) + return; + + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); + if (def->features.cow)
def->features.cow != VIR_TRISTATE_BOOL_ABSENT
+ virBufferAsprintf(buf, "<cow state='%s'/>\n", + virTristateBoolTypeToString(def->features.cow)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); +}
On Mon, Jul 20, 2020 at 1:33 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
We already support a "nocow" flag for storage volumes, but this requires extra work by the mgmt app or user when creating images on btrfs. We want to "do the right thing" out of the box for btrfs.
We achieve this by changint the storage pool code so that when creating a storage pool we always try to disable COW on btrfs filesystems. We then add an <cow state="yes|no"/> feature in the pool XML to let apps override the default logic.
The COW setting on the pool is inherited by any volumes.
The main thing not solved here is that the default directory at /var/lib/libvirt/images is created by the RPM itself, not by a normal "pool-build" command.
Fortunately it appears that virt-install will explicitly invoke a storage pool build even if the directory already exists.
Daniel P. Berrangé (4): util: add a helper method for controlling the COW flag on btrfs storage: convert to use virFileSetCOW storage: attempt to disable COW by default conf: add control over COW for storage pool directories
docs/formatstorage.html.in | 25 +++++++ docs/schemas/storagepool.rng | 30 ++++++++ src/conf/storage_conf.c | 49 +++++++++++++ src/conf/storage_conf.h | 8 +++ src/libvirt_private.syms | 1 + src/storage/storage_util.c | 46 +++++------- src/util/virfile.c | 76 ++++++++++++++++++++ src/util/virfile.h | 3 + tests/storagepoolxml2xmlin/pool-dir-cow.xml | 10 +++ tests/storagepoolxml2xmlout/pool-dir-cow.xml | 15 ++++ tests/storagepoolxml2xmltest.c | 1 + 11 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-cow.xml create mode 100644 tests/storagepoolxml2xmlout/pool-dir-cow.xml
Patch series looks fine. Tested it and it seemed to automatically do what I expected on a fresh Fedora Rawhide system. Reviewed-by: Neal Gompa <ngompa13@gmail.com> -- 真実はいつも一つ！/ Always, there's only one truth!
On Mon, Jul 20, 2020 at 18:33:18 +0100, Daniel Berrange wrote:
We already support a "nocow" flag for storage volumes, but this requires extra work by the mgmt app or user when creating images on btrfs. We want to "do the right thing" out of the box for btrfs.
We achieve this by changint the storage pool code so that when creating a storage pool we always try to disable COW on btrfs filesystems. We then add an <cow state="yes|no"/> feature in the pool XML to let apps override the default logic.
The COW setting on the pool is inherited by any volumes.
The main thing not solved here is that the default directory at /var/lib/libvirt/images is created by the RPM itself, not by a normal "pool-build" command.
Fortunately it appears that virt-install will explicitly invoke a storage pool build even if the directory already exists.
Daniel P. Berrangé (4): util: add a helper method for controlling the COW flag on btrfs storage: convert to use virFileSetCOW storage: attempt to disable COW by default conf: add control over COW for storage pool directories
Once you add the comment to patch 1 and fix the two nits in patch 4: Series: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 7/20/20 7:33 PM, Daniel P. Berrangé wrote:
We already support a "nocow" flag for storage volumes, but this requires extra work by the mgmt app or user when creating images on btrfs. We want to "do the right thing" out of the box for btrfs.
We achieve this by changint the storage pool code so that when creating a storage pool we always try to disable COW on btrfs filesystems. We then add an <cow state="yes|no"/> feature in the pool XML to let apps override the default logic.
The COW setting on the pool is inherited by any volumes.
The main thing not solved here is that the default directory at /var/lib/libvirt/images is created by the RPM itself, not by a normal "pool-build" command.
Fortunately it appears that virt-install will explicitly invoke a storage pool build even if the directory already exists.
Daniel P. Berrangé (4): util: add a helper method for controlling the COW flag on btrfs storage: convert to use virFileSetCOW storage: attempt to disable COW by default conf: add control over COW for storage pool directories
docs/formatstorage.html.in | 25 +++++++ docs/schemas/storagepool.rng | 30 ++++++++ src/conf/storage_conf.c | 49 +++++++++++++ src/conf/storage_conf.h | 8 +++ src/libvirt_private.syms | 1 + src/storage/storage_util.c | 46 +++++------- src/util/virfile.c | 76 ++++++++++++++++++++ src/util/virfile.h | 3 + tests/storagepoolxml2xmlin/pool-dir-cow.xml | 10 +++ tests/storagepoolxml2xmlout/pool-dir-cow.xml | 15 ++++ tests/storagepoolxml2xmltest.c | 1 + 11 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-cow.xml create mode 100644 tests/storagepoolxml2xmlout/pool-dir-cow.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (5)
-
Daniel P. Berrangé -
Jim Fehlig -
Michal Privoznik -
Neal Gompa -
Peter Krempa