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.
© 2016 - 2026 Red Hat, Inc.