[PATCH v5 0/9] Misc props.c cleanups

[PATCH v5 0/9] Misc props.c cleanups

[Anand Jain]

v5: drops patch
    [PATCH v4 03/10] btrfs: trivial, fix c coding style
    Chanegs are mainly to use BUG_ON instead of ASSERT as in the 
     original code in patches 5-7/9. Fix ASSERT in 8/9. And add
     a rename in 9/9. Each individual patches as the details of
     the changes.

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 (9):
  btrfs: kill __btrfs_set_prop()
  btrfs: drop redundant forward declaration in props.c
  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   |  43 ++++++++---
 fs/btrfs/ioctl.c |  10 ++-
 fs/btrfs/props.c | 221 +++++++++++++++++++++++++++++--------------------------
 fs/btrfs/props.h |   7 +-
 fs/btrfs/xattr.c |  76 +++++++++----------
 fs/btrfs/xattr.h |   5 +-
 6 files changed, 194 insertions(+), 168 deletions(-)

-- 
1.8.3.1

[PATCH v5 1/9] btrfs: kill __btrfs_set_prop()

[Anand Jain]

btrfs_set_prop() is a redirect to __btrfs_set_prop() with the
transaction handler equal to NULL.  __btrfs_set_prop() inturn passes this
NULL trans to do_setxattr() which then transaction is actually created.

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>
---
v5: none
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 v5 2/9] 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>
---
v5: none
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 v5 3/9] 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>
Reviewed-by: David Sterba <dsterba@suse.com>
---
v5: none
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

[PATCH v5 4/9] 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() starts transaction to update the
attribute and also to update the inode. So for better clarity, create
btrfs_set_prop_trans() with no transaction pointer as argument, in
preparation to start transaction here instead of doing it down the
call chain at btrfs_setxattr().

Also now the btrfs_set_prop() is a static function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v5: change log updated, fix c-style.
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 77a03076b18e..ca4af745fb90 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,
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 v5 5/9] 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(). And pass the started
transaction to btrfs_set_prop() instead of NULL. This patch copies the
inode update part from btrfs_setxattr() to btrfs_set_prop_trans(),
as it is from the original code.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v5: fix c-style. Used BUG_ON, instead of ASSERT. Change log update.
v4: born
 fs/btrfs/props.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index ca4af745fb90..75a68d7d39a2 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,26 @@ 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);
+		BUG_ON(ret);
+	}
+	btrfs_end_transaction(trans);
+	return ret;
 }
 
 static int iterate_object_props(struct btrfs_root *root,
-- 
1.8.3.1

[PATCH v5 6/9] 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, (copies code
    from btrfs_setxattr() as in the original, which needs a fix).
  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>
---
v5: change log update. Use BUG_ON instead of ASSERT.
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..2b9c3394fc34 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);
+		BUG_ON(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 v5 7/9] 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().
For which this patch copied code of btrfs_setxattr() as it is in the
original, which needs a fix later.

Suggested-by: David Sterba <dsterba@suse.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v5: Use BUG_ON instead of ASSERT. Changelog update.
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..e96b0096eaca 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);
+		BUG_ON(ret);
+	}
+
+	btrfs_end_transaction(trans);
+
+	return ret;
 }
 
 static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
-- 
1.8.3.1

[PATCH v5 8/9] btrfs: btrfs_setxattr argument trans is never NULL

[Anand Jain]

The following patches
	btrfs: start transaction in btrfs_xattr_handler_set
	btrfs: start transaction in btrfs_set_acl
	btrfs: start transaction in btrfs_set_prop_trans

brought changes so that btrfs_setxattr() argument trans is never NULL, so
delete the part of the code when trans is NULL. Also fix the c-code style.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v5: Fix ASSERT, assert on trans == NULL. Fix change log.
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 e96b0096eaca..2cbde7cac14b 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

[PATCH v5 9/9] 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 and call do_setxattr()
directly.  Delete original btrfs_setxattr(), and rename do_setxattr()
to btrfs_setxattr(). Also add few c-style. Kindly note the arguments
of original do_setxattr() and original btrfs_setxattr() are same, so the
diff obliterates the changes as described above.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style. 
v4: born
 fs/btrfs/acl.c   |  7 +++++++
 fs/btrfs/props.c | 16 ++++++++++------
 fs/btrfs/xattr.c | 29 +++++++++--------------------
 fs/btrfs/xattr.h |  5 ++---
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 2b9c3394fc34..a8a1060c8cbe 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;
 	}
 
+	if (btrfs_root_readonly(root)) {
+		ret = -EROFS;
+		goto out;
+	}
+
 	ret = btrfs_setxattr(trans, inode, name, value, size, 0);
+
 out:
 	kfree(value);
 
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 75a68d7d39a2..fa85a97fa13e 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 = btrfs_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 = btrfs_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 = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
+				     flags);
 		return ret;
 	}
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 2cbde7cac14b..358e6aed01ca 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -76,9 +76,11 @@ 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)
+/*
+ * @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_dir_item *di = NULL;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -217,22 +219,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,6 +343,9 @@ 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);
@@ -445,7 +434,7 @@ 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);
+				     xattr->value_len, 0);
 		kfree(name);
 		if (err < 0)
 			break;
diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
index 471fcac6ff55..26ff6c11774f 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 btrfs_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 v5 4/9] btrfs: refactor btrfs_set_prop add btrfs_set_prop_trans

[David Sterba]

On Fri, Mar 01, 2019 at 12:34:50PM +0800, Anand Jain wrote:
> 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() starts transaction to update the
> attribute and also to update the inode. So for better clarity, create
> btrfs_set_prop_trans() with no transaction pointer as argument, in
> preparation to start transaction here instead of doing it down the
> call chain at btrfs_setxattr().
> 
> Also now the btrfs_set_prop() is a static function.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v5: change log updated, fix c-style.
> 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().

That's right, it should be _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.

No, the patch separation does make it easier to review.

Re: [PATCH v5 6/9] btrfs: start transaction in btrfs_set_acl

[David Sterba]

On Fri, Mar 01, 2019 at 12:34:52PM +0800, Anand Jain wrote:
> 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, (copies code
>     from btrfs_setxattr() as in the original, which needs a fix).
>   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.

You don't need to mention fixing the coding style unless it's the sole
purpose of the patch.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v5: change log update. Use BUG_ON instead of ASSERT.
> v4: born
>  fs/btrfs/acl.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> -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)

Please don't do such drive-by changes, there's no difference but one has
still make sure there is none and that it's not an accidental edit.
It's ok if you rename or move a function to fix the indentation but
otherwise I have to review it and then undo it.

Re: [PATCH v5 9/9] btrfs: kill btrfs_setxattr

[David Sterba]

On Fri, Mar 01, 2019 at 12:34:55PM +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 and call do_setxattr()
> directly.  Delete original btrfs_setxattr(), and rename do_setxattr()
> to btrfs_setxattr(). Also add few c-style. Kindly note the arguments
> of original do_setxattr() and original btrfs_setxattr() are same, so the
> diff obliterates the changes as described above.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style. 
> v4: born
>  fs/btrfs/acl.c   |  7 +++++++
>  fs/btrfs/props.c | 16 ++++++++++------
>  fs/btrfs/xattr.c | 29 +++++++++--------------------
>  fs/btrfs/xattr.h |  5 ++---
>  4 files changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 2b9c3394fc34..a8a1060c8cbe 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;
>  	}
>  
> +	if (btrfs_root_readonly(root)) {
> +		ret = -EROFS;
> +		goto out;

All the readonly checks needs to go first as it's the global condition,
the following checks make sure that the arguments are valid and depend
on the previous.

> -		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
> -				       NULL, 0, flags);
> +		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> +				     flags);

drive-by change

>  		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 = btrfs_setxattr(trans, inode, handler->xattr_name, value,
> +			     value_len, flags);

same

>  	if (ret)
>  		return ret;
>  	ret = handler->apply(inode, value, value_len);
>  	if (ret) {
> -		btrfs_setxattr(trans, inode, handler->xattr_name,
> -				 NULL, 0, flags);
> +		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> +				     flags);

And that one silently changes the return value semantics but looks like
the other two, "just fixing the indentation". The original code does not
set 'ret' as the whole operation returns what the property handler
returned.

The setxattr call here resets the property. If this is not the
right, then fixed separately.

Re: [PATCH v5 0/9] Misc props.c cleanups

[David Sterba]

On Fri, Mar 01, 2019 at 12:34:46PM +0800, Anand Jain wrote:
> v5: drops patch
>     [PATCH v4 03/10] btrfs: trivial, fix c coding style
>     Chanegs are mainly to use BUG_ON instead of ASSERT as in the 
>      original code in patches 5-7/9. Fix ASSERT in 8/9. And add
>      a rename in 9/9. Each individual patches as the details of
>      the changes.

The cleanup makes it easier to follow and the transaction semantics is
more visible, good.

I've fixed up the small things commented under the patches, no need to
resend. Patchset added to 5.2 queue, thanks.

Re: [PATCH v5 0/9] Misc props.c cleanups

[Anand Jain]

On 3/8/19 11:01 PM, David Sterba wrote:
> On Fri, Mar 01, 2019 at 12:34:46PM +0800, Anand Jain wrote:
>> v5: drops patch
>>      [PATCH v4 03/10] btrfs: trivial, fix c coding style
>>      Chanegs are mainly to use BUG_ON instead of ASSERT as in the
>>       original code in patches 5-7/9. Fix ASSERT in 8/9. And add
>>       a rename in 9/9. Each individual patches as the details of
>>       the changes.
> 
> The cleanup makes it easier to follow and the transaction semantics is
> more visible, good.
> 
> I've fixed up the small things commented under the patches, no need to
> resend. Patchset added to 5.2 queue, thanks.
> 

Thanks for the feedback, review and integration.

Just checked these branches scrub-misc, misc-next, for-next-20190307
any idea which branch these patches went to.

Thanks, Anand

Re: [PATCH v5 9/9] btrfs: kill btrfs_setxattr

[Anand Jain]

On 3/8/19 10:56 PM, David Sterba wrote:
> On Fri, Mar 01, 2019 at 12:34:55PM +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 and call do_setxattr()
>> directly.  Delete original btrfs_setxattr(), and rename do_setxattr()
>> to btrfs_setxattr(). Also add few c-style. Kindly note the arguments
>> of original do_setxattr() and original btrfs_setxattr() are same, so the
>> diff obliterates the changes as described above.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style.
>> v4: born
>>   fs/btrfs/acl.c   |  7 +++++++
>>   fs/btrfs/props.c | 16 ++++++++++------
>>   fs/btrfs/xattr.c | 29 +++++++++--------------------
>>   fs/btrfs/xattr.h |  5 ++---
>>   4 files changed, 28 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
>> index 2b9c3394fc34..a8a1060c8cbe 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;
>>   	}
>>   
>> +	if (btrfs_root_readonly(root)) {
>> +		ret = -EROFS;
>> +		goto out;
> 
> All the readonly checks needs to go first as it's the global condition,
> the following checks make sure that the arguments are valid and depend
> on the previous.
> 
>> -		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
>> -				       NULL, 0, flags);
>> +		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
>> +				     flags);
> 
> drive-by change
> 
>>   		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 = btrfs_setxattr(trans, inode, handler->xattr_name, value,
>> +			     value_len, flags);
> 
> same
> 
>>   	if (ret)
>>   		return ret;
>>   	ret = handler->apply(inode, value, value_len);
>>   	if (ret) {
>> -		btrfs_setxattr(trans, inode, handler->xattr_name,
>> -				 NULL, 0, flags);
>> +		ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
>> +				     flags);
>
> And that one silently changes the return value semantics but looks like
> the other two, "just fixing the indentation". The original code does not
> set 'ret' as the whole operation returns what the property handler
> returned.
> 
> The setxattr call here resets the property. If this is not the
> right, then fixed separately.

  That's copy and paste error. :-(. It should return ret of the
  handler->apply(). Because if the undo part which is
  btrfs_setxattr() is successful it means we fail silently.
  But at this place the handler->apply() can not fail. So the
  fail part is only theoretical.

  Also, theoretically in the original code the undo part is bit wrong,
  instead of resetting to the NULL it should reset back to the
  old value.

  I was trying to read the patch which is integrated if you
  have made any changes, so that I can send the fix. But I
  can't seems to find them.

Thanks, Anand

Re: [PATCH v5 0/9] Misc props.c cleanups

[David Sterba]

On Sat, Mar 09, 2019 at 08:04:14AM +0800, Anand Jain wrote:
> On 3/8/19 11:01 PM, David Sterba wrote:
> > On Fri, Mar 01, 2019 at 12:34:46PM +0800, Anand Jain wrote:
> >> v5: drops patch
> >>      [PATCH v4 03/10] btrfs: trivial, fix c coding style
> >>      Chanegs are mainly to use BUG_ON instead of ASSERT as in the
> >>       original code in patches 5-7/9. Fix ASSERT in 8/9. And add
> >>       a rename in 9/9. Each individual patches as the details of
> >>       the changes.
> > 
> > The cleanup makes it easier to follow and the transaction semantics is
> > more visible, good.
> > 
> > I've fixed up the small things commented under the patches, no need to
> > resend. Patchset added to 5.2 queue, thanks.
> > 
> 
> Thanks for the feedback, review and integration.
> 
> Just checked these branches scrub-misc, misc-next, for-next-20190307
> any idea which branch these patches went to.

It's in a local topic branch and will appear in for-next once the merge
window is over. I've pushed it out now as ext/anand/cleanup-props-v5 so
you can have a look.

Re: [PATCH v5 0/9] Misc props.c cleanups

[Anand Jain]

On 3/11/19 10:41 PM, David Sterba wrote:
> On Sat, Mar 09, 2019 at 08:04:14AM +0800, Anand Jain wrote:
>> On 3/8/19 11:01 PM, David Sterba wrote:
>>> On Fri, Mar 01, 2019 at 12:34:46PM +0800, Anand Jain wrote:
>>>> v5: drops patch
>>>>       [PATCH v4 03/10] btrfs: trivial, fix c coding style
>>>>       Chanegs are mainly to use BUG_ON instead of ASSERT as in the
>>>>        original code in patches 5-7/9. Fix ASSERT in 8/9. And add
>>>>        a rename in 9/9. Each individual patches as the details of
>>>>        the changes.
>>>
>>> The cleanup makes it easier to follow and the transaction semantics is
>>> more visible, good.
>>>
>>> I've fixed up the small things commented under the patches, no need to
>>> resend. Patchset added to 5.2 queue, thanks.
>>>
>>
>> Thanks for the feedback, review and integration.
>>
>> Just checked these branches scrub-misc, misc-next, for-next-20190307
>> any idea which branch these patches went to.
> 
> It's in a local topic branch and will appear in for-next once the merge
> window is over. I've pushed it out now as ext/anand/cleanup-props-v5 so
> you can have a look.

Thanks. Patches [1-9]/9 Looks good.

Re: [PATCH v5 0/9] Misc props.c cleanups

[David Sterba]

On Tue, Mar 12, 2019 at 07:12:24PM +0800, Anand Jain wrote:
> 
> 
> On 3/11/19 10:41 PM, David Sterba wrote:
> > On Sat, Mar 09, 2019 at 08:04:14AM +0800, Anand Jain wrote:
> >> On 3/8/19 11:01 PM, David Sterba wrote:
> >>> On Fri, Mar 01, 2019 at 12:34:46PM +0800, Anand Jain wrote:
> >>>> v5: drops patch
> >>>>       [PATCH v4 03/10] btrfs: trivial, fix c coding style
> >>>>       Chanegs are mainly to use BUG_ON instead of ASSERT as in the
> >>>>        original code in patches 5-7/9. Fix ASSERT in 8/9. And add
> >>>>        a rename in 9/9. Each individual patches as the details of
> >>>>        the changes.
> >>>
> >>> The cleanup makes it easier to follow and the transaction semantics is
> >>> more visible, good.
> >>>
> >>> I've fixed up the small things commented under the patches, no need to
> >>> resend. Patchset added to 5.2 queue, thanks.
> >>>
> >>
> >> Thanks for the feedback, review and integration.
> >>
> >> Just checked these branches scrub-misc, misc-next, for-next-20190307
> >> any idea which branch these patches went to.
> > 
> > It's in a local topic branch and will appear in for-next once the merge
> > window is over. I've pushed it out now as ext/anand/cleanup-props-v5 so
> > you can have a look.
> 
> Thanks. Patches [1-9]/9 Looks good.

Thanks, added to misc-5.2