| Message ID | 20240811122415.6575-1-realwakka@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs-progs: property: introduce drop_subtree_threshold property | expand |
在 2024/8/11 21:54, Sidong Yang 写道: > This patch introduces new property drop_subtree_threshold. This property > could be set/get easily by root dir without find sysfs path. > > Fixes: https://github.com/kdave/btrfs-progs/issues/795 > > Issue: #795 > Signed-off-by: Sidong Yang <realwakka@gmail.com> > --- > v2: error msg for -ENOENT, fix desc for prop > --- > cmds/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/cmds/property.c b/cmds/property.c > index a36b5ab2..88344001 100644 > --- a/cmds/property.c > +++ b/cmds/property.c > @@ -35,6 +35,7 @@ > #include "common/utils.h" > #include "common/help.h" > #include "common/filesystem-utils.h" > +#include "common/sysfs-utils.h" > #include "cmds/commands.h" > #include "cmds/props.h" > > @@ -236,6 +237,50 @@ out: > > return ret; > } > +static int prop_drop_subtree_threshold(enum prop_object_type type, > + const char *object, > + const char *name, > + const char *value, > + bool force) { > + int ret; > + int fd; > + int sysfs_fd; > + char buf[255]; > + > + fd = btrfs_open_path(object, value, false); > + if (fd < 0) > + return -errno; > + > + sysfs_fd = sysfs_open_fsid_file(fd, "qgroups/drop_subtree_threshold"); > + if (sysfs_fd < 0) { > + if (sysfs_fd == -ENOENT) { > + error("failed to access qgroups/drop_subtree_threshold for %s," > + " quota should be enabled for this property: %m", > + object); I do not think this needs to be error(), warning() should be enough. > + } > + close(fd); > + return -errno; Furthermore for ENOENT case I do not even think we should return error. The point here is, if qgroup is not running (or running in simple quota) mode, there is no need to set threshold at all, thus no need to return any error. > + } > + > + if (value) { > + ret = write(sysfs_fd, value, strlen(value)); > + } else { > + ret = read(sysfs_fd, buf, 255); > + if (ret > 0) { > + buf[ret] = 0; > + pr_verbose(LOG_DEFAULT, "drop_subtree_threshold=%s", buf); > + } > + } > + if (ret < 0) { > + ret = -errno; > + } else { > + ret = 0; > + } > + > + close(sysfs_fd); > + close(fd); > + return ret; > +} > > const struct prop_handler prop_handlers[] = { > { > @@ -259,6 +304,14 @@ const struct prop_handler prop_handlers[] = { > .types = prop_object_inode, > .handler = prop_compression > }, > + { > + .name = "drop_subtree_threshold", > + .desc = "subtree level threshold to mark qgroup inconsistent during" > + "snapshot deletion, can reduce qgroup workload of snapshot deletion", Please do not break the string, even it's very long. Thanks, Qu > + .read_only = 0, > + .types = prop_object_root, > + .handler = prop_drop_subtree_threshold > + }, > {NULL, NULL, 0, 0, NULL} > }; >
On Sun, 11 Aug 2024 12:24:15 +0000 Sidong Yang <realwakka@gmail.com> wrote: > This patch introduces new property drop_subtree_threshold. This property > could be set/get easily by root dir without find sysfs path. > > Fixes: https://github.com/kdave/btrfs-progs/issues/795 > > Issue: #795 > Signed-off-by: Sidong Yang <realwakka@gmail.com> > --- > v2: error msg for -ENOENT, fix desc for prop > --- > cmds/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/cmds/property.c b/cmds/property.c > index a36b5ab2..88344001 100644 > --- a/cmds/property.c > +++ b/cmds/property.c > @@ -35,6 +35,7 @@ > #include "common/utils.h" > #include "common/help.h" > #include "common/filesystem-utils.h" > +#include "common/sysfs-utils.h" > #include "cmds/commands.h" > #include "cmds/props.h" > > @@ -236,6 +237,50 @@ out: > > return ret; > } > +static int prop_drop_subtree_threshold(enum prop_object_type type, > + const char *object, > + const char *name, > + const char *value, > + bool force) { > + int ret; > + int fd; > + int sysfs_fd; > + char buf[255]; > + > + fd = btrfs_open_path(object, value, false); This line is indented with spaces instead of TABs like the rest and as it should. I did not check if this is the only occurrence. Checkpatch should catch this: https://docs.kernel.org/dev-tools/checkpatch.html > + if (fd < 0) > + return -errno; > + > + sysfs_fd = sysfs_open_fsid_file(fd, "qgroups/drop_subtree_threshold"); > + if (sysfs_fd < 0) { > + if (sysfs_fd == -ENOENT) { > + error("failed to access qgroups/drop_subtree_threshold for %s," > + " quota should be enabled for this property: %m", > + object); > + } > + close(fd); > + return -errno; > + } > + > + if (value) { > + ret = write(sysfs_fd, value, strlen(value)); > + } else { > + ret = read(sysfs_fd, buf, 255); > + if (ret > 0) { > + buf[ret] = 0; > + pr_verbose(LOG_DEFAULT, "drop_subtree_threshold=%s", buf); > + } > + } > + if (ret < 0) { > + ret = -errno; > + } else { > + ret = 0; > + } > + > + close(sysfs_fd); > + close(fd); > + return ret; > +} > > const struct prop_handler prop_handlers[] = { > { > @@ -259,6 +304,14 @@ const struct prop_handler prop_handlers[] = { > .types = prop_object_inode, > .handler = prop_compression > }, > + { > + .name = "drop_subtree_threshold", > + .desc = "subtree level threshold to mark qgroup inconsistent during" > + "snapshot deletion, can reduce qgroup workload of snapshot deletion", > + .read_only = 0, > + .types = prop_object_root, > + .handler = prop_drop_subtree_threshold > + }, > {NULL, NULL, 0, 0, NULL} > }; >
On Mon, Aug 12, 2024 at 08:16:13AM +0930, Qu Wenruo wrote: > > > 在 2024/8/11 21:54, Sidong Yang 写道: > > This patch introduces new property drop_subtree_threshold. This property > > could be set/get easily by root dir without find sysfs path. > > > > Fixes: https://github.com/kdave/btrfs-progs/issues/795 > > > > Issue: #795 > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > > --- > > v2: error msg for -ENOENT, fix desc for prop > > --- > > cmds/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/cmds/property.c b/cmds/property.c > > index a36b5ab2..88344001 100644 > > --- a/cmds/property.c > > +++ b/cmds/property.c > > @@ -35,6 +35,7 @@ > > #include "common/utils.h" > > #include "common/help.h" > > #include "common/filesystem-utils.h" > > +#include "common/sysfs-utils.h" > > #include "cmds/commands.h" > > #include "cmds/props.h" > > @@ -236,6 +237,50 @@ out: > > return ret; > > } > > +static int prop_drop_subtree_threshold(enum prop_object_type type, > > + const char *object, > > + const char *name, > > + const char *value, > > + bool force) { > > + int ret; > > + int fd; > > + int sysfs_fd; > > + char buf[255]; > > + > > + fd = btrfs_open_path(object, value, false); > > + if (fd < 0) > > + return -errno; > > + > > + sysfs_fd = sysfs_open_fsid_file(fd, "qgroups/drop_subtree_threshold"); > > + if (sysfs_fd < 0) { > > + if (sysfs_fd == -ENOENT) { > > + error("failed to access qgroups/drop_subtree_threshold for %s," > > + " quota should be enabled for this property: %m", > > + object); > > I do not think this needs to be error(), warning() should be enough. > > > + } > > + close(fd); > > + return -errno; > > Furthermore for ENOENT case I do not even think we should return error. > > The point here is, if qgroup is not running (or running in simple quota) > mode, there is no need to set threshold at all, thus no need to return any > error. > Okay, I understood. It's better to return 0 if qgroup is not running in next patch version. > > + } > > + > > + if (value) { > > + ret = write(sysfs_fd, value, strlen(value)); > > + } else { > > + ret = read(sysfs_fd, buf, 255); > > + if (ret > 0) { > > + buf[ret] = 0; > > + pr_verbose(LOG_DEFAULT, "drop_subtree_threshold=%s", buf); > > + } > > + } > > + if (ret < 0) { > > + ret = -errno; > > + } else { > > + ret = 0; > > + } > > + > > + close(sysfs_fd); > > + close(fd); > > + return ret; > > +} > > const struct prop_handler prop_handlers[] = { > > { > > @@ -259,6 +304,14 @@ const struct prop_handler prop_handlers[] = { > > .types = prop_object_inode, > > .handler = prop_compression > > }, > > + { > > + .name = "drop_subtree_threshold", > > + .desc = "subtree level threshold to mark qgroup inconsistent during" > > + "snapshot deletion, can reduce qgroup workload of snapshot deletion", > > Please do not break the string, even it's very long. Thanks, I was worried about this. Thanks, Sidong > > Thanks, > Qu > > > + .read_only = 0, > > + .types = prop_object_root, > > + .handler = prop_drop_subtree_threshold > > + }, > > {NULL, NULL, 0, 0, NULL} > > };
On Mon, Aug 12, 2024 at 04:17:20AM +0500, Roman Mamedov wrote: Hi! Roman. Thanks for review. > On Sun, 11 Aug 2024 12:24:15 +0000 > Sidong Yang <realwakka@gmail.com> wrote: > > > This patch introduces new property drop_subtree_threshold. This property > > could be set/get easily by root dir without find sysfs path. > > > > Fixes: https://github.com/kdave/btrfs-progs/issues/795 > > > > Issue: #795 > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > > --- > > v2: error msg for -ENOENT, fix desc for prop > > --- > > cmds/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/cmds/property.c b/cmds/property.c > > index a36b5ab2..88344001 100644 > > --- a/cmds/property.c > > +++ b/cmds/property.c > > @@ -35,6 +35,7 @@ > > #include "common/utils.h" > > #include "common/help.h" > > #include "common/filesystem-utils.h" > > +#include "common/sysfs-utils.h" > > #include "cmds/commands.h" > > #include "cmds/props.h" > > > > @@ -236,6 +237,50 @@ out: > > > > return ret; > > } > > +static int prop_drop_subtree_threshold(enum prop_object_type type, > > + const char *object, > > + const char *name, > > + const char *value, > > + bool force) { > > + int ret; > > + int fd; > > + int sysfs_fd; > > + char buf[255]; > > + > > + fd = btrfs_open_path(object, value, false); > > This line is indented with spaces instead of TABs like the rest and as it > should. I did not check if this is the only occurrence. > > Checkpatch should catch this: https://docs.kernel.org/dev-tools/checkpatch.html Oops, I didn't notice about this. thanks for tip! > > > + if (fd < 0) > > + return -errno; > > + > > + sysfs_fd = sysfs_open_fsid_file(fd, "qgroups/drop_subtree_threshold"); > > + if (sysfs_fd < 0) { > > + if (sysfs_fd == -ENOENT) { > > + error("failed to access qgroups/drop_subtree_threshold for %s," > > + " quota should be enabled for this property: %m", > > + object); > > + } > > + close(fd); > > + return -errno; > > + } > > + > > + if (value) { > > + ret = write(sysfs_fd, value, strlen(value)); > > + } else { > > + ret = read(sysfs_fd, buf, 255); > > + if (ret > 0) { > > + buf[ret] = 0; > > + pr_verbose(LOG_DEFAULT, "drop_subtree_threshold=%s", buf); > > + } > > + } > > + if (ret < 0) { > > + ret = -errno; > > + } else { > > + ret = 0; > > + } > > + > > + close(sysfs_fd); > > + close(fd); > > + return ret; > > +} > > > > const struct prop_handler prop_handlers[] = { > > { > > @@ -259,6 +304,14 @@ const struct prop_handler prop_handlers[] = { > > .types = prop_object_inode, > > .handler = prop_compression > > }, > > + { > > + .name = "drop_subtree_threshold", > > + .desc = "subtree level threshold to mark qgroup inconsistent during" > > + "snapshot deletion, can reduce qgroup workload of snapshot deletion", > > + .read_only = 0, > > + .types = prop_object_root, > > + .handler = prop_drop_subtree_threshold > > + }, > > {NULL, NULL, 0, 0, NULL} > > }; > > > > > -- > With respect, > Roman Thanks, Sidong
diff --git a/cmds/property.c b/cmds/property.c index a36b5ab2..88344001 100644 --- a/cmds/property.c +++ b/cmds/property.c @@ -35,6 +35,7 @@ #include "common/utils.h" #include "common/help.h" #include "common/filesystem-utils.h" +#include "common/sysfs-utils.h" #include "cmds/commands.h" #include "cmds/props.h" @@ -236,6 +237,50 @@ out: return ret; } +static int prop_drop_subtree_threshold(enum prop_object_type type, + const char *object, + const char *name, + const char *value, + bool force) { + int ret; + int fd; + int sysfs_fd; + char buf[255]; + + fd = btrfs_open_path(object, value, false); + if (fd < 0) + return -errno; + + sysfs_fd = sysfs_open_fsid_file(fd, "qgroups/drop_subtree_threshold"); + if (sysfs_fd < 0) { + if (sysfs_fd == -ENOENT) { + error("failed to access qgroups/drop_subtree_threshold for %s," + " quota should be enabled for this property: %m", + object); + } + close(fd); + return -errno; + } + + if (value) { + ret = write(sysfs_fd, value, strlen(value)); + } else { + ret = read(sysfs_fd, buf, 255); + if (ret > 0) { + buf[ret] = 0; + pr_verbose(LOG_DEFAULT, "drop_subtree_threshold=%s", buf); + } + } + if (ret < 0) { + ret = -errno; + } else { + ret = 0; + } + + close(sysfs_fd); + close(fd); + return ret; +} const struct prop_handler prop_handlers[] = { { @@ -259,6 +304,14 @@ const struct prop_handler prop_handlers[] = { .types = prop_object_inode, .handler = prop_compression }, + { + .name = "drop_subtree_threshold", + .desc = "subtree level threshold to mark qgroup inconsistent during" + "snapshot deletion, can reduce qgroup workload of snapshot deletion", + .read_only = 0, + .types = prop_object_root, + .handler = prop_drop_subtree_threshold + }, {NULL, NULL, 0, 0, NULL} };
This patch introduces new property drop_subtree_threshold. This property could be set/get easily by root dir without find sysfs path. Fixes: https://github.com/kdave/btrfs-progs/issues/795 Issue: #795 Signed-off-by: Sidong Yang <realwakka@gmail.com> --- v2: error msg for -ENOENT, fix desc for prop --- cmds/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)