[PATCH v4 00/10] Misc props.c cleanups

[PATCH v4 00/10] Misc props.c cleanups

[Anand Jain]

v4: More cleanups patch 4-10 were added. 4/10 is a generic cleanup.
    5-9/10 are cleanups to assist killing btrfs_setxattr(). And
    10/10 kills btrfs_setxattr().

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 (10):
  btrfs: kill __btrfs_set_prop()
  btrfs: drop redundant forward declaration in props.c
  btrfs: trivial, fix c coding style
  btrfs: rename fs_info argument to fs_private
  btrfs: refactor btrfs_set_prop add btrfs_set_prop_trans
  btrfs: start transaction in btrfs_set_prop_trans
  btrfs: start transaction in btrfs_set_acl
  btrfs: start transaction in btrfs_xattr_handler_set
  btrfs: btrfs_setxattr argument trans is never NULL
  btrfs: kill btrfs_setxattr

 fs/btrfs/acl.c   |  45 ++++++++---
 fs/btrfs/ioctl.c |  10 ++-
 fs/btrfs/props.c | 232 ++++++++++++++++++++++++++++---------------------------
 fs/btrfs/props.h |   7 +-
 fs/btrfs/xattr.c |  75 ++++++++----------
 fs/btrfs/xattr.h |   5 +-
 6 files changed, 196 insertions(+), 178 deletions(-)

-- 
1.8.3.1

[PATCH v4 01/10] btrfs: kill __btrfs_set_prop()

[Anand Jain]

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>
---
v4: none
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 494f0f10d70e..cb34532960d2 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

[PATCH v4 02/10] btrfs: drop redundant forward declaration in props.c

[Anand Jain]

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>
---
v4: none
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

[PATCH v4 03/10] btrfs: trivial, fix c coding style

[Anand Jain]

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

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v4: none.
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

Re: [PATCH v4 03/10] btrfs: trivial, fix c coding style

[David Sterba]

On Sat, Feb 23, 2019 at 01:39:45AM +0800, Anand Jain wrote:
> Maintain the lines extented upto 80 char where possible, and indent the
> argument.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

I don't think such patches are necessary, there's a lot of strange
formatting from the past and we could fix the ugly ones but otherwise
strive to fix everything before it gets merged.  I do a lot of fixups
just before the final merge, but having everybody follow the same coding
style is near to impossible.

Re: [PATCH v4 03/10] btrfs: trivial, fix c coding style

[Anand Jain]

On 2/28/19 12:05 AM, David Sterba wrote:
> On Sat, Feb 23, 2019 at 01:39:45AM +0800, Anand Jain wrote:
>> Maintain the lines extented upto 80 char where possible, and indent the
>> argument.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> I don't think such patches are necessary, there's a lot of strange
> formatting from the past and we could fix the ugly ones but otherwise
> strive to fix everything before it gets merged.  I do a lot of fixups
> just before the final merge, but having everybody follow the same coding
> style is near to impossible.
> 

Ok. Will drop this.

[PATCH v4 04/10] btrfs: rename fs_info argument to fs_private

[Anand Jain]

fs_info is commonly used to represent struct fs_info *, rename
to fs_private to avoid confusion.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born
 fs/btrfs/xattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 499bb79ba135..6971cbf286b5 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -419,10 +419,10 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
 };
 
 static int btrfs_initxattrs(struct inode *inode,
-			    const struct xattr *xattr_array, void *fs_info)
+			    const struct xattr *xattr_array, void *fs_private)
 {
+	struct btrfs_trans_handle *trans = fs_private;
 	const struct xattr *xattr;
-	struct btrfs_trans_handle *trans = fs_info;
 	unsigned int nofs_flag;
 	char *name;
 	int err = 0;
-- 
1.8.3.1

Re: [PATCH v4 04/10] btrfs: rename fs_info argument to fs_private

[David Sterba]

On Sat, Feb 23, 2019 at 01:39:46AM +0800, Anand Jain wrote:
> fs_info is commonly used to represent struct fs_info *, rename
> to fs_private to avoid confusion.

Yeah, I had the same thoughts. The parameter name in interface function
security_inode_init_security is fs_data, I think we can use that name
too.

Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH v4 05/10] btrfs: refactor btrfs_set_prop add btrfs_set_prop_trans

[Anand Jain]

btrfs_set_prop() accepts transaction pointer as the first argument,
however in ioctl.c for the purpose of setting the compression property,
we call btrfs_set_prop() with NULL transaction pointer. Down in
the call chain  btrfs_setxattr() would start transaction to update the
attribute and also to update the inode. So for better clarity, create
btrfs_set_prop_notrans() with no transaction pointer as argument.

And now btrfs_set_prop() is a static function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born

Its named btrfs_set_prop_trans() instead of btrfs_set_prop_notrans()
because in the next patch I am starting the transaction in
btrfs_set_prop_trans(). I am ok if this and the next patch are squashed
during integration, as it is separated only for the purpose of easy review.

 fs/btrfs/ioctl.c | 12 ++++++------
 fs/btrfs/props.c | 12 +++++++++---
 fs/btrfs/props.h |  5 ++---
 fs/btrfs/xattr.c |  2 +-
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index cb34532960d2..ba58adce4566 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -284,8 +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(NULL, inode, "btrfs.compression", NULL, 0,
-				     0);
+		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
+					     0, 0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
 	} else if (fsflags & FS_COMPR_FL) {
@@ -303,14 +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(NULL, inode, "btrfs.compression", comp,
-				     strlen(comp), 0);
+		ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp,
+					     strlen(comp), 0);
 		if (ret)
 			goto out_drop;
 
 	} else {
-		ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0,
-				     0);
+		ret = btrfs_set_prop_trans(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 3c15f19bfd2f..e581da1bfbb6 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -55,9 +55,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
 	return NULL;
 }
 
-int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
-		   const char *name, const char *value, size_t value_len,
-		   int flags)
+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)
 {
 	const struct prop_handler *handler;
 	int ret;
@@ -100,6 +100,12 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 	return 0;
 }
 
+int btrfs_set_prop_trans(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,
 				void (*iterator)(void *,
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 9dbdae47cf27..01b3d4bbb207 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -10,9 +10,8 @@
 
 void __init btrfs_props_init(void);
 
-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_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 6971cbf286b5..69126d5b4d62 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(NULL, inode, name, value, size, flags);
+	return btrfs_set_prop_trans(inode, name, value, size, flags);
 }
 
 static const struct xattr_handler btrfs_security_xattr_handler = {
-- 
1.8.3.1

[PATCH v4 06/10] btrfs: start transaction in btrfs_set_prop_trans

[Anand Jain]

In preparation to make trans argument of btrfs_setxattr() a mandatory,
start the transaction in btrfs_set_prop_trans.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born
 fs/btrfs/props.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index e581da1bfbb6..f878ba3160f0 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/hashtable.h>
+#include <linux/iversion.h>
 #include "props.h"
 #include "btrfs_inode.h"
 #include "transaction.h"
@@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 int btrfs_set_prop_trans(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);
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+	struct btrfs_trans_handle *trans;
+	int ret;
+
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	ret = btrfs_set_prop(trans, inode, name, value, value_len, flags);
+
+	if (!ret) {
+		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);
+		ASSERT(!ret);
+	}
+	btrfs_end_transaction(trans);
+	return ret;
 }
 
 static int iterate_object_props(struct btrfs_root *root,
-- 
1.8.3.1

Re: [PATCH v4 06/10] btrfs: start transaction in btrfs_set_prop_trans

[David Sterba]

On Sat, Feb 23, 2019 at 01:39:48AM +0800, Anand Jain wrote:
> In preparation to make trans argument of btrfs_setxattr() a mandatory,
> start the transaction in btrfs_set_prop_trans.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v4: born
>  fs/btrfs/props.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index e581da1bfbb6..f878ba3160f0 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/hashtable.h>
> +#include <linux/iversion.h>
>  #include "props.h"
>  #include "btrfs_inode.h"
>  #include "transaction.h"
> @@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>  int btrfs_set_prop_trans(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);
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_trans_handle *trans;
> +	int ret;
> +
> +	trans = btrfs_start_transaction(root, 2);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
> +	ret = btrfs_set_prop(trans, inode, name, value, value_len, flags);
> +
> +	if (!ret) {
> +		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);
> +		ASSERT(!ret);

This is not right. The previous code uses BUG_ON which is also not
right, but does not silently continue if asserts are compiled out.
Please add proper error handling here.

> +	}
> +	btrfs_end_transaction(trans);
> +	return ret;
>  }
>  
>  static int iterate_object_props(struct btrfs_root *root,
> -- 
> 1.8.3.1

Re: [PATCH v4 06/10] btrfs: start transaction in btrfs_set_prop_trans

[Anand Jain]

On 2/28/19 12:08 AM, David Sterba wrote:
> On Sat, Feb 23, 2019 at 01:39:48AM +0800, Anand Jain wrote:
>> In preparation to make trans argument of btrfs_setxattr() a mandatory,
>> start the transaction in btrfs_set_prop_trans.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v4: born
>>   fs/btrfs/props.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index e581da1bfbb6..f878ba3160f0 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -4,6 +4,7 @@
>>    */
>>   
>>   #include <linux/hashtable.h>
>> +#include <linux/iversion.h>
>>   #include "props.h"
>>   #include "btrfs_inode.h"
>>   #include "transaction.h"
>> @@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>>   int btrfs_set_prop_trans(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);
>> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>> +	struct btrfs_trans_handle *trans;
>> +	int ret;
>> +
>> +	trans = btrfs_start_transaction(root, 2);
>> +	if (IS_ERR(trans))
>> +		return PTR_ERR(trans);
>> +
>> +	ret = btrfs_set_prop(trans, inode, name, value, value_len, flags);
>> +
>> +	if (!ret) {
>> +		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);
>> +		ASSERT(!ret);
> 
> This is not right. The previous code uses BUG_ON which is also not
> right, but does not silently continue if asserts are compiled out.
> Please add proper error handling here.

  Error handling should save and undo of inode version, i_ctime and
  runtime_flags. Is a new patch for this OK? and here will use BUG_ON
  as in the original.

Thanks, Anand

>> +	}
>> +	btrfs_end_transaction(trans);
>> +	return ret;
>>   }
>>   
>>   static int iterate_object_props(struct btrfs_root *root,
>> -- 
>> 1.8.3.1

Re: [PATCH v4 06/10] btrfs: start transaction in btrfs_set_prop_trans

[David Sterba]

On Thu, Feb 28, 2019 at 06:36:40PM +0800, Anand Jain wrote:
> >> +	if (!ret) {
> >> +		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);
> >> +		ASSERT(!ret);
> > 
> > This is not right. The previous code uses BUG_ON which is also not
> > right, but does not silently continue if asserts are compiled out.
> > Please add proper error handling here.
> 
>   Error handling should save and undo of inode version, i_ctime and
>   runtime_flags. Is a new patch for this OK? and here will use BUG_ON
>   as in the original.

Ok, use BUG_ON, it's effectively only a code copy. The error handling
could be tricky here.

[PATCH v4 07/10] btrfs: start transaction in btrfs_set_acl

[Anand Jain]

The main motivation is to avoid NULL for the trans argument reaching
btrfs_set_xattr(). This patch does the following in btrfs_set_acl().
  Create transaction and handle the inode changes update.
  Rename __btrfs_set_acl to do_set_acl as this is a helper function.
  Fix a c-code style, expand function declaration to max length 80 char.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born
 fs/btrfs/acl.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 5810463dc6d2..e395615dd304 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -11,10 +11,12 @@
 #include <linux/sched.h>
 #include <linux/sched/mm.h>
 #include <linux/slab.h>
+#include <linux/iversion.h>
 
 #include "ctree.h"
 #include "btrfs_inode.h"
 #include "xattr.h"
+#include "transaction.h"
 
 struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 {
@@ -52,8 +54,8 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
-			 struct inode *inode, struct posix_acl *acl, int type)
+static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
+		      struct posix_acl *acl, int type)
 {
 	int ret, size = 0;
 	const char *name;
@@ -107,20 +109,36 @@ int btrfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int ret;
 	umode_t old_mode = inode->i_mode;
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root = BTRFS_I(inode)->root;
 
 	if (type == ACL_TYPE_ACCESS && acl) {
 		ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (ret)
 			return ret;
 	}
-	ret = __btrfs_set_acl(NULL, inode, acl, type);
-	if (ret)
+
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	ret = do_set_acl(trans, inode, acl, type);
+	if (ret) {
 		inode->i_mode = old_mode;
+	} else {
+		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);
+		ASSERT(!ret);
+	}
+
+	btrfs_end_transaction(trans);
 	return ret;
 }
 
-int btrfs_init_acl(struct btrfs_trans_handle *trans,
-		   struct inode *inode, struct inode *dir)
+int btrfs_init_acl(struct btrfs_trans_handle *trans, struct inode *inode,
+		   struct inode *dir)
 {
 	struct posix_acl *default_acl, *acl;
 	int ret = 0;
@@ -134,15 +152,13 @@ int btrfs_init_acl(struct btrfs_trans_handle *trans,
 		return ret;
 
 	if (default_acl) {
-		ret = __btrfs_set_acl(trans, inode, default_acl,
-				      ACL_TYPE_DEFAULT);
+		ret = do_set_acl(trans, inode, default_acl, ACL_TYPE_DEFAULT);
 		posix_acl_release(default_acl);
 	}
 
 	if (acl) {
 		if (!ret)
-			ret = __btrfs_set_acl(trans, inode, acl,
-					      ACL_TYPE_ACCESS);
+			ret = do_set_acl(trans, inode, acl, ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
 	}
 
-- 
1.8.3.1

[PATCH v4 08/10] btrfs: start transaction in btrfs_xattr_handler_set

[Anand Jain]

Generic extended attributes on the inode are set using
btrfs_xattr_handler_set(), and the required transaction for this update
is started by btrfs_setxattr(). For better granulation of the
transaction start and end, do this in btrfs_xattr_handler_set().

Suggested-by: David Sterba <dsterba@suse.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born
 fs/btrfs/xattr.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 69126d5b4d62..8e301a1ae304 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -369,8 +369,29 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
 				   const char *name, const void *buffer,
 				   size_t size, int flags)
 {
+	int ret;
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+
 	name = xattr_full_name(handler, name);
-	return btrfs_setxattr(NULL, inode, name, buffer, size, flags);
+
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	ret = btrfs_setxattr(trans, inode, name, buffer, size, flags);
+
+	if (!ret) {
+		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);
+		ASSERT(!ret);
+	}
+
+	btrfs_end_transaction(trans);
+
+	return ret;
 }
 
 static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
-- 
1.8.3.1

[PATCH v4 09/10] btrfs: btrfs_setxattr argument trans is never NULL

[Anand Jain]

The following patches
   btrfs: cleanup btrfs_set_acl
   btrfs: start transaction in btrfs_xattr_handler_set
   btrfs: create transaction in btrfs_set_prop_notrans

made the btrfs_setxattr() argument trans to be never NULL, so delete the
code when trans is NULL in btrfs_setxattr(). Also fix the c-code style.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born
 fs/btrfs/xattr.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 8e301a1ae304..b3281d4d95b9 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -220,35 +220,17 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
 /*
  * @value: "" makes the attribute to empty, NULL removes it
  */
-int btrfs_setxattr(struct btrfs_trans_handle *trans,
-		     struct inode *inode, const char *name,
-		     const void *value, size_t size, int flags)
+int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
+		   const char *name, const void *value, size_t size, int flags)
 {
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	int ret;
+
+	ASSERT(!trans);
 
 	if (btrfs_root_readonly(root))
 		return -EROFS;
 
-	if (trans)
-		return do_setxattr(trans, inode, name, value, size, flags);
-
-	trans = btrfs_start_transaction(root, 2);
-	if (IS_ERR(trans))
-		return PTR_ERR(trans);
-
-	ret = do_setxattr(trans, inode, name, value, size, flags);
-	if (ret)
-		goto out;
-
-	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);
-	BUG_ON(ret);
-out:
-	btrfs_end_transaction(trans);
-	return ret;
+	return do_setxattr(trans, inode, name, value, size, flags);
 }
 
 ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
-- 
1.8.3.1

Re: [PATCH v4 09/10] btrfs: btrfs_setxattr argument trans is never NULL

[David Sterba]

On Sat, Feb 23, 2019 at 01:39:51AM +0800, Anand Jain wrote:
> The following patches
>    btrfs: cleanup btrfs_set_acl
>    btrfs: start transaction in btrfs_xattr_handler_set
>    btrfs: create transaction in btrfs_set_prop_notrans
> 
> made the btrfs_setxattr() argument trans to be never NULL, so delete the
> code when trans is NULL in btrfs_setxattr(). Also fix the c-code style.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v4: born
>  fs/btrfs/xattr.c | 28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 8e301a1ae304..b3281d4d95b9 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -220,35 +220,17 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
>  /*
>   * @value: "" makes the attribute to empty, NULL removes it
>   */
> -int btrfs_setxattr(struct btrfs_trans_handle *trans,
> -		     struct inode *inode, const char *name,
> -		     const void *value, size_t size, int flags)
> +int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> +		   const char *name, const void *value, size_t size, int flags)
>  {
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	int ret;
> +
> +	ASSERT(!trans);

Isn't this reversed?

	ASSERT(trans == 0) is not what we want

Re: [PATCH v4 09/10] btrfs: btrfs_setxattr argument trans is never NULL

[Anand Jain]

On 2/28/19 12:16 AM, David Sterba wrote:
> On Sat, Feb 23, 2019 at 01:39:51AM +0800, Anand Jain wrote:
>> The following patches
>>     btrfs: cleanup btrfs_set_acl
>>     btrfs: start transaction in btrfs_xattr_handler_set
>>     btrfs: create transaction in btrfs_set_prop_notrans
>>
>> made the btrfs_setxattr() argument trans to be never NULL, so delete the
>> code when trans is NULL in btrfs_setxattr(). Also fix the c-code style.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v4: born
>>   fs/btrfs/xattr.c | 28 +++++-----------------------
>>   1 file changed, 5 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
>> index 8e301a1ae304..b3281d4d95b9 100644
>> --- a/fs/btrfs/xattr.c
>> +++ b/fs/btrfs/xattr.c
>> @@ -220,35 +220,17 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
>>   /*
>>    * @value: "" makes the attribute to empty, NULL removes it
>>    */
>> -int btrfs_setxattr(struct btrfs_trans_handle *trans,
>> -		     struct inode *inode, const char *name,
>> -		     const void *value, size_t size, int flags)
>> +int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
>> +		   const char *name, const void *value, size_t size, int flags)
>>   {
>>   	struct btrfs_root *root = BTRFS_I(inode)->root;
>> -	int ret;
>> +
>> +	ASSERT(!trans);
> 
> Isn't this reversed?
> 
> 	ASSERT(trans == 0) is not what we want

You are right. As such the intention was to delete this function as a 
whole, which happened in the next patch. Will fix.

Thanks, Anand

[PATCH v4 10/10] btrfs: kill btrfs_setxattr

[Anand Jain]

Now btrfs_setxattr() is a very small function with just check for
readonly FS and redirect the call to do_setxattr(). So instead
move that checks to the parent functions.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4: born
 fs/btrfs/acl.c   |  9 ++++++++-
 fs/btrfs/props.c | 16 ++++++++++------
 fs/btrfs/xattr.c | 30 ++++++++----------------------
 fs/btrfs/xattr.h |  5 ++---
 4 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index e395615dd304..75abe7f0d05d 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
 	int ret, size = 0;
 	const char *name;
 	char *value = NULL;
+	struct btrfs_root *root = BTRFS_I(inode)->root;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
@@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
 			goto out;
 	}
 
-	ret = btrfs_setxattr(trans, inode, name, value, size, 0);
+	if (btrfs_root_readonly(root)) {
+		ret = -EROFS;
+		goto out;
+	}
+
+	ret = do_setxattr(trans, inode, name, value, size, 0);
+
 out:
 	kfree(value);
 
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f878ba3160f0..be6d16ccc738 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -60,6 +60,7 @@ 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)
 {
+	struct btrfs_root *root = BTRFS_I(inode)->root;
 	const struct prop_handler *handler;
 	int ret;
 
@@ -70,9 +71,12 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 	if (!handler)
 		return -EINVAL;
 
+	if (btrfs_root_readonly(root))
+		return -EROFS;
+
 	if (value_len == 0) {
-		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
-				     flags);
+		ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
+				  flags);
 		if (ret)
 			return ret;
 
@@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 	ret = handler->validate(value, value_len);
 	if (ret)
 		return ret;
-	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
-			       value, value_len, flags);
+	ret = do_setxattr(trans, inode, handler->xattr_name, value, value_len,
+			  flags);
 	if (ret)
 		return ret;
 	ret = handler->apply(inode, value, value_len);
 	if (ret) {
-		btrfs_setxattr(trans, inode, handler->xattr_name,
-				 NULL, 0, flags);
+		ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
+				  flags);
 		return ret;
 	}
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index b3281d4d95b9..4e1ccfbfa44a 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -76,9 +76,8 @@ int btrfs_getxattr(struct inode *inode, const char *name,
 	return ret;
 }
 
-static int do_setxattr(struct btrfs_trans_handle *trans,
-		       struct inode *inode, const char *name,
-		       const void *value, size_t size, int flags)
+int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
+		const char *name, const void *value, size_t size, int flags)
 {
 	struct btrfs_dir_item *di = NULL;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -217,22 +216,6 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-/*
- * @value: "" makes the attribute to empty, NULL removes it
- */
-int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
-		   const char *name, const void *value, size_t size, int flags)
-{
-	struct btrfs_root *root = BTRFS_I(inode)->root;
-
-	ASSERT(!trans);
-
-	if (btrfs_root_readonly(root))
-		return -EROFS;
-
-	return do_setxattr(trans, inode, name, value, size, flags);
-}
-
 ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
 	struct btrfs_key key;
@@ -357,11 +340,14 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
 
 	name = xattr_full_name(handler, name);
 
+	if (btrfs_root_readonly(root))
+		return -EROFS;
+
 	trans = btrfs_start_transaction(root, 2);
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	ret = btrfs_setxattr(trans, inode, name, buffer, size, flags);
+	ret = do_setxattr(trans, inode, name, buffer, size, flags);
 
 	if (!ret) {
 		inode_inc_iversion(inode);
@@ -444,8 +430,8 @@ static int btrfs_initxattrs(struct inode *inode,
 		}
 		strcpy(name, XATTR_SECURITY_PREFIX);
 		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
-		err = btrfs_setxattr(trans, inode, name, xattr->value,
-				xattr->value_len, 0);
+		err = do_setxattr(trans, inode, name, xattr->value,
+				  xattr->value_len, 0);
 		kfree(name);
 		if (err < 0)
 			break;
diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
index 471fcac6ff55..276f81a622d5 100644
--- a/fs/btrfs/xattr.h
+++ b/fs/btrfs/xattr.h
@@ -12,9 +12,8 @@
 
 int btrfs_getxattr(struct inode *inode, const char *name,
 		void *buffer, size_t size);
-int btrfs_setxattr(struct btrfs_trans_handle *trans,
-			    struct inode *inode, const char *name,
-			    const void *value, size_t size, int flags);
+int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
+		const char *name, const void *value, size_t size, int flags);
 ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size);
 
 int btrfs_xattr_security_init(struct btrfs_trans_handle *trans,
-- 
1.8.3.1

Re: [PATCH v4 10/10] btrfs: kill btrfs_setxattr

[David Sterba]

On Sat, Feb 23, 2019 at 01:39:52AM +0800, Anand Jain wrote:
> Now btrfs_setxattr() is a very small function with just check for
> readonly FS and redirect the call to do_setxattr(). So instead
> move that checks to the parent functions.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v4: born
>  fs/btrfs/acl.c   |  9 ++++++++-
>  fs/btrfs/props.c | 16 ++++++++++------
>  fs/btrfs/xattr.c | 30 ++++++++----------------------
>  fs/btrfs/xattr.h |  5 ++---
>  4 files changed, 28 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index e395615dd304..75abe7f0d05d 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
>  	int ret, size = 0;
>  	const char *name;
>  	char *value = NULL;
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>  
>  	switch (type) {
>  	case ACL_TYPE_ACCESS:
> @@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
>  			goto out;
>  	}
>  
> -	ret = btrfs_setxattr(trans, inode, name, value, size, 0);
> +	if (btrfs_root_readonly(root)) {
> +		ret = -EROFS;
> +		goto out;
> +	}
> +
> +	ret = do_setxattr(trans, inode, name, value, size, 0);
> +
>  out:
>  	kfree(value);
>  
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index f878ba3160f0..be6d16ccc738 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -60,6 +60,7 @@ 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)
>  {
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	const struct prop_handler *handler;
>  	int ret;
>  
> @@ -70,9 +71,12 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>  	if (!handler)
>  		return -EINVAL;
>  
> +	if (btrfs_root_readonly(root))
> +		return -EROFS;
> +
>  	if (value_len == 0) {
> -		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> -				     flags);
> +		ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> +				  flags);
>  		if (ret)
>  			return ret;
>  
> @@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>  	ret = handler->validate(value, value_len);
>  	if (ret)
>  		return ret;
> -	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
> -			       value, value_len, flags);
> +	ret = do_setxattr(trans, inode, handler->xattr_name, value, value_len,
> +			  flags);
>  	if (ret)
>  		return ret;
>  	ret = handler->apply(inode, value, value_len);
>  	if (ret) {
> -		btrfs_setxattr(trans, inode, handler->xattr_name,
> -				 NULL, 0, flags);
> +		ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> +				  flags);
>  		return ret;
>  	}
>  
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index b3281d4d95b9..4e1ccfbfa44a 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -76,9 +76,8 @@ int btrfs_getxattr(struct inode *inode, const char *name,
>  	return ret;
>  }
>  
> -static int do_setxattr(struct btrfs_trans_handle *trans,
> -		       struct inode *inode, const char *name,
> -		       const void *value, size_t size, int flags)
> +int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> +		const char *name, const void *value, size_t size, int flags)
>  {
>  	struct btrfs_dir_item *di = NULL;
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> @@ -217,22 +216,6 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -/*
> - * @value: "" makes the attribute to empty, NULL removes it
> - */
> -int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> -		   const char *name, const void *value, size_t size, int flags)
> -{
> -	struct btrfs_root *root = BTRFS_I(inode)->root;
> -
> -	ASSERT(!trans);
> -
> -	if (btrfs_root_readonly(root))
> -		return -EROFS;
> -
> -	return do_setxattr(trans, inode, name, value, size, flags);
> -}
> -
>  ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
>  {
>  	struct btrfs_key key;
> @@ -357,11 +340,14 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
>  
>  	name = xattr_full_name(handler, name);
>  
> +	if (btrfs_root_readonly(root))
> +		return -EROFS;
> +
>  	trans = btrfs_start_transaction(root, 2);
>  	if (IS_ERR(trans))
>  		return PTR_ERR(trans);
>  
> -	ret = btrfs_setxattr(trans, inode, name, buffer, size, flags);
> +	ret = do_setxattr(trans, inode, name, buffer, size, flags);
>  
>  	if (!ret) {
>  		inode_inc_iversion(inode);
> @@ -444,8 +430,8 @@ static int btrfs_initxattrs(struct inode *inode,
>  		}
>  		strcpy(name, XATTR_SECURITY_PREFIX);
>  		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
> -		err = btrfs_setxattr(trans, inode, name, xattr->value,
> -				xattr->value_len, 0);
> +		err = do_setxattr(trans, inode, name, xattr->value,
> +				  xattr->value_len, 0);
>  		kfree(name);
>  		if (err < 0)
>  			break;
> diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
> index 471fcac6ff55..276f81a622d5 100644
> --- a/fs/btrfs/xattr.h
> +++ b/fs/btrfs/xattr.h
> @@ -12,9 +12,8 @@
>  
>  int btrfs_getxattr(struct inode *inode, const char *name,
>  		void *buffer, size_t size);
> -int btrfs_setxattr(struct btrfs_trans_handle *trans,
> -			    struct inode *inode, const char *name,
> -			    const void *value, size_t size, int flags);
> +int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> +		const char *name, const void *value, size_t size, int flags);

No, this is backwards. Why do you promote a static helper to interface
function but without the btrfs_ prefix?

The point of do_something functions is to actually do the thing assuming
that external conditions have been checked, like the readonly status.
All such checks should happen at the entry, ie. beginning of the various
callbacks.

Re: [PATCH v4 10/10] btrfs: kill btrfs_setxattr

[Anand Jain]

On 2/28/19 12:21 AM, David Sterba wrote:
> On Sat, Feb 23, 2019 at 01:39:52AM +0800, Anand Jain wrote:
>> Now btrfs_setxattr() is a very small function with just check for
>> readonly FS and redirect the call to do_setxattr(). So instead
>> move that checks to the parent functions.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v4: born
>>   fs/btrfs/acl.c   |  9 ++++++++-
>>   fs/btrfs/props.c | 16 ++++++++++------
>>   fs/btrfs/xattr.c | 30 ++++++++----------------------
>>   fs/btrfs/xattr.h |  5 ++---
>>   4 files changed, 28 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
>> index e395615dd304..75abe7f0d05d 100644
>> --- a/fs/btrfs/acl.c
>> +++ b/fs/btrfs/acl.c
>> @@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
>>   	int ret, size = 0;
>>   	const char *name;
>>   	char *value = NULL;
>> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>>   
>>   	switch (type) {
>>   	case ACL_TYPE_ACCESS:
>> @@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
>>   			goto out;
>>   	}
>>   
>> -	ret = btrfs_setxattr(trans, inode, name, value, size, 0);
>> +	if (btrfs_root_readonly(root)) {
>> +		ret = -EROFS;
>> +		goto out;
>> +	}
>> +
>> +	ret = do_setxattr(trans, inode, name, value, size, 0);
>> +
>>   out:
>>   	kfree(value);
>>   
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index f878ba3160f0..be6d16ccc738 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -60,6 +60,7 @@ 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)
>>   {
>> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>>   	const struct prop_handler *handler;
>>   	int ret;
>>   
>> @@ -70,9 +71,12 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>>   	if (!handler)
>>   		return -EINVAL;
>>   
>> +	if (btrfs_root_readonly(root))
>> +		return -EROFS;
>> +
>>   	if (value_len == 0) {
>> -		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
>> -				     flags);
>> +		ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
>> +				  flags);
>>   		if (ret)
>>   			return ret;
>>   
>> @@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>>   	ret = handler->validate(value, value_len);
>>   	if (ret)
>>   		return ret;
>> -	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
>> -			       value, value_len, flags);
>> +	ret = do_setxattr(trans, inode, handler->xattr_name, value, value_len,
>> +			  flags);
>>   	if (ret)
>>   		return ret;
>>   	ret = handler->apply(inode, value, value_len);
>>   	if (ret) {
>> -		btrfs_setxattr(trans, inode, handler->xattr_name,
>> -				 NULL, 0, flags);
>> +		ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
>> +				  flags);
>>   		return ret;
>>   	}
>>   
>> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
>> index b3281d4d95b9..4e1ccfbfa44a 100644
>> --- a/fs/btrfs/xattr.c
>> +++ b/fs/btrfs/xattr.c
>> @@ -76,9 +76,8 @@ int btrfs_getxattr(struct inode *inode, const char *name,
>>   	return ret;
>>   }
>>   
>> -static int do_setxattr(struct btrfs_trans_handle *trans,
>> -		       struct inode *inode, const char *name,
>> -		       const void *value, size_t size, int flags)
>> +int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
>> +		const char *name, const void *value, size_t size, int flags)
>>   {
>>   	struct btrfs_dir_item *di = NULL;
>>   	struct btrfs_root *root = BTRFS_I(inode)->root;
>> @@ -217,22 +216,6 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
>>   	return ret;
>>   }
>>   
>> -/*
>> - * @value: "" makes the attribute to empty, NULL removes it
>> - */
>> -int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
>> -		   const char *name, const void *value, size_t size, int flags)
>> -{
>> -	struct btrfs_root *root = BTRFS_I(inode)->root;
>> -
>> -	ASSERT(!trans);
>> -
>> -	if (btrfs_root_readonly(root))
>> -		return -EROFS;
>> -
>> -	return do_setxattr(trans, inode, name, value, size, flags);
>> -}
>> -
>>   ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>   {
>>   	struct btrfs_key key;
>> @@ -357,11 +340,14 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
>>   
>>   	name = xattr_full_name(handler, name);
>>   
>> +	if (btrfs_root_readonly(root))
>> +		return -EROFS;
>> +
>>   	trans = btrfs_start_transaction(root, 2);
>>   	if (IS_ERR(trans))
>>   		return PTR_ERR(trans);
>>   
>> -	ret = btrfs_setxattr(trans, inode, name, buffer, size, flags);
>> +	ret = do_setxattr(trans, inode, name, buffer, size, flags);
>>   
>>   	if (!ret) {
>>   		inode_inc_iversion(inode);
>> @@ -444,8 +430,8 @@ static int btrfs_initxattrs(struct inode *inode,
>>   		}
>>   		strcpy(name, XATTR_SECURITY_PREFIX);
>>   		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
>> -		err = btrfs_setxattr(trans, inode, name, xattr->value,
>> -				xattr->value_len, 0);
>> +		err = do_setxattr(trans, inode, name, xattr->value,
>> +				  xattr->value_len, 0);
>>   		kfree(name);
>>   		if (err < 0)
>>   			break;
>> diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
>> index 471fcac6ff55..276f81a622d5 100644
>> --- a/fs/btrfs/xattr.h
>> +++ b/fs/btrfs/xattr.h
>> @@ -12,9 +12,8 @@
>>   
>>   int btrfs_getxattr(struct inode *inode, const char *name,
>>   		void *buffer, size_t size);
>> -int btrfs_setxattr(struct btrfs_trans_handle *trans,
>> -			    struct inode *inode, const char *name,
>> -			    const void *value, size_t size, int flags);
>> +int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
>> +		const char *name, const void *value, size_t size, int flags);
> 
> No, this is backwards. Why do you promote a static helper to interface
> function but without the btrfs_ prefix?
> 
> The point of do_something functions is to actually do the thing assuming
> that external conditions have been checked, like the readonly status.
> All such checks should happen at the entry, ie. beginning of the various
> callbacks.

  I though so as well. Will rename.

Thanks, Anand