Re: [PATCH] btrfs-progs: property: introduce drop_subtree_threshold property

[Sidong Yang <realwakka@gmail.com>]

On Sun, Aug 11, 2024 at 03:25:13PM +0930, Qu Wenruo wrote:

Hi Qu. Thanks for review.
> 
> 
> 在 2024/8/10 15:47, 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>
> > ---
> >   cmds/property.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 47 insertions(+)
> > 
> > diff --git a/cmds/property.c b/cmds/property.c
> > index a36b5ab2..44b62af6 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,45 @@ 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) {
> 
> Since qgroups/ directory is automatically generated/removed according to
> the qgroup status, you have to handle -ENOENT case, other than just
> erroring out.
> 
> At least you can do a warning if qgroup is not enabled.

Okay, I didn't care about it. It should handle -ENOENT and print warning
in next version.
> 
> > +		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 +299,13 @@ const struct prop_handler prop_handlers[] = {
> >   		.types = prop_object_inode,
> >   		.handler = prop_compression
> >   	},
> > +	{
> > +		.name = "drop_subtree_threshold",
> > +		.desc = "threshold for dropping reference to subtree",
> 
> The feature is qgroup specific, thus it's better to mention it.
> 
> Furthermore you didn't explain what threshold it is.
> 
> I'd go something like:
> 
> "subtree level threshold to mark qgroup inconsistent during snapshot
> deletion, can reduce qgroup workload of snapshot deletion".

Thanks for detailed example. It will be used for next patch. Honestly, I
had no idea about this option. I'll look at the kernel implementation.

Thanks,
Sidong

> 
> Thanks,
> Qu
> > +		.read_only = 0,
> > +		.types = prop_object_root,
> > +		.handler = prop_drop_subtree_threshold
> > +	},
> >   	{NULL, NULL, 0, 0, NULL}
> >   };
> >