[PATCH 0/2 RFC] fix duplicate code in btrfs_ioctl_setflags

[PATCH 0/2 RFC] fix duplicate code in btrfs_ioctl_setflags

[Anand Jain]

There are two reasons why I am marking this as RFC as of now.
I don't see the preparatory patches yet, I wonder if this should
be based on for-next-20190415? I couldn't reproduce the issue,
which theoretically should exist as in the patch 2/2.

Some more cleanups related to the inode flags such as [1]
[1]
(don't change binode->flags directly, but do all flag updates on a
 temporary variable)
is wip.

[PATCH 1/2 RFC] btrfs: refactor btrfs_set_props to validate externally

[Anand Jain]

In preparation to merge multiple transactions when setting the
inode attribute flags, move btrfs_set_props()'s validation part
to outside.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 8c9a908d3acc..63e6e2f5b659 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -284,6 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		binode->flags &= ~BTRFS_INODE_COMPRESS;
 		binode->flags |= BTRFS_INODE_NOCOMPRESS;
 
+		/* set no-compression no need to validate prop here */
 		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
 					   0, 0);
 		if (ret && ret != -ENODATA)
@@ -299,6 +300,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		binode->flags |= BTRFS_INODE_COMPRESS;
 		binode->flags &= ~BTRFS_INODE_NOCOMPRESS;
 
+		/* compress_type is already validated during mount options */
 		comp = btrfs_compress_type2str(fs_info->compress_type);
 		if (!comp || comp[0] == 0)
 			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
@@ -309,6 +311,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 			goto out_drop;
 
 	} else {
+		/* reset prop, no need of validate prop here */
 		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
 					   0, 0);
 		if (ret && ret != -ENODATA)
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 44b7bf647ab3..e356dd2a0f73 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -55,6 +55,23 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
+int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
+{
+	const struct prop_handler *handler;
+
+	if (strlen(name) <= XATTR_BTRFS_PREFIX_LEN)
+		return -EINVAL;
+
+	handler = find_prop_handler(name, NULL);
+	if (!handler)
+		return -EINVAL;
+
+	if (value_len == 0)
+		return 0;
+
+	return handler->validate(value, value_len);
+}
+
 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)
@@ -62,9 +79,6 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 	const struct prop_handler *handler;
 	int ret;
 
-	if (strlen(name) <= XATTR_BTRFS_PREFIX_LEN)
-		return -EINVAL;
-
 	handler = find_prop_handler(name, NULL);
 	if (!handler)
 		return -EINVAL;
@@ -85,9 +99,6 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		return ret;
 	}
 
-	ret = handler->validate(value, value_len);
-	if (ret)
-		return ret;
 	if (trans)
 		ret = btrfs_setxattr(trans, inode, handler->xattr_name, value,
 				     value_len, flags);
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index b1a6b233b774..01d2c1899bc7 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -12,6 +12,7 @@ void __init btrfs_props_init(void);
 
 int btrfs_set_prop_trans(struct inode *inode, const char *name,
 			 const char *value, size_t value_len, int flags);
+int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
 
 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 10da873d11f5..807cc24dc885 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -373,7 +373,12 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
 					const char *name, const void *value,
 					size_t size, int flags)
 {
+	int ret;
+
 	name = xattr_full_name(handler, name);
+	ret = btrfs_validate_prop(name, value, size);
+	if (ret)
+		return ret;
 	return btrfs_set_prop_trans(inode, name, value, size, flags);
 }
 
-- 
2.17.1

[PATCH 2/2 RFC] btrfs: start transaction in btrfs_ioctl_setflags()

[Anand Jain]

When inode attribute flags is set through FS_IOC_SETFLAGS ioctl, there is
a bit of duplicate codes, the following things happens twice -
start/end_transaction, inode_inc_iversion(), current_time update to
inode->i_ctime, and btrfs_update_inode(). These are updated both at
btrfs_ioctl_setflags() and btrfs_set_props() as well. (RFC: So inode
version and the generation number should have increased by +2, but I
don't see that in the test case below ran without this patch)

This patch merges these two duplicate codes at btrfs_ioctl_setflags().

$ cat a
mkfs.btrfs -fq /dev/sdb || exit
mount /dev/sdb /btrfs
touch /btrfs/file1
sync
echo before:
btrfs in dump-super /dev/sdb | grep ^generation
btrfs in dump-tree /dev/sdb | grep -A4 "257 XATTR_ITEM"
btrfs in dump-tree /dev/sdb | grep -A4 "257 INODE_ITEM 0) item"
echo
echo "chattr +Ac /btrfs/file1"
chattr +Ac /btrfs/file1
sync
echo after:
btrfs in dump-super /dev/sdb | grep ^generation
btrfs in dump-tree /dev/sdb | grep -A4 "257 XATTR_ITEM"
btrfs in dump-tree /dev/sdb | grep -A4 "257 INODE_ITEM 0) item"

$ umount /btrfs; ./a
before:
generation		6
	item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
		generation 6 transid 6 size 0 nbytes 0
		block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
		sequence 19 flags 0x0(none)

chattr +Ac /btrfs/file1
after:
generation		7
	item 6 key (257 XATTR_ITEM 550297449) itemoff 15815 itemsize 51
		location key (0 UNKNOWN.0 0) type XATTR
		transid 7 data_len 4 name_len 17
		name: btrfs.compression
		data zlib
	item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
		generation 6 transid 7 size 0 nbytes 0
		block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
		sequence 21 flags 0xa00(NOATIME|COMPRESS)

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c | 38 ++++++++++++++++++--------------------
 fs/btrfs/props.c |  6 +++---
 fs/btrfs/props.h |  3 +++
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 63e6e2f5b659..82772523bb87 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -192,6 +192,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	u64 old_flags;
 	unsigned int old_i_flags;
 	umode_t mode;
+	const char *comp = NULL;
 
 	if (!inode_owner_or_capable(inode))
 		return -EPERM;
@@ -283,14 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	if (fsflags & FS_NOCOMP_FL) {
 		binode->flags &= ~BTRFS_INODE_COMPRESS;
 		binode->flags |= BTRFS_INODE_NOCOMPRESS;
-
-		/* set no-compression no need to validate prop here */
-		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
-					   0, 0);
-		if (ret && ret != -ENODATA)
-			goto out_drop;
 	} else if (fsflags & FS_COMPR_FL) {
-		const char *comp;
 
 		if (IS_SWAPFILE(inode)) {
 			ret = -ETXTBSY;
@@ -300,36 +294,40 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		binode->flags |= BTRFS_INODE_COMPRESS;
 		binode->flags &= ~BTRFS_INODE_NOCOMPRESS;
 
-		/* compress_type is already validated during mount options */
 		comp = btrfs_compress_type2str(fs_info->compress_type);
 		if (!comp || comp[0] == 0)
 			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
-
-		ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp,
-					   strlen(comp), 0);
-		if (ret)
-			goto out_drop;
-
 	} else {
-		/* reset prop, no need of validate prop here */
-		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);
 	}
 
-	trans = btrfs_start_transaction(root, 1);
+	trans = btrfs_start_transaction(root, 3);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out_drop;
 	}
 
+	if (comp) {
+		ret = btrfs_set_prop(trans, inode, "btrfs.compression", comp,
+				     strlen(comp), 0);
+		if (ret)
+			goto out_abort;
+		set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);
+	} else {
+		ret = btrfs_set_prop(trans, inode, "btrfs.compression", NULL,
+				     0, 0);
+		if (ret && ret != -ENODATA)
+			goto out_abort;
+	}
+
 	btrfs_sync_inode_flags_to_i_flags(inode);
 	inode_inc_iversion(inode);
 	inode->i_ctime = current_time(inode);
 	ret = btrfs_update_inode(trans, root, inode);
 
+ out_abort:
+	if (ret)
+		btrfs_abort_transaction(trans, ret);
 	btrfs_end_transaction(trans);
  out_drop:
 	if (ret) {
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index e356dd2a0f73..aedf5a7d69c9 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -72,9 +72,9 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
 	return handler->validate(value, value_len);
 }
 
-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;
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 01d2c1899bc7..30b99348977d 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -12,6 +12,9 @@ void __init btrfs_props_init(void);
 
 int btrfs_set_prop_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);
 int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
 
 int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
-- 
2.17.1

Re: [PATCH 2/2 RFC] btrfs: start transaction in btrfs_ioctl_setflags()

[David Sterba]

On Wed, Apr 17, 2019 at 11:37:06PM +0800, Anand Jain wrote:
> When inode attribute flags is set through FS_IOC_SETFLAGS ioctl, there is
> a bit of duplicate codes, the following things happens twice -
> start/end_transaction, inode_inc_iversion(), current_time update to
> inode->i_ctime, and btrfs_update_inode(). These are updated both at
> btrfs_ioctl_setflags() and btrfs_set_props() as well. (RFC: So inode
> version and the generation number should have increased by +2, but I
> don't see that in the test case below ran without this patch)
> 
> This patch merges these two duplicate codes at btrfs_ioctl_setflags().
> 
> $ cat a
> mkfs.btrfs -fq /dev/sdb || exit
> mount /dev/sdb /btrfs
> touch /btrfs/file1
> sync
> echo before:
> btrfs in dump-super /dev/sdb | grep ^generation
> btrfs in dump-tree /dev/sdb | grep -A4 "257 XATTR_ITEM"
> btrfs in dump-tree /dev/sdb | grep -A4 "257 INODE_ITEM 0) item"
> echo
> echo "chattr +Ac /btrfs/file1"
> chattr +Ac /btrfs/file1
> sync
> echo after:
> btrfs in dump-super /dev/sdb | grep ^generation
> btrfs in dump-tree /dev/sdb | grep -A4 "257 XATTR_ITEM"
> btrfs in dump-tree /dev/sdb | grep -A4 "257 INODE_ITEM 0) item"
> 
> $ umount /btrfs; ./a
> before:
> generation		6
> 	item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
> 		generation 6 transid 6 size 0 nbytes 0
> 		block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
> 		sequence 19 flags 0x0(none)
> 
> chattr +Ac /btrfs/file1
> after:
> generation		7
> 	item 6 key (257 XATTR_ITEM 550297449) itemoff 15815 itemsize 51
> 		location key (0 UNKNOWN.0 0) type XATTR
> 		transid 7 data_len 4 name_len 17
> 		name: btrfs.compression
> 		data zlib
> 	item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
> 		generation 6 transid 7 size 0 nbytes 0
> 		block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
> 		sequence 21 flags 0xa00(NOATIME|COMPRESS)
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c | 38 ++++++++++++++++++--------------------
>  fs/btrfs/props.c |  6 +++---
>  fs/btrfs/props.h |  3 +++
>  3 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 63e6e2f5b659..82772523bb87 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -192,6 +192,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	u64 old_flags;
>  	unsigned int old_i_flags;
>  	umode_t mode;
> +	const char *comp = NULL;
>  
>  	if (!inode_owner_or_capable(inode))
>  		return -EPERM;
> @@ -283,14 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	if (fsflags & FS_NOCOMP_FL) {
>  		binode->flags &= ~BTRFS_INODE_COMPRESS;
>  		binode->flags |= BTRFS_INODE_NOCOMPRESS;
> -
> -		/* set no-compression no need to validate prop here */
> -		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
> -					   0, 0);
> -		if (ret && ret != -ENODATA)
> -			goto out_drop;
>  	} else if (fsflags & FS_COMPR_FL) {
> -		const char *comp;
>  
>  		if (IS_SWAPFILE(inode)) {
>  			ret = -ETXTBSY;
> @@ -300,36 +294,40 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  		binode->flags |= BTRFS_INODE_COMPRESS;
>  		binode->flags &= ~BTRFS_INODE_NOCOMPRESS;
>  
> -		/* compress_type is already validated during mount options */
>  		comp = btrfs_compress_type2str(fs_info->compress_type);
>  		if (!comp || comp[0] == 0)
>  			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
> -
> -		ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp,
> -					   strlen(comp), 0);
> -		if (ret)
> -			goto out_drop;
> -
>  	} else {
> -		/* reset prop, no need of validate prop here */
> -		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);
>  	}
>  
> -	trans = btrfs_start_transaction(root, 1);
> +	trans = btrfs_start_transaction(root, 3);

This should reflect if the properties are actually changed or not, so
there's not unnecessary reservation made.

>  	if (IS_ERR(trans)) {
>  		ret = PTR_ERR(trans);
>  		goto out_drop;
>  	}
>  
> +	if (comp) {
> +		ret = btrfs_set_prop(trans, inode, "btrfs.compression", comp,
> +				     strlen(comp), 0);
> +		if (ret)
> +			goto out_abort;

Transaction abort should be done at the place where they happen and not
aggregated.

I see now that the validation is not necessary here.

> +		set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);
> +	} else {
> +		ret = btrfs_set_prop(trans, inode, "btrfs.compression", NULL,
> +				     0, 0);
> +		if (ret && ret != -ENODATA)
> +			goto out_abort;

Same.

> +	}
> +
>  	btrfs_sync_inode_flags_to_i_flags(inode);
>  	inode_inc_iversion(inode);
>  	inode->i_ctime = current_time(inode);
>  	ret = btrfs_update_inode(trans, root, inode);
>  
> + out_abort:
> +	if (ret)
> +		btrfs_abort_transaction(trans, ret);
>  	btrfs_end_transaction(trans);
>   out_drop:
>  	if (ret) {
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index e356dd2a0f73..aedf5a7d69c9 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -72,9 +72,9 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
>  	return handler->validate(value, value_len);
>  }
>  
> -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)

Exporting a fuction should be in another patch.

>  {
>  	const struct prop_handler *handler;
>  	int ret;
> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
> index 01d2c1899bc7..30b99348977d 100644
> --- a/fs/btrfs/props.h
> +++ b/fs/btrfs/props.h
> @@ -12,6 +12,9 @@ void __init btrfs_props_init(void);
>  
>  int btrfs_set_prop_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);
>  int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
>  
>  int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
> -- 
> 2.17.1

Re: [PATCH 2/2 RFC] btrfs: start transaction in btrfs_ioctl_setflags()

[Anand Jain]

Thanks for the comments.. more below.

On 18/4/19 9:23 PM, David Sterba wrote:
> On Wed, Apr 17, 2019 at 11:37:06PM +0800, Anand Jain wrote:
>> When inode attribute flags is set through FS_IOC_SETFLAGS ioctl, there is
>> a bit of duplicate codes, the following things happens twice -
>> start/end_transaction, inode_inc_iversion(), current_time update to
>> inode->i_ctime, and btrfs_update_inode(). These are updated both at
>> btrfs_ioctl_setflags() and btrfs_set_props() as well. (RFC: So inode
>> version and the generation number should have increased by +2, but I
>> don't see that in the test case below ran without this patch)

Looks like we don't commit transaction on the end transaction. So its
just about optimizing the duplicate code.

>> This patch merges these two duplicate codes at btrfs_ioctl_setflags().
>>
>> $ cat a
>> mkfs.btrfs -fq /dev/sdb || exit
>> mount /dev/sdb /btrfs
>> touch /btrfs/file1
>> sync
>> echo before:
>> btrfs in dump-super /dev/sdb | grep ^generation
>> btrfs in dump-tree /dev/sdb | grep -A4 "257 XATTR_ITEM"
>> btrfs in dump-tree /dev/sdb | grep -A4 "257 INODE_ITEM 0) item"
>> echo
>> echo "chattr +Ac /btrfs/file1"
>> chattr +Ac /btrfs/file1
>> sync
>> echo after:
>> btrfs in dump-super /dev/sdb | grep ^generation
>> btrfs in dump-tree /dev/sdb | grep -A4 "257 XATTR_ITEM"
>> btrfs in dump-tree /dev/sdb | grep -A4 "257 INODE_ITEM 0) item"
>>
>> $ umount /btrfs; ./a
>> before:
>> generation		6
>> 	item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
>> 		generation 6 transid 6 size 0 nbytes 0
>> 		block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
>> 		sequence 19 flags 0x0(none)
>>
>> chattr +Ac /btrfs/file1
>> after:
>> generation		7
>> 	item 6 key (257 XATTR_ITEM 550297449) itemoff 15815 itemsize 51
>> 		location key (0 UNKNOWN.0 0) type XATTR
>> 		transid 7 data_len 4 name_len 17
>> 		name: btrfs.compression
>> 		data zlib
>> 	item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
>> 		generation 6 transid 7 size 0 nbytes 0
>> 		block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
>> 		sequence 21 flags 0xa00(NOATIME|COMPRESS)
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/ioctl.c | 38 ++++++++++++++++++--------------------
>>   fs/btrfs/props.c |  6 +++---
>>   fs/btrfs/props.h |  3 +++
>>   3 files changed, 24 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 63e6e2f5b659..82772523bb87 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -192,6 +192,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>>   	u64 old_flags;
>>   	unsigned int old_i_flags;
>>   	umode_t mode;
>> +	const char *comp = NULL;
>>   
>>   	if (!inode_owner_or_capable(inode))
>>   		return -EPERM;
>> @@ -283,14 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>>   	if (fsflags & FS_NOCOMP_FL) {
>>   		binode->flags &= ~BTRFS_INODE_COMPRESS;
>>   		binode->flags |= BTRFS_INODE_NOCOMPRESS;
>> -
>> -		/* set no-compression no need to validate prop here */
>> -		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
>> -					   0, 0);
>> -		if (ret && ret != -ENODATA)
>> -			goto out_drop;
>>   	} else if (fsflags & FS_COMPR_FL) {
>> -		const char *comp;
>>   
>>   		if (IS_SWAPFILE(inode)) {
>>   			ret = -ETXTBSY;
>> @@ -300,36 +294,40 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>>   		binode->flags |= BTRFS_INODE_COMPRESS;
>>   		binode->flags &= ~BTRFS_INODE_NOCOMPRESS;
>>   
>> -		/* compress_type is already validated during mount options */
>>   		comp = btrfs_compress_type2str(fs_info->compress_type);
>>   		if (!comp || comp[0] == 0)
>>   			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
>> -
>> -		ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp,
>> -					   strlen(comp), 0);
>> -		if (ret)
>> -			goto out_drop;
>> -
>>   	} else {
>> -		/* reset prop, no need of validate prop here */
>> -		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);
>>   	}
>>   
>> -	trans = btrfs_start_transaction(root, 1);
>> +	trans = btrfs_start_transaction(root, 3);
> 
> This should reflect if the properties are actually changed or not, so
> there's not unnecessary reservation made.

  Here we assume reset a flag, if its unset.
  So to preserve a flag which is already set, the cli
  internally is doing some kind of read modify write.
  So we over write the compression property with the
  same value all the time. Can this optimization go into
  a new patch, as its been like that in the original code
  as well.


>>   	if (IS_ERR(trans)) {
>>   		ret = PTR_ERR(trans);
>>   		goto out_drop;
>>   	}
>>   
>> +	if (comp) {
>> +		ret = btrfs_set_prop(trans, inode, "btrfs.compression", comp,
>> +				     strlen(comp), 0);
>> +		if (ret)
>> +			goto out_abort;
> 
> Transaction abort should be done at the place where they happen and not
> aggregated.

  oh yes. Will do.

> I see now that the validation is not necessary here.
> 
>> +		set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);
>> +	} else {
>> +		ret = btrfs_set_prop(trans, inode, "btrfs.compression", NULL,
>> +				     0, 0);
>> +		if (ret && ret != -ENODATA)
>> +			goto out_abort;
> 
> Same.

yep thanks.

>> +	}
>> +
>>   	btrfs_sync_inode_flags_to_i_flags(inode);
>>   	inode_inc_iversion(inode);
>>   	inode->i_ctime = current_time(inode);
>>   	ret = btrfs_update_inode(trans, root, inode);
>>   
>> + out_abort:
>> +	if (ret)
>> +		btrfs_abort_transaction(trans, ret);
>>   	btrfs_end_transaction(trans);
>>    out_drop:
>>   	if (ret) {
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index e356dd2a0f73..aedf5a7d69c9 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -72,9 +72,9 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
>>   	return handler->validate(value, value_len);
>>   }
>>   
>> -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)
> 
> Exporting a fuction should be in another patch.

ok.

Thanks, Anand

>>   {
>>   	const struct prop_handler *handler;
>>   	int ret;
>> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
>> index 01d2c1899bc7..30b99348977d 100644
>> --- a/fs/btrfs/props.h
>> +++ b/fs/btrfs/props.h
>> @@ -12,6 +12,9 @@ void __init btrfs_props_init(void);
>>   
>>   int btrfs_set_prop_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);
>>   int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
>>   
>>   int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
>> -- 
>> 2.17.1