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

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

[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);
+		}
+		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}
 };
 
-- 
2.43.0

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

[Qu Wenruo]

在 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}
>   };
>   

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

[Roman Mamedov]

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}
>  };
>  


-- 
With respect,
Roman

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

[Sidong Yang]

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}
> >   };

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

[Sidong Yang]

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