Re: [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&gt; >>>> --- >>>> 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'. */