12:33 p.m.
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
12:33 p.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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
7:57 a.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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?
8 a.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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 :|
8:08 a.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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.
9:59 a.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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 :|
3:22 p.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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
8:01 a.m.
New subject: [libvirt PATCH 1/4] util: add a helper method for controlling the COW flag on btrfs
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(a)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.
12:33 p.m.
New subject: [libvirt PATCH 2/4] storage: convert to use virFileSetCOW
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(a)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
12:33 p.m.
New subject: [libvirt PATCH 3/4] storage: attempt to disable COW by default
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(a)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
12:33 p.m.
New subject: [libvirt PATCH 4/4] conf: add control over COW for storage pool directories
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(a)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
8:06 a.m.
New subject: [libvirt PATCH 4/4] conf: add control over COW for storage pool directories
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(a)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");
+}
6:56 p.m.
On Mon, Jul 20, 2020 at 1:33 PM Daniel P. Berrangé <berrange(a)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(a)gmail.com>
--
真実はいつも一つ！/ Always, there's only one truth!
8:07 a.m.
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(a)redhat.com>
9:27 a.m.
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(a)redhat.com>
Michal
1583
days inactive
1586
days old
14 comments
5 participants
participants (5)
-
Daniel P. Berrangé -
Jim Fehlig -
Michal Privoznik -
Neal Gompa -
Peter Krempa