[PATCH 0/6] cleanup property and extended attribute set

[PATCH 0/6] cleanup property and extended attribute set

[Anand Jain]

In an attempt to stream line the property and extended attribute set here
are the few cleanup patches.

 1/6 to 3/6 are mostly non functional cleanups (except for the conversion
  to non static function in 3/6) and can be merged together.
 4/6 removes the readonly root check in btrfs_setxattr() more details in
  the change log.
 5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
  threads with transaction and without transaction respectively, so this
  patch uses them.
 6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
  now btrfs_setxattr_trans() can drop the trans arg.

Anand Jain (6):
  btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
  btrfs: rename do_setxattr to btrfs_setxattr
  btrfs: declare btrfs_setxattr as a non static function
  btrfs: remove redundant readonly root check in btrfs_setxattr_trans
  btrfs: split thread with trans to use btrfs_setxattr
  btrfs: cleanup btrfs_setxattr_trans drop trans arg

 fs/btrfs/acl.c   |  6 +++++-
 fs/btrfs/props.c | 25 +++++++++++++++++++------
 fs/btrfs/xattr.c | 25 ++++++++++---------------
 fs/btrfs/xattr.h |  7 ++++---
 4 files changed, 38 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH 1/6] btrfs: rename btrfs_setxattr to btrfs_setxattr_trans

[Anand Jain]

Rename btrfs_setxattr() to btrfs_setxattr_trans(), so that do_setxattr()
can be renamed to btrfs_setxattr().
Preparatory patch, no functional changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/acl.c   |  2 +-
 fs/btrfs/props.c | 20 ++++++++++----------
 fs/btrfs/xattr.c | 12 ++++++------
 fs/btrfs/xattr.h |  2 +-
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 5810463dc6d2..d3b04c6abc61 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -93,7 +93,7 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
 			goto out;
 	}
 
-	ret = btrfs_setxattr(trans, inode, name, value, size, 0);
+	ret = btrfs_setxattr_trans(trans, inode, name, value, size, 0);
 out:
 	kfree(value);
 
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 0d1c3485c098..61ced0ebb5ba 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -70,8 +70,8 @@ static 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(trans, inode, handler->xattr_name,
+					   NULL, 0, flags);
 		if (ret)
 			return ret;
 
@@ -84,14 +84,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(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);
+		btrfs_setxattr_trans(trans, inode, handler->xattr_name,
+				     NULL, 0, flags);
 		return ret;
 	}
 
@@ -358,13 +358,13 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		if (ret)
 			return ret;
 
-		ret = btrfs_setxattr(trans, inode, h->xattr_name, value,
-				     strlen(value), 0);
+		ret = btrfs_setxattr_trans(trans, inode, h->xattr_name, value,
+					   strlen(value), 0);
 		if (!ret) {
 			ret = h->apply(inode, value, strlen(value));
 			if (ret)
-				btrfs_setxattr(trans, inode, h->xattr_name,
-					       NULL, 0, 0);
+				btrfs_setxattr_trans(trans, inode, h->xattr_name,
+						     NULL, 0, 0);
 			else
 				set_bit(BTRFS_INODE_HAS_PROPS,
 					&BTRFS_I(inode)->runtime_flags);
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index fa820c56ba3e..38eb78aac0a7 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -220,9 +220,9 @@ 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_trans(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;
@@ -370,7 +370,7 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
 				   size_t size, int flags)
 {
 	name = xattr_full_name(handler, name);
-	return btrfs_setxattr(NULL, inode, name, buffer, size, flags);
+	return btrfs_setxattr_trans(NULL, inode, name, buffer, size, flags);
 }
 
 static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
@@ -441,8 +441,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 = btrfs_setxattr_trans(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..36d54a15cbfe 100644
--- a/fs/btrfs/xattr.h
+++ b/fs/btrfs/xattr.h
@@ -12,7 +12,7 @@ extern const struct xattr_handler *btrfs_xattr_handlers[];
 
 int btrfs_getxattr(struct inode *inode, const char *name,
 		void *buffer, size_t size);
-int btrfs_setxattr(struct btrfs_trans_handle *trans,
+int btrfs_setxattr_trans(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);
-- 
2.17.1

[PATCH 2/6] btrfs: rename do_setxattr to btrfs_setxattr

[Anand Jain]

When trans is not NULL btrfs_setxattr() calls do_setxattr()  directly
with a check for readonly root. Rename do_setxattr() btrfs_setxattr() in
preparation to call do_setxattr() directly instead.
Preparatory patch, no functional changes.

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

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 38eb78aac0a7..fd1469ef55d6 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -76,9 +76,9 @@ 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)
+static 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;
@@ -231,13 +231,13 @@ int btrfs_setxattr_trans(struct btrfs_trans_handle *trans,
 		return -EROFS;
 
 	if (trans)
-		return do_setxattr(trans, inode, name, value, size, flags);
+		return btrfs_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);
+	ret = btrfs_setxattr(trans, inode, name, value, size, flags);
 	if (ret)
 		goto out;
 
-- 
2.17.1

[PATCH 3/6] btrfs: declare btrfs_setxattr as a non static function

[Anand Jain]

Preparatory patch, as we are going split the threads with transaction and
threads without transaction to use the respective btrfs_setxattr() and
btrfs_setxattr_trans() functions. Declare btrfs_setxattr() as non static
function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/xattr.c | 5 ++---
 fs/btrfs/xattr.h | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index fd1469ef55d6..b2b68676ec52 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 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_dir_item *di = NULL;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
index 36d54a15cbfe..a95834cc3c04 100644
--- a/fs/btrfs/xattr.h
+++ b/fs/btrfs/xattr.h
@@ -12,6 +12,8 @@ extern const struct xattr_handler *btrfs_xattr_handlers[];
 
 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_trans(struct btrfs_trans_handle *trans,
 			    struct inode *inode, const char *name,
 			    const void *value, size_t size, int flags);
-- 
2.17.1

[PATCH 4/6] btrfs: remove redundant readonly root check in btrfs_setxattr_trans

[Anand Jain]

btrfs_setxattr_trans() is called by 5 threads as below and all of them are
write based threads, which won't run for a readonly root. So its ok to
remove the readonly root check here.

1.
__btrfs_set_acl()
btrfs_init_acl()
btrfs_init_inode_security()

2.
__btrfs_set_acl()
btrfs_set_acl()

3.
btrfs_set_prop()
btrfs_set_prop_trans()
  /                   \
btrfs_ioctl_setflags()   btrfs_xattr_handler_set_prop()

4.
btrfs_xattr_handler_set()

5.
btrfs_initxattrs()
btrfs_xattr_security_init()
btrfs_init_inode_security()

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

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index b2b68676ec52..4c447b1f32e5 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -226,9 +226,6 @@ int btrfs_setxattr_trans(struct btrfs_trans_handle *trans,
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	int ret;
 
-	if (btrfs_root_readonly(root))
-		return -EROFS;
-
 	if (trans)
 		return btrfs_setxattr(trans, inode, name, value, size, flags);
 
-- 
2.17.1

[PATCH 5/6] btrfs: split thread with trans to use btrfs_setxattr

[Anand Jain]

When the parent thread has already created the transaction call
btrfs_setxattr() directly. Also adds assert in btrfs_setxattr()
to check if trans is not null.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/acl.c   |  6 +++++-
 fs/btrfs/props.c | 34 ++++++++++++++++++++++++----------
 fs/btrfs/xattr.c |  6 ++++--
 3 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index d3b04c6abc61..7fe6551bc59b 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -93,7 +93,11 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
 			goto out;
 	}
 
-	ret = btrfs_setxattr_trans(trans, inode, name, value, size, 0);
+	if (trans)
+		ret = btrfs_setxattr(trans, inode, name, value, size, 0);
+	else
+		ret = btrfs_setxattr_trans(NULL, inode, name, value, size, 0);
+
 out:
 	kfree(value);
 
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 61ced0ebb5ba..a73c1bdc7b05 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -70,8 +70,13 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		return -EINVAL;
 
 	if (value_len == 0) {
-		ret = btrfs_setxattr_trans(trans, inode, handler->xattr_name,
-					   NULL, 0, flags);
+		if (trans)
+			ret = btrfs_setxattr(trans, inode, handler->xattr_name,
+					     NULL, 0, flags);
+		else
+			ret = btrfs_setxattr_trans(NULL, inode,
+						   handler->xattr_name, NULL, 0,
+						   flags);
 		if (ret)
 			return ret;
 
@@ -84,14 +89,23 @@ 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(trans, inode, handler->xattr_name,
-				   value, value_len, flags);
+	if (trans)
+		ret = btrfs_setxattr(trans, inode, handler->xattr_name, value,
+				     value_len, flags);
+	else
+		ret = btrfs_setxattr_trans(NULL, inode, handler->xattr_name,
+					   value, value_len, flags);
+
 	if (ret)
 		return ret;
 	ret = handler->apply(inode, value, value_len);
 	if (ret) {
-		btrfs_setxattr_trans(trans, inode, handler->xattr_name,
-				     NULL, 0, flags);
+		if (trans)
+			btrfs_setxattr(trans, inode, handler->xattr_name, NULL,
+				       0, flags);
+		else
+			btrfs_setxattr_trans(NULL, inode, handler->xattr_name,
+					     NULL, 0, flags);
 		return ret;
 	}
 
@@ -358,13 +372,13 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		if (ret)
 			return ret;
 
-		ret = btrfs_setxattr_trans(trans, inode, h->xattr_name, value,
-					   strlen(value), 0);
+		ret = btrfs_setxattr(trans, inode, h->xattr_name, value,
+				     strlen(value), 0);
 		if (!ret) {
 			ret = h->apply(inode, value, strlen(value));
 			if (ret)
-				btrfs_setxattr_trans(trans, inode, h->xattr_name,
-						     NULL, 0, 0);
+				btrfs_setxattr(trans, inode, h->xattr_name,
+					       NULL, 0, 0);
 			else
 				set_bit(BTRFS_INODE_HAS_PROPS,
 					&BTRFS_I(inode)->runtime_flags);
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 4c447b1f32e5..623d508f21a6 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -86,6 +86,8 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
 	size_t name_len = strlen(name);
 	int ret = 0;
 
+	ASSERT(trans);
+
 	if (name_len + size > BTRFS_MAX_XATTR_SIZE(root->fs_info))
 		return -ENOSPC;
 
@@ -437,8 +439,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(trans, inode, name, xattr->value,
-					   xattr->value_len, 0);
+		err = btrfs_setxattr(trans, inode, name, xattr->value,
+				     xattr->value_len, 0);
 		kfree(name);
 		if (err < 0)
 			break;
-- 
2.17.1

[PATCH 6/6] btrfs: cleanup btrfs_setxattr_trans drop trans arg

[Anand Jain]

The previous preparatory
  btrfs: split thread with trans to use btrfs_setxattr
made sure that btrfs_setxattr_trans() is called only when trans arg is
NULL.

Now this patch cleanups btrfs_setxattr_trans() and drops the trans arg.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/acl.c   |  2 +-
 fs/btrfs/props.c | 13 ++++++-------
 fs/btrfs/xattr.c |  9 +++------
 fs/btrfs/xattr.h |  5 ++---
 4 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 7fe6551bc59b..a0af1b952c4d 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -96,7 +96,7 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
 	if (trans)
 		ret = btrfs_setxattr(trans, inode, name, value, size, 0);
 	else
-		ret = btrfs_setxattr_trans(NULL, inode, name, value, size, 0);
+		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 a73c1bdc7b05..44b7bf647ab3 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -74,9 +74,8 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 			ret = btrfs_setxattr(trans, inode, handler->xattr_name,
 					     NULL, 0, flags);
 		else
-			ret = btrfs_setxattr_trans(NULL, inode,
-						   handler->xattr_name, NULL, 0,
-						   flags);
+			ret = btrfs_setxattr_trans(inode, handler->xattr_name,
+						   NULL, 0, flags);
 		if (ret)
 			return ret;
 
@@ -93,8 +92,8 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		ret = btrfs_setxattr(trans, inode, handler->xattr_name, value,
 				     value_len, flags);
 	else
-		ret = btrfs_setxattr_trans(NULL, inode, handler->xattr_name,
-					   value, value_len, flags);
+		ret = btrfs_setxattr_trans(inode, handler->xattr_name, value,
+					   value_len, flags);
 
 	if (ret)
 		return ret;
@@ -104,8 +103,8 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 			btrfs_setxattr(trans, inode, handler->xattr_name, NULL,
 				       0, flags);
 		else
-			btrfs_setxattr_trans(NULL, inode, handler->xattr_name,
-					     NULL, 0, flags);
+			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 623d508f21a6..10da873d11f5 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -221,16 +221,13 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
 /*
  * @value: "" makes the attribute to empty, NULL removes it
  */
-int btrfs_setxattr_trans(struct btrfs_trans_handle *trans,
-			 struct inode *inode, const char *name,
+int btrfs_setxattr_trans(struct inode *inode, const char *name,
 			 const void *value, size_t size, int flags)
 {
 	struct btrfs_root *root = BTRFS_I(inode)->root;
+	struct btrfs_trans_handle *trans;
 	int ret;
 
-	if (trans)
-		return btrfs_setxattr(trans, inode, name, value, size, flags);
-
 	trans = btrfs_start_transaction(root, 2);
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
@@ -368,7 +365,7 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler,
 				   size_t size, int flags)
 {
 	name = xattr_full_name(handler, name);
-	return btrfs_setxattr_trans(NULL, inode, name, buffer, size, flags);
+	return btrfs_setxattr_trans(inode, name, buffer, size, flags);
 }
 
 static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
index a95834cc3c04..1cd3fc0a8f17 100644
--- a/fs/btrfs/xattr.h
+++ b/fs/btrfs/xattr.h
@@ -14,9 +14,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_trans(struct btrfs_trans_handle *trans,
-			    struct inode *inode, const char *name,
-			    const void *value, size_t size, int flags);
+int btrfs_setxattr_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,
-- 
2.17.1

Re: [PATCH 0/6] cleanup property and extended attribute set

[Anand Jain]

just a note: this is based on misc-next.

On 12/4/19 4:02 PM, Anand Jain wrote:
> In an attempt to stream line the property and extended attribute set here
> are the few cleanup patches.
> 
>   1/6 to 3/6 are mostly non functional cleanups (except for the conversion
>    to non static function in 3/6) and can be merged together.
>   4/6 removes the readonly root check in btrfs_setxattr() more details in
>    the change log.
>   5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
>    threads with transaction and without transaction respectively, so this
>    patch uses them.
>   6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
>    now btrfs_setxattr_trans() can drop the trans arg.
> 
> Anand Jain (6):
>    btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
>    btrfs: rename do_setxattr to btrfs_setxattr
>    btrfs: declare btrfs_setxattr as a non static function
>    btrfs: remove redundant readonly root check in btrfs_setxattr_trans
>    btrfs: split thread with trans to use btrfs_setxattr
>    btrfs: cleanup btrfs_setxattr_trans drop trans arg
> 
>   fs/btrfs/acl.c   |  6 +++++-
>   fs/btrfs/props.c | 25 +++++++++++++++++++------
>   fs/btrfs/xattr.c | 25 ++++++++++---------------
>   fs/btrfs/xattr.h |  7 ++++---
>   4 files changed, 38 insertions(+), 25 deletions(-)
> 

Re: [PATCH 0/6] cleanup property and extended attribute set

[David Sterba]

On Fri, Apr 12, 2019 at 04:02:53PM +0800, Anand Jain wrote:
> In an attempt to stream line the property and extended attribute set here
> are the few cleanup patches.
> 
>  1/6 to 3/6 are mostly non functional cleanups (except for the conversion
>   to non static function in 3/6) and can be merged together.
>  4/6 removes the readonly root check in btrfs_setxattr() more details in
>   the change log.
>  5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
>   threads with transaction and without transaction respectively, so this
>   patch uses them.
>  6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
>   now btrfs_setxattr_trans() can drop the trans arg.
> 
> Anand Jain (6):
>   btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
>   btrfs: rename do_setxattr to btrfs_setxattr
>   btrfs: declare btrfs_setxattr as a non static function
>   btrfs: remove redundant readonly root check in btrfs_setxattr_trans
>   btrfs: split thread with trans to use btrfs_setxattr
>   btrfs: cleanup btrfs_setxattr_trans drop trans arg

Looks good to me, thanks. The result is very close to what the previous
patchset did. Patchset will go to for-next soon.

Re: [PATCH 0/6] cleanup property and extended attribute set

[Anand Jain]

On 16/4/19 3:01 am, David Sterba wrote:
> On Fri, Apr 12, 2019 at 04:02:53PM +0800, Anand Jain wrote:
>> In an attempt to stream line the property and extended attribute set here
>> are the few cleanup patches.
>>
>>   1/6 to 3/6 are mostly non functional cleanups (except for the conversion
>>    to non static function in 3/6) and can be merged together.
>>   4/6 removes the readonly root check in btrfs_setxattr() more details in
>>    the change log.
>>   5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
>>    threads with transaction and without transaction respectively, so this
>>    patch uses them.
>>   6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
>>    now btrfs_setxattr_trans() can drop the trans arg.
>>
>> Anand Jain (6):
>>    btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
>>    btrfs: rename do_setxattr to btrfs_setxattr
>>    btrfs: declare btrfs_setxattr as a non static function
>>    btrfs: remove redundant readonly root check in btrfs_setxattr_trans
>>    btrfs: split thread with trans to use btrfs_setxattr
>>    btrfs: cleanup btrfs_setxattr_trans drop trans arg
> 
> Looks good to me, thanks. The result is very close to what the previous
> patchset did. Patchset will go to for-next soon.
> 

Thanks. On top of these, I am writing patches to merge 
start_transactions in btrfs_ioctl_setflags().

-Anand

Re: [PATCH 0/6] cleanup property and extended attribute set

[David Sterba]

On Tue, Apr 16, 2019 at 06:01:58AM +0800, Anand Jain wrote:
> 
> 
> On 16/4/19 3:01 am, David Sterba wrote:
> > On Fri, Apr 12, 2019 at 04:02:53PM +0800, Anand Jain wrote:
> >> In an attempt to stream line the property and extended attribute set here
> >> are the few cleanup patches.
> >>
> >>   1/6 to 3/6 are mostly non functional cleanups (except for the conversion
> >>    to non static function in 3/6) and can be merged together.
> >>   4/6 removes the readonly root check in btrfs_setxattr() more details in
> >>    the change log.
> >>   5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
> >>    threads with transaction and without transaction respectively, so this
> >>    patch uses them.
> >>   6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
> >>    now btrfs_setxattr_trans() can drop the trans arg.
> >>
> >> Anand Jain (6):
> >>    btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
> >>    btrfs: rename do_setxattr to btrfs_setxattr
> >>    btrfs: declare btrfs_setxattr as a non static function
> >>    btrfs: remove redundant readonly root check in btrfs_setxattr_trans
> >>    btrfs: split thread with trans to use btrfs_setxattr
> >>    btrfs: cleanup btrfs_setxattr_trans drop trans arg
> > 
> > Looks good to me, thanks. The result is very close to what the previous
> > patchset did. Patchset will go to for-next soon.
> > 
> 
> Thanks. On top of these, I am writing patches to merge 
> start_transactions in btrfs_ioctl_setflags().

My current idea how to change btrfs_ioctl_setflags is like that (but I
haven't prototyped it so it might not work):

- don't change binode->flags directly, but do all flag updates on a
  temporary variable

- if a property needs to be changed, do validation first, then start
  transaction and pass it to the property handler

- in the finalizing code, start a transaction unless it's been started
  already, apply the iflags and end transaction

This means there are up to 4 starting points of transaction, but the
property validation should never fail between start/end region. There
are other potential failures due to ENOMEM or ENOSPC, but that's the
general set of errors we can't avoid.

Re: [PATCH 0/6] cleanup property and extended attribute set

[Anand Jain]

On 17/4/19 5:31 PM, David Sterba wrote:
> On Tue, Apr 16, 2019 at 06:01:58AM +0800, Anand Jain wrote:
>>
>>
>> On 16/4/19 3:01 am, David Sterba wrote:
>>> On Fri, Apr 12, 2019 at 04:02:53PM +0800, Anand Jain wrote:
>>>> In an attempt to stream line the property and extended attribute set here
>>>> are the few cleanup patches.
>>>>
>>>>    1/6 to 3/6 are mostly non functional cleanups (except for the conversion
>>>>     to non static function in 3/6) and can be merged together.
>>>>    4/6 removes the readonly root check in btrfs_setxattr() more details in
>>>>     the change log.
>>>>    5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
>>>>     threads with transaction and without transaction respectively, so this
>>>>     patch uses them.
>>>>    6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
>>>>     now btrfs_setxattr_trans() can drop the trans arg.
>>>>
>>>> Anand Jain (6):
>>>>     btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
>>>>     btrfs: rename do_setxattr to btrfs_setxattr
>>>>     btrfs: declare btrfs_setxattr as a non static function
>>>>     btrfs: remove redundant readonly root check in btrfs_setxattr_trans
>>>>     btrfs: split thread with trans to use btrfs_setxattr
>>>>     btrfs: cleanup btrfs_setxattr_trans drop trans arg
>>>
>>> Looks good to me, thanks. The result is very close to what the previous
>>> patchset did. Patchset will go to for-next soon.
>>>
>>
>> Thanks. On top of these, I am writing patches to merge
>> start_transactions in btrfs_ioctl_setflags().
> 
> My current idea how to change btrfs_ioctl_setflags is like that (but I
> haven't prototyped it so it might not work):
> 
> - don't change binode->flags directly, but do all flag updates on a
>    temporary variable
> 
> - if a property needs to be changed, do validation first, then start
>    transaction and pass it to the property handler
 >
> - in the finalizing code, start a transaction unless it's been started
>    already, apply the iflags and end transaction

I almost have the same way, but not in the same change sequence.
Which should be fine.

> This means there are up to 4 starting points of transaction, but the
> property validation should never fail between start/end region. There
> are other potential failures due to ENOMEM or ENOSPC, but that's the
> general set of errors we can't avoid.
> 

My new docker build+test setup is giving me a little bit of
headache. I will send the patches for the review soon.

Thanks, Anand

Re: [PATCH 0/6] cleanup property and extended attribute set

[Anand Jain]

On 17/4/19 5:31 PM, David Sterba wrote:
> On Tue, Apr 16, 2019 at 06:01:58AM +0800, Anand Jain wrote:
>>
>>
>> On 16/4/19 3:01 am, David Sterba wrote:
>>> On Fri, Apr 12, 2019 at 04:02:53PM +0800, Anand Jain wrote:
>>>> In an attempt to stream line the property and extended attribute set here
>>>> are the few cleanup patches.
>>>>
>>>>    1/6 to 3/6 are mostly non functional cleanups (except for the conversion
>>>>     to non static function in 3/6) and can be merged together.
>>>>    4/6 removes the readonly root check in btrfs_setxattr() more details in
>>>>     the change log.
>>>>    5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
>>>>     threads with transaction and without transaction respectively, so this
>>>>     patch uses them.
>>>>    6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
>>>>     now btrfs_setxattr_trans() can drop the trans arg.
>>>>
>>>> Anand Jain (6):
>>>>     btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
>>>>     btrfs: rename do_setxattr to btrfs_setxattr
>>>>     btrfs: declare btrfs_setxattr as a non static function
>>>>     btrfs: remove redundant readonly root check in btrfs_setxattr_trans
>>>>     btrfs: split thread with trans to use btrfs_setxattr
>>>>     btrfs: cleanup btrfs_setxattr_trans drop trans arg
>>>
>>> Looks good to me, thanks. The result is very close to what the previous
>>> patchset did. Patchset will go to for-next soon.
>>>
>>
>> Thanks. On top of these, I am writing patches to merge
>> start_transactions in btrfs_ioctl_setflags().
> 
> My current idea how to change btrfs_ioctl_setflags is like that (but I
> haven't prototyped it so it might not work):
> 
> - don't change binode->flags directly, but do all flag updates on a
>    temporary variable
> 
> - if a property needs to be changed, do validation first, then start
>    transaction and pass it to the property handler

  In btrfs_ioctl_setflags() we don't need the validate() at all, as the
  the property strings are hard coded with in the kernel or already
  verified during the mount -o option (fs_info->compress_type).
  So only place where we need the validate is
  btrfs_xattr_handler_set_prop().

Thanks, Anand

> - in the finalizing code, start a transaction unless it's been started
>    already, apply the iflags and end transaction
> 
> This means there are up to 4 starting points of transaction, but the
> property validation should never fail between start/end region. There
> are other potential failures due to ENOMEM or ENOSPC, but that's the
> general set of errors we can't avoid.
>