| Message ID | 20220324042235.1483914-1-asmadeus@codewreck.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs-progs: prop: add datacow inode property | expand |
Hi, Dominique Martinet wrote on Thu, Mar 24, 2022 at 01:22:35PM +0900: > The btrfs property documentation states that it is an unified and > user-friendly method to tune btrfs properties instead of chattr, > so let's add something for datacow as well. I appreciate it's a trifling feature, but I'd appreciate not having to teach our users about chattr if they could only have to manipulate btrfs properties so I'd appreciate some feedback! :) If you just say 'no' I'll bite the bullet and install e2fsprogs just for btrfs and document the command, but as things stand my users (embedded device developpers) have no way of disabling cow for e.g. database workloads and that's not really good long-term. > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > - I've sent it on github[1] first as there were other PRs, I'll close it > there if this gets a reply > [1] https://github.com/kdave/btrfs-progs/pull/454 > > - naming: I wasn't sure whether to name it datacow with yes/no, or making it > "nodatacow" with true/false (readonly uses true/false so it might make more > sense to use the later), I've picked datacow to avoid double-negation for > ease of understanding but happy to change it to anything > > - documentation: I got a bit confused with the rst and asciidoc file, as > things got "converted" to rst recently but the asciidoc file didn't get > removed. Should I have updated both? > > Documentation/btrfs-man5.asciidoc | 2 +- > Documentation/btrfs-property.rst | 3 + > Documentation/ch-swapfile.rst | 2 +- > cmds/property.c | 67 ++++++++++++++++++++++ > tests/cli-tests/017-btrfs-property/test.sh | 25 ++++++++ > 5 files changed, 97 insertions(+), 2 deletions(-) > create mode 100755 tests/cli-tests/017-btrfs-property/test.sh > > diff --git a/Documentation/btrfs-man5.asciidoc b/Documentation/btrfs-man5.asciidoc > index dd296fac6fec..a2ed7eb582d9 100644 > --- a/Documentation/btrfs-man5.asciidoc > +++ b/Documentation/btrfs-man5.asciidoc > @@ -712,7 +712,7 @@ To create and activate a swapfile run the following commands: > > -------------------- > # truncate -s 0 swapfile > -# chattr +C swapfile > +# btrfs property set swapfile datacow no > # fallocate -l 2G swapfile > # chmod 0600 swapfile > # mkswap swapfile > diff --git a/Documentation/btrfs-property.rst b/Documentation/btrfs-property.rst > index 5896faa2b2e2..600f6e60d255 100644 > --- a/Documentation/btrfs-property.rst > +++ b/Documentation/btrfs-property.rst > @@ -48,6 +48,9 @@ get [-t <type>] <object> [<name>] > compression > compression algorithm set for an inode, possible values: *lzo*, *zlib*, *zstd*. > To disable compression use "" (empty string), *no* or *none*. > + datacow > + copy on write flag for an inode: *no* or *yes*. > + This is the same as ``chattr``/``lsattr`` *+C* flag. > > list [-t <type>] <object> > Lists available properties with their descriptions for the given object. > diff --git a/Documentation/ch-swapfile.rst b/Documentation/ch-swapfile.rst > index 9d121bc5c569..f682e868632a 100644 > --- a/Documentation/ch-swapfile.rst > +++ b/Documentation/ch-swapfile.rst > @@ -36,7 +36,7 @@ To create and activate a swapfile run the following commands: > .. code-block:: bash > > # truncate -s 0 swapfile > - # chattr +C swapfile > + # btrfs property set swapfile datacow no > # fallocate -l 2G swapfile > # chmod 0600 swapfile > # mkswap swapfile > diff --git a/cmds/property.c b/cmds/property.c > index b3ccc0ff69b0..de9fde9e09e2 100644 > --- a/cmds/property.c > +++ b/cmds/property.c > @@ -24,6 +24,7 @@ > #include <sys/xattr.h> > #include <uuid/uuid.h> > #include <btrfsutil.h> > +#include <linux/fs.h> > #include "cmds/commands.h" > #include "cmds/props.h" > #include "kernel-shared/ctree.h" > @@ -232,6 +233,65 @@ static int prop_compression(enum prop_object_type type, > return ret; > } > > +static int prop_datacow(enum prop_object_type type, > + const char *object, > + const char *name, > + const char *value, > + bool force) > +{ > + int ret; > + ssize_t sret; > + int fd = -1; > + DIR *dirstream = NULL; > + //int open_flags = value ? O_RDWR : O_RDONLY; > + int open_flags = O_RDONLY; > + int attr; > + > + fd = open_file_or_dir3(object, &dirstream, open_flags); > + if (fd == -1) { > + ret = -errno; > + error("failed to open %s: %m", object); > + goto out; > + } > + > + sret = ioctl(fd, FS_IOC_GETFLAGS, &attr); > + if (sret < 0) { > + ret = -errno; > + error("failed to get attr flags on %s: %m", object); > + goto out; > + } > + > + if (value) { > + if (strcmp(value, "no") == 0) { > + attr |= FS_NOCOW_FL; > + } else if (strcmp(value, "yes") == 0) { > + attr &= ~FS_NOCOW_FL; > + } else { > + ret = -EINVAL; > + error("datacow value must be yes or no"); > + goto out; > + } > + > + sret = ioctl(fd, FS_IOC_SETFLAGS, &attr); > + if (sret < 0) { > + ret = -errno; > + error("failed to set nocow flag on %s: %m", > + object); > + goto out; > + } > + } else { > + fprintf(stdout, "datacow=%s\n", > + attr & FS_NOCOW_FL ? "no" : "yes"); > + } > + > + ret = 0; > +out: > + if (fd >= 0) > + close_file_or_dir(fd, dirstream); > + > + return ret; > +} > + > const struct prop_handler prop_handlers[] = { > { > .name ="ro", > @@ -254,6 +314,13 @@ const struct prop_handler prop_handlers[] = { > .types = prop_object_inode, > .handler = prop_compression > }, > + { > + .name = "datacow", > + .desc = "copy on write status of a file", > + .read_only = 0, > + .types = prop_object_inode, > + .handler = prop_datacow > + }, > {NULL, NULL, 0, 0, NULL} > }; > > diff --git a/tests/cli-tests/017-btrfs-property/test.sh b/tests/cli-tests/017-btrfs-property/test.sh > new file mode 100755 > index 000000000000..1da3eda4cd3a > --- /dev/null > +++ b/tests/cli-tests/017-btrfs-property/test.sh > @@ -0,0 +1,25 @@ > +#!/bin/bash > +# test btrfs property commands > + > +source "$TEST_TOP/common" > + > +# compare with lsattr to make sure > +check_global_prereq lsattr > + > +setup_root_helper > +prepare_test_dev > + > +run_check_mkfs_test_dev > +run_check_mount_test_dev > + > +run_check $SUDO_HELPER touch "$TEST_MNT/file" > +run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/file" datacow no > +run_check_stdout $SUDO_HELPER "$TOP/btrfs" property get "$TEST_MNT/file" datacow | > + grep -q "datacow=no" || _fail "datacow wasn't no" > +run_check_stdout $SUDO_HELPER lsattr "$TEST_MNT/file" | > + grep -q -- "C.* " || _fail "lsattr didn't agree NOCOW flag is set" > +run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/file" datacow yes > +run_check_stdout $SUDO_HELPER "$TOP/btrfs" property get "$TEST_MNT/file" datacow | > + grep -q "datacow=yes" || _fail "datacow wasn't yes" > + > +run_check_umount_test_dev
On 30.03.22 г. 8:45 ч., Dominique Martinet wrote: > > I appreciate it's a trifling feature, but I'd appreciate not having to > teach our users about chattr if they could only have to manipulate btrfs > properties so I'd appreciate some feedback!:) > > If you just say 'no' I'll bite the bullet and install e2fsprogs just for > btrfs and document the command, but as things stand my users (embedded > device developpers) have no way of disabling cow for e.g. database > workloads and that's not really good long-term. Just my 2 cents: I think we should strive to rely as much as possible on the generic infrastructure where we can. The nocow options is one such case. The way I see it btrfs property is used to manage features which are indeed specific to btrfs and have no generic alternative. What's more I don't see how 'chattr +C /some/path' can be considered 'complex' to teach someone, plus chattr is a standard linux utility.
Thanks for the reply!! Nikolay Borisov wrote on Tue, Apr 05, 2022 at 05:21:06PM +0300: > On 30.03.22 г. 8:45 ч., Dominique Martinet wrote: > > If you just say 'no' I'll bite the bullet and install e2fsprogs just for > > btrfs and document the command, but as things stand my users (embedded > > device developpers) have no way of disabling cow for e.g. database > > workloads and that's not really good long-term. > > Just my 2 cents: I think we should strive to rely as much as possible on the > generic infrastructure where we can. The nocow options is one such case. The > way I see it btrfs property is used to manage features which are indeed > specific to btrfs and have no generic alternative. That's not what the man page says: btrfs property provides an unified and user-friendly method to tune different btrfs properties instead of using the traditional method like chattr(1) or lsattr(1). I appreciate not wanting to duplicate code however, but documentation should be adjusted if that is what we want to do. > What's more I don't see how 'chattr +C /some/path' can be considered > 'complex' to teach someone, plus chattr is a standard linux utility. Well, '+C' is harder to remember than 'datacow', so in that sense yes it is more complex to teach someone from that regard. My users are mostly windows users who barely ever used linux, and I'm throwing a listing of command they should use in which conditions in our product manual, so it's much more coherent to have a group of 'btrfs property set' commands where I explain ro/compression/datacow together than resort to 'chattr' on one. Yes, there are other generic chattr commands that work on btrfs (append only, immutable come to mind immediately), but compression and datacow are historically handled differently for btrfs... Admittedly not a very good reason, the above and manual page paragraph I quoted are more important to me. (Speaking of which if the mountpoint has compress=smth, lsattr doesn't list +c and `btrfs property set x compression none` doesn't seem to cancel it, so I see no way of disabling compression on a single file in that case on 5.17.1 -- didn't that use to work?) Another reason for me would be that, on alpine linux, chattr is packaged as part of e2fsprogs-extra, which grows my root filesystem by a whole 2MB. This might not seem much, but when I have to make everything fit in <200MB every bit counts. Adding a new flag here doesn't increase the size of the system much. (busybox has a chattr implementation, but for some reason it's not enabled on alpine linux -- I'll request to consider enabling it after checking how big it is if this isn't wanted here) Thanks,
diff --git a/Documentation/btrfs-man5.asciidoc b/Documentation/btrfs-man5.asciidoc index dd296fac6fec..a2ed7eb582d9 100644 --- a/Documentation/btrfs-man5.asciidoc +++ b/Documentation/btrfs-man5.asciidoc @@ -712,7 +712,7 @@ To create and activate a swapfile run the following commands: -------------------- # truncate -s 0 swapfile -# chattr +C swapfile +# btrfs property set swapfile datacow no # fallocate -l 2G swapfile # chmod 0600 swapfile # mkswap swapfile diff --git a/Documentation/btrfs-property.rst b/Documentation/btrfs-property.rst index 5896faa2b2e2..600f6e60d255 100644 --- a/Documentation/btrfs-property.rst +++ b/Documentation/btrfs-property.rst @@ -48,6 +48,9 @@ get [-t <type>] <object> [<name>] compression compression algorithm set for an inode, possible values: *lzo*, *zlib*, *zstd*. To disable compression use "" (empty string), *no* or *none*. + datacow + copy on write flag for an inode: *no* or *yes*. + This is the same as ``chattr``/``lsattr`` *+C* flag. list [-t <type>] <object> Lists available properties with their descriptions for the given object. diff --git a/Documentation/ch-swapfile.rst b/Documentation/ch-swapfile.rst index 9d121bc5c569..f682e868632a 100644 --- a/Documentation/ch-swapfile.rst +++ b/Documentation/ch-swapfile.rst @@ -36,7 +36,7 @@ To create and activate a swapfile run the following commands: .. code-block:: bash # truncate -s 0 swapfile - # chattr +C swapfile + # btrfs property set swapfile datacow no # fallocate -l 2G swapfile # chmod 0600 swapfile # mkswap swapfile diff --git a/cmds/property.c b/cmds/property.c index b3ccc0ff69b0..de9fde9e09e2 100644 --- a/cmds/property.c +++ b/cmds/property.c @@ -24,6 +24,7 @@ #include <sys/xattr.h> #include <uuid/uuid.h> #include <btrfsutil.h> +#include <linux/fs.h> #include "cmds/commands.h" #include "cmds/props.h" #include "kernel-shared/ctree.h" @@ -232,6 +233,65 @@ static int prop_compression(enum prop_object_type type, return ret; } +static int prop_datacow(enum prop_object_type type, + const char *object, + const char *name, + const char *value, + bool force) +{ + int ret; + ssize_t sret; + int fd = -1; + DIR *dirstream = NULL; + //int open_flags = value ? O_RDWR : O_RDONLY; + int open_flags = O_RDONLY; + int attr; + + fd = open_file_or_dir3(object, &dirstream, open_flags); + if (fd == -1) { + ret = -errno; + error("failed to open %s: %m", object); + goto out; + } + + sret = ioctl(fd, FS_IOC_GETFLAGS, &attr); + if (sret < 0) { + ret = -errno; + error("failed to get attr flags on %s: %m", object); + goto out; + } + + if (value) { + if (strcmp(value, "no") == 0) { + attr |= FS_NOCOW_FL; + } else if (strcmp(value, "yes") == 0) { + attr &= ~FS_NOCOW_FL; + } else { + ret = -EINVAL; + error("datacow value must be yes or no"); + goto out; + } + + sret = ioctl(fd, FS_IOC_SETFLAGS, &attr); + if (sret < 0) { + ret = -errno; + error("failed to set nocow flag on %s: %m", + object); + goto out; + } + } else { + fprintf(stdout, "datacow=%s\n", + attr & FS_NOCOW_FL ? "no" : "yes"); + } + + ret = 0; +out: + if (fd >= 0) + close_file_or_dir(fd, dirstream); + + return ret; +} + const struct prop_handler prop_handlers[] = { { .name ="ro", @@ -254,6 +314,13 @@ const struct prop_handler prop_handlers[] = { .types = prop_object_inode, .handler = prop_compression }, + { + .name = "datacow", + .desc = "copy on write status of a file", + .read_only = 0, + .types = prop_object_inode, + .handler = prop_datacow + }, {NULL, NULL, 0, 0, NULL} }; diff --git a/tests/cli-tests/017-btrfs-property/test.sh b/tests/cli-tests/017-btrfs-property/test.sh new file mode 100755 index 000000000000..1da3eda4cd3a --- /dev/null +++ b/tests/cli-tests/017-btrfs-property/test.sh @@ -0,0 +1,25 @@ +#!/bin/bash +# test btrfs property commands + +source "$TEST_TOP/common" + +# compare with lsattr to make sure +check_global_prereq lsattr + +setup_root_helper +prepare_test_dev + +run_check_mkfs_test_dev +run_check_mount_test_dev + +run_check $SUDO_HELPER touch "$TEST_MNT/file" +run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/file" datacow no +run_check_stdout $SUDO_HELPER "$TOP/btrfs" property get "$TEST_MNT/file" datacow | + grep -q "datacow=no" || _fail "datacow wasn't no" +run_check_stdout $SUDO_HELPER lsattr "$TEST_MNT/file" | + grep -q -- "C.* " || _fail "lsattr didn't agree NOCOW flag is set" +run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/file" datacow yes +run_check_stdout $SUDO_HELPER "$TOP/btrfs" property get "$TEST_MNT/file" datacow | + grep -q "datacow=yes" || _fail "datacow wasn't yes" + +run_check_umount_test_dev