Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/3] Misc props.c cleanups
@ 2019-02-12  2:18 Anand Jain
  2019-02-12  2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-12  2:18 UTC (permalink / raw)
  To: linux-btrfs

v3: Merge patch 2/5 and 3/5 as in v1.
    Not included 1/5 in v1 as its already integrated in misc-next.

While adding the readmirror property found few cleanup things which
can be fixed. As these aren't part of upcoming readmirror property
I am sending these separately.

Anand Jain (3):
  btrfs: kill __btrfs_set_prop()
  btrfs: drop redundant forward declaration in props.c
  btrfs: trivial, fix c coding style

 fs/btrfs/ioctl.c |  10 +--
 fs/btrfs/props.c | 201 +++++++++++++++++++++++++------------------------------
 fs/btrfs/props.h |   6 +-
 fs/btrfs/xattr.c |   2 +-
 4 files changed, 99 insertions(+), 120 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/3] btrfs: kill __btrfs_set_prop()
  2019-02-12  2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
@ 2019-02-12  2:18 ` Anand Jain
  2019-02-20 19:00   ` David Sterba
  2019-02-12  2:18 ` [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c Anand Jain
  2019-02-12  2:18 ` [PATCH v3 3/3] btrfs: trivial, fix c coding style Anand Jain
  2 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2019-02-12  2:18 UTC (permalink / raw)
  To: linux-btrfs

btrfs_set_prop() is a redirect to __btrfs_set_prop() with the
transaction handler equal to NULL. And __btrfs_set_prop() inturn diectly
uses trans to do_setxattr() which when trans is NULL creates a transaction.

Instead rename __btrfs_set_prop() to btrfs_set_prop(), and update the
caller with NULL argument.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: none
v2: none

 fs/btrfs/ioctl.c | 10 ++++++----
 fs/btrfs/props.c | 22 +++++-----------------
 fs/btrfs/props.h |  6 ++----
 fs/btrfs/xattr.c |  2 +-
 4 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 3f9d7be30bf4..5a4ed2f66e09 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -284,7 +284,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		binode->flags &= ~BTRFS_INODE_COMPRESS;
 		binode->flags |= BTRFS_INODE_NOCOMPRESS;
 
-		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
+		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
+				     0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
 	} else if (fsflags & FS_COMPR_FL) {
@@ -302,13 +303,14 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		if (!comp || comp[0] == 0)
 			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
 
-		ret = btrfs_set_prop(inode, "btrfs.compression",
-				     comp, strlen(comp), 0);
+		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", comp,
+				     strlen(comp), 0);
 		if (ret)
 			goto out_drop;
 
 	} else {
-		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
+		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
+				     0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
 		binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index dc6140013ae8..4525a2a4d1cd 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -85,12 +85,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
 	return NULL;
 }
 
-static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
-			    struct inode *inode,
-			    const char *name,
-			    const char *value,
-			    size_t value_len,
-			    int flags)
+int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
+		   const char *name, const char *value, size_t value_len,
+		   int flags)
 {
 	const struct prop_handler *handler;
 	int ret;
@@ -133,15 +130,6 @@ static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
-int btrfs_set_prop(struct inode *inode,
-		   const char *name,
-		   const char *value,
-		   size_t value_len,
-		   int flags)
-{
-	return __btrfs_set_prop(NULL, inode, name, value, value_len, flags);
-}
-
 static int iterate_object_props(struct btrfs_root *root,
 				struct btrfs_path *path,
 				u64 objectid,
@@ -313,8 +301,8 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
 		if (ret)
 			goto out;
-		ret = __btrfs_set_prop(trans, inode, h->xattr_name,
-				       value, strlen(value), 0);
+		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,
+				     strlen(value), 0);
 		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);
 		if (ret)
 			goto out;
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 618815b4f9d5..9dbdae47cf27 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -10,10 +10,8 @@
 
 void __init btrfs_props_init(void);
 
-int btrfs_set_prop(struct inode *inode,
-		   const char *name,
-		   const char *value,
-		   size_t value_len,
+int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
+		   const char *name, const char *value, size_t value_len,
 		   int flags);
 
 int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index f141b45ce349..499bb79ba135 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -379,7 +379,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
 					size_t size, int flags)
 {
 	name = xattr_full_name(handler, name);
-	return btrfs_set_prop(inode, name, value, size, flags);
+	return btrfs_set_prop(NULL, inode, name, value, size, flags);
 }
 
 static const struct xattr_handler btrfs_security_xattr_handler = {
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c
  2019-02-12  2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
  2019-02-12  2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
@ 2019-02-12  2:18 ` Anand Jain
  2019-02-12  2:18 ` [PATCH v3 3/3] btrfs: trivial, fix c coding style Anand Jain
  2 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-12  2:18 UTC (permalink / raw)
  To: linux-btrfs

Drop forward declaration of the functions,
prop_compression_validate(), prop_compression_apply() and
prop_compression_extract(). By moving prop_handlers[], btrfs_props_init()
prop_compression_validate(), prop_compression_apply() and
prop_compression_extract() appropriately within the file. No functional
changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v3: merge with 2/5 and 3/5 as in v1
v2: none
 fs/btrfs/props.c | 163 +++++++++++++++++++++++++++----------------------------
 1 file changed, 79 insertions(+), 84 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 4525a2a4d1cd..77a03076b18e 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -23,36 +23,6 @@ struct prop_handler {
 	int inheritable;
 };
 
-static int prop_compression_validate(const char *value, size_t len);
-static int prop_compression_apply(struct inode *inode,
-				  const char *value,
-				  size_t len);
-static const char *prop_compression_extract(struct inode *inode);
-
-static struct prop_handler prop_handlers[] = {
-	{
-		.xattr_name = XATTR_BTRFS_PREFIX "compression",
-		.validate = prop_compression_validate,
-		.apply = prop_compression_apply,
-		.extract = prop_compression_extract,
-		.inheritable = 1
-	},
-};
-
-void __init btrfs_props_init(void)
-{
-	int i;
-
-	hash_init(prop_handlers_ht);
-
-	for (i = 0; i < ARRAY_SIZE(prop_handlers); i++) {
-		struct prop_handler *p = &prop_handlers[i];
-		u64 h = btrfs_name_hash(p->xattr_name, strlen(p->xattr_name));
-
-		hash_add(prop_handlers_ht, &p->node, h);
-	}
-}
-
 static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
 {
 	struct hlist_head *h;
@@ -271,6 +241,78 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
 	return ret;
 }
 
+static int prop_compression_validate(const char *value, size_t len)
+{
+	if (!value)
+		return 0;
+
+	if (!strncmp("lzo", value, len))
+		return 0;
+	else if (!strncmp("zlib", value, len))
+		return 0;
+	else if (!strncmp("zstd", value, len))
+		return 0;
+
+	return -EINVAL;
+}
+
+static int prop_compression_apply(struct inode *inode, const char *value,
+				  size_t len)
+{
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	int type;
+
+	if (len == 0) {
+		BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
+		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
+		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
+
+		return 0;
+	}
+
+	if (!strncmp("lzo", value, 3)) {
+		type = BTRFS_COMPRESS_LZO;
+		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
+	} else if (!strncmp("zlib", value, 4)) {
+		type = BTRFS_COMPRESS_ZLIB;
+	} else if (!strncmp("zstd", value, len)) {
+		type = BTRFS_COMPRESS_ZSTD;
+		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
+	} else {
+		return -EINVAL;
+	}
+
+	BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
+	BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
+	BTRFS_I(inode)->prop_compress = type;
+
+	return 0;
+}
+
+static const char *prop_compression_extract(struct inode *inode)
+{
+	switch (BTRFS_I(inode)->prop_compress) {
+	case BTRFS_COMPRESS_ZLIB:
+	case BTRFS_COMPRESS_LZO:
+	case BTRFS_COMPRESS_ZSTD:
+		return btrfs_compress_type2str(BTRFS_I(inode)->prop_compress);
+	default:
+		break;
+	}
+
+	return NULL;
+}
+
+static struct prop_handler prop_handlers[] = {
+	{
+		.xattr_name = XATTR_BTRFS_PREFIX "compression",
+		.validate = prop_compression_validate,
+		.apply = prop_compression_apply,
+		.extract = prop_compression_extract,
+		.inheritable = 1
+	},
+};
+
 static int inherit_props(struct btrfs_trans_handle *trans,
 			 struct inode *inode,
 			 struct inode *parent)
@@ -352,64 +394,17 @@ int btrfs_subvol_inherit_props(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static int prop_compression_validate(const char *value, size_t len)
-{
-	if (!strncmp("lzo", value, len))
-		return 0;
-	else if (!strncmp("zlib", value, len))
-		return 0;
-	else if (!strncmp("zstd", value, len))
-		return 0;
-
-	return -EINVAL;
-}
-
-static int prop_compression_apply(struct inode *inode,
-				  const char *value,
-				  size_t len)
+void __init btrfs_props_init(void)
 {
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	int type;
-
-	if (len == 0) {
-		BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
-		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
-		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
-
-		return 0;
-	}
-
-	if (!strncmp("lzo", value, 3)) {
-		type = BTRFS_COMPRESS_LZO;
-		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
-	} else if (!strncmp("zlib", value, 4)) {
-		type = BTRFS_COMPRESS_ZLIB;
-	} else if (!strncmp("zstd", value, len)) {
-		type = BTRFS_COMPRESS_ZSTD;
-		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
-	} else {
-		return -EINVAL;
-	}
+	int i;
 
-	BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
-	BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
-	BTRFS_I(inode)->prop_compress = type;
+	hash_init(prop_handlers_ht);
 
-	return 0;
-}
+	for (i = 0; i < ARRAY_SIZE(prop_handlers); i++) {
+		struct prop_handler *p = &prop_handlers[i];
+		u64 h = btrfs_name_hash(p->xattr_name, strlen(p->xattr_name));
 
-static const char *prop_compression_extract(struct inode *inode)
-{
-	switch (BTRFS_I(inode)->prop_compress) {
-	case BTRFS_COMPRESS_ZLIB:
-	case BTRFS_COMPRESS_LZO:
-	case BTRFS_COMPRESS_ZSTD:
-		return btrfs_compress_type2str(BTRFS_I(inode)->prop_compress);
-	default:
-		break;
+		hash_add(prop_handlers_ht, &p->node, h);
 	}
-
-	return NULL;
 }
 
-
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 3/3] btrfs: trivial, fix c coding style
  2019-02-12  2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
  2019-02-12  2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
  2019-02-12  2:18 ` [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c Anand Jain
@ 2019-02-12  2:18 ` Anand Jain
  2 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-12  2:18 UTC (permalink / raw)
  To: linux-btrfs

Maintain the lines extented upto 80 char where possible, and indent the
argument.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: changelog added.
 fs/btrfs/props.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 77a03076b18e..3c15f19bfd2f 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -70,8 +70,8 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		return -EINVAL;
 
 	if (value_len == 0) {
-		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
-				       NULL, 0, flags);
+		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
+				     flags);
 		if (ret)
 			return ret;
 
@@ -101,12 +101,10 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 }
 
 static int iterate_object_props(struct btrfs_root *root,
-				struct btrfs_path *path,
-				u64 objectid,
+				struct btrfs_path *path, u64 objectid,
 				void (*iterator)(void *,
 						 const struct prop_handler *,
-						 const char *,
-						 size_t),
+						 const char *, size_t),
 				void *ctx)
 {
 	int ret;
@@ -211,10 +209,8 @@ static int iterate_object_props(struct btrfs_root *root,
 	return ret;
 }
 
-static void inode_prop_iterator(void *ctx,
-				const struct prop_handler *handler,
-				const char *value,
-				size_t len)
+static void inode_prop_iterator(void *ctx, const struct prop_handler *handler,
+				const char *value, size_t len)
 {
 	struct inode *inode = ctx;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/3] btrfs: kill __btrfs_set_prop()
  2019-02-12  2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
@ 2019-02-20 19:00   ` David Sterba
  2019-02-21  9:03     ` Anand Jain
  0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2019-02-20 19:00 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Tue, Feb 12, 2019 at 10:18:49AM +0800, Anand Jain wrote:
> btrfs_set_prop() is a redirect to __btrfs_set_prop() with the
> transaction handler equal to NULL. And __btrfs_set_prop() inturn diectly
> uses trans to do_setxattr() which when trans is NULL creates a transaction.

That's right and I think that some of the callsites could actually pass
the existing transaction to btrfs_set_prop instead of relying on the
fallback behaviour.

Also, the potential NULL as transaction handle is not a good thing from
the API point of view and I'd like to reduce that to minimum (there are
some justified cases).

> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3f9d7be30bf4..5a4ed2f66e09 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -284,7 +284,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  		binode->flags &= ~BTRFS_INODE_COMPRESS;
>  		binode->flags |= BTRFS_INODE_NOCOMPRESS;
>  
> -		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
> +		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
> +				     0);
>  		if (ret && ret != -ENODATA)
>  			goto out_drop;
>  	} else if (fsflags & FS_COMPR_FL) {
> @@ -302,13 +303,14 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  		if (!comp || comp[0] == 0)
>  			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
>  
> -		ret = btrfs_set_prop(inode, "btrfs.compression",
> -				     comp, strlen(comp), 0);
> +		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", comp,
> +				     strlen(comp), 0);
>  		if (ret)
>  			goto out_drop;
>  
>  	} else {
> -		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
> +		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
> +				     0);
>  		if (ret && ret != -ENODATA)
>  			goto out_drop;
>  		binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);

When the if-else block ends, there's a new transaction started, this
seems unnecessary.

> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index dc6140013ae8..4525a2a4d1cd 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -85,12 +85,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
>  	return NULL;
>  }
>  
> -static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
> -			    struct inode *inode,
> -			    const char *name,
> -			    const char *value,
> -			    size_t value_len,
> -			    int flags)
> +int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
> +		   const char *name, const char *value, size_t value_len,
> +		   int flags)
>  {
>  	const struct prop_handler *handler;
>  	int ret;
> @@ -133,15 +130,6 @@ static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
>  	return 0;
>  }
>  
> -int btrfs_set_prop(struct inode *inode,
> -		   const char *name,
> -		   const char *value,
> -		   size_t value_len,
> -		   int flags)
> -{
> -	return __btrfs_set_prop(NULL, inode, name, value, value_len, flags);

I agree that one function would be better here, with defined semantics
of 'trans'.

There are more cleanups around the properties, also in the xattr
handling functions. This patchset is a step in the right direction and I
think the cleanups could be more extensive.

The idea for the xattr function:

- the VFS callbacks (like btrfs_xattr_handler_set_prop) will start the
  transaction themselves and pass the handle to the prop function

- a xattr function that does not take a valid trans could be named like
  btrfs_setxattr_notrans and will start the transaction, ie. lifting
  that to the callers

- there's a maze of the xattr callbacks so assertions are needed

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/3] btrfs: kill __btrfs_set_prop()
  2019-02-20 19:00   ` David Sterba
@ 2019-02-21  9:03     ` Anand Jain
  0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-21  9:03 UTC (permalink / raw)
  To: dsterba, linux-btrfs



On 2/21/19 3:00 AM, David Sterba wrote:
> On Tue, Feb 12, 2019 at 10:18:49AM +0800, Anand Jain wrote:
>> btrfs_set_prop() is a redirect to __btrfs_set_prop() with the
>> transaction handler equal to NULL. And __btrfs_set_prop() inturn diectly
>> uses trans to do_setxattr() which when trans is NULL creates a transaction.
> 
> That's right and I think that some of the callsites could actually pass
> the existing transaction to btrfs_set_prop instead of relying on the
> fallback behaviour.
> 
> Also, the potential NULL as transaction handle is not a good thing from
> the API point of view and I'd like to reduce that to minimum (there are
> some justified cases).
> 
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 3f9d7be30bf4..5a4ed2f66e09 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -284,7 +284,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>>   		binode->flags &= ~BTRFS_INODE_COMPRESS;
>>   		binode->flags |= BTRFS_INODE_NOCOMPRESS;
>>   
>> -		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
>> +		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
>> +				     0);
>>   		if (ret && ret != -ENODATA)
>>   			goto out_drop;
>>   	} else if (fsflags & FS_COMPR_FL) {
>> @@ -302,13 +303,14 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>>   		if (!comp || comp[0] == 0)
>>   			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
>>   
>> -		ret = btrfs_set_prop(inode, "btrfs.compression",
>> -				     comp, strlen(comp), 0);
>> +		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", comp,
>> +				     strlen(comp), 0);
>>   		if (ret)
>>   			goto out_drop;
>>   
>>   	} else {
>> -		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
>> +		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
>> +				     0);
>>   		if (ret && ret != -ENODATA)
>>   			goto out_drop;
>>   		binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
> 
> When the if-else block ends, there's a new transaction started, this
> seems unnecessary.
> 
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index dc6140013ae8..4525a2a4d1cd 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -85,12 +85,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
>>   	return NULL;
>>   }
>>   
>> -static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
>> -			    struct inode *inode,
>> -			    const char *name,
>> -			    const char *value,
>> -			    size_t value_len,
>> -			    int flags)
>> +int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>> +		   const char *name, const char *value, size_t value_len,
>> +		   int flags)
>>   {
>>   	const struct prop_handler *handler;
>>   	int ret;
>> @@ -133,15 +130,6 @@ static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
>>   	return 0;
>>   }
>>   
>> -int btrfs_set_prop(struct inode *inode,
>> -		   const char *name,
>> -		   const char *value,
>> -		   size_t value_len,
>> -		   int flags)
>> -{
>> -	return __btrfs_set_prop(NULL, inode, name, value, value_len, flags);
> 
> I agree that one function would be better here, with defined semantics
> of 'trans'.
> 
> There are more cleanups around the properties, also in the xattr
> handling functions. This patchset is a step in the right direction and I
> think the cleanups could be more extensive.
> 
> The idea for the xattr function:
> 
> - the VFS callbacks (like btrfs_xattr_handler_set_prop) will start the
>    transaction themselves and pass the handle to the prop function

These vfs callbacks must handle the update inode part as well, which 
btrfs_setxattr() skips if trans != NULL.

----
         inode_inc_iversion(inode);
         inode->i_ctime = current_time(inode);
         set_bit(BTRFS_INODE_COPY_EVERYTHING, 
&BTRFS_I(inode)->runtime_flags);
         ret = btrfs_update_inode(trans, root, inode);
----

Will give a try.

> - a xattr function that does not take a valid trans could be named like
>    btrfs_setxattr_notrans and will start the transaction, ie. lifting
>    that to the callers

Yep. Also btrfs_set_props_notrans() and btrfs_set_acl_notrans().

> - there's a maze of the xattr callbacks so assertions are needed

ok.

Thanks, Anand

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12  2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
2019-02-12  2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
2019-02-20 19:00   ` David Sterba
2019-02-21  9:03     ` Anand Jain
2019-02-12  2:18 ` [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c Anand Jain
2019-02-12  2:18 ` [PATCH v3 3/3] btrfs: trivial, fix c coding style Anand Jain

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox