[PATCH 0/4 RESEND] btrfs: fix property bugs

[PATCH 0/4 RESEND] btrfs: fix property bugs

[Anand Jain]

1/4 Fixes a bug that we were applying an incomplete string zstd.
2/4 Fixes a bug that the compression property gets reset to NULL after a
    failed attempt to change the compression parameter on the object.
3/4 preparatory patch so that btrfs_set_prop_trans() can be merged with
    btrfs_set_prop() in 4/4.
4/4 Fixes a bug that we were incrementing the generation even when the
    property validation check failed.

3/4 and 4/4 are only added here.

Anand Jain (4):
  btrfs: fix zstd compression parameter
  btrfs: fix vanished compression property after failed set
  btrfs: open code btrfs_set_prop in inherit_prop
  btrfs: fix property validate fail should not increment generation

 fs/btrfs/ioctl.c | 10 +++---
 fs/btrfs/props.c | 94 ++++++++++++++++++++++++++++++--------------------------
 fs/btrfs/props.h |  4 +--
 fs/btrfs/xattr.c |  2 +-
 4 files changed, 58 insertions(+), 52 deletions(-)

-- 
1.8.3.1

[PATCH 1/4 RESEND] btrfs: fix zstd compression parameter

[Anand Jain]

We let to pass zstd compression parameter even if its not fully written.
For example:
  btrfs prop set /btrfs compression zst
  btrfs prop get /btrfs compression
     compression=zst

zlib and lzo are fine.

Fix it by using the expected number of char in strncmp().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/props.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f1a8be67f639..3cc007e3c7f8 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -304,7 +304,7 @@ static int prop_compression_apply(struct inode *inode, const char *value,
 		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
 	} else if (!strncmp("zlib", value, 4)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	} else if (!strncmp("zstd", value, len)) {
+	} else if (!strncmp("zstd", value, 4)) {
 		type = BTRFS_COMPRESS_ZSTD;
 		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
 	} else {
-- 
1.8.3.1

[PATCH] fstests: btrfs/048: amend property validation cases

[Anand Jain]

Add more property validation cases which are fixed by the patches [1]
 [1]
  btrfs: fix property validate fail should not increment generation
  btrfs: open code btrfs_set_prop in inherit_prop
  btrfs: fix vanished compression property after failed set
  btrfs: fix zstd compression parameter

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
(As this fstest patch depends on the kernel patches,
 this patch isn't sent to fstests mailing list yet).

 tests/btrfs/048     | 25 +++++++++++++++++++++++++
 tests/btrfs/048.out | 16 ++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/tests/btrfs/048 b/tests/btrfs/048
index 588219579cc6..657634f7cc3e 100755
--- a/tests/btrfs/048
+++ b/tests/btrfs/048
@@ -6,6 +6,11 @@
 #
 # Btrfs properties test. The btrfs properties feature was introduced in the
 # linux kernel 3.14.
+# Fails without the kernel patches:
+#  btrfs: fix property validate fail should not increment generation
+#  btrfs: open code btrfs_set_prop in inherit_prop
+#  btrfs: fix vanished compression property after failed set
+#  btrfs: fix zstd compression parameter
 #
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
@@ -34,6 +39,7 @@ _supported_os Linux
 _require_test
 _require_scratch
 _require_btrfs_command "property"
+_require_btrfs_command inspect-internal dump-super
 
 send_files_dir=$TEST_DIR/btrfs-test-$seq
 
@@ -203,5 +209,24 @@ $BTRFS_UTIL_PROG property get $SCRATCH_MNT/sv1 compression
 touch $SCRATCH_MNT/sv1/file2
 $BTRFS_UTIL_PROG property get $SCRATCH_MNT/sv1/file2 compression
 
+echo -e "\nTesting argument validation, should fail"
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'lz' | _filter_scratch
+echo "***"
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'zli' | _filter_scratch
+echo "***"
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'zst' | _filter_scratch
+
+echo -e "\nTesting if property is persistent across failed validation"
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'lzo'
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'zli' | _filter_scratch
+$BTRFS_UTIL_PROG property get $SCRATCH_MNT compression
+
+echo -e "\nTesting generation is unchanged after failed validation"
+$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
+$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep '^generation'
+$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'lz' | _filter_scratch
+$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
+$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep '^generation'
+
 status=0
 exit
diff --git a/tests/btrfs/048.out b/tests/btrfs/048.out
index 3e4e3d28950a..00f39bc01227 100644
--- a/tests/btrfs/048.out
+++ b/tests/btrfs/048.out
@@ -76,3 +76,19 @@ compression=zlib
 Testing subvolume property inheritance
 compression=lzo
 compression=lzo
+
+Testing argument validation, should fail
+ERROR: failed to set compression for /mnt/scratch: Invalid argument
+***
+ERROR: failed to set compression for /mnt/scratch: Invalid argument
+***
+ERROR: failed to set compression for /mnt/scratch: Invalid argument
+
+Testing if property is persistent across failed validation
+ERROR: failed to set compression for /mnt/scratch: Invalid argument
+compression=lzo
+
+Testing generation is unchanged after failed validation
+generation		7
+ERROR: failed to set compression for /mnt/scratch: Invalid argument
+generation		7
-- 
2.17.1

[PATCH 2/4 RESEND] btrfs: fix vanished compression property after failed set

[Anand Jain]

The compression property resets to NULL, instead of the old value if we
fail to set the new compression parameter.

btrfs prop get /btrfs compression
  compression=lzo
btrfs prop set /btrfs compression zli
  ERROR: failed to set compression for /btrfs: Invalid argument
btrfs prop get /btrfs compression

This is because the compression property ->validate() is successful for
'zli' as the strncmp() used the len passed from the userland.

Fix it by using the expected string length in strncmp().

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

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 3cc007e3c7f8..72a06c4d3c70 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -275,11 +275,11 @@ static int prop_compression_validate(const char *value, size_t len)
 	if (!value)
 		return 0;
 
-	if (!strncmp("lzo", value, len))
+	if (!strncmp("lzo", value, 3))
 		return 0;
-	else if (!strncmp("zlib", value, len))
+	else if (!strncmp("zlib", value, 4))
 		return 0;
-	else if (!strncmp("zstd", value, len))
+	else if (!strncmp("zstd", value, 4))
 		return 0;
 
 	return -EINVAL;
-- 
1.8.3.1

[PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

[Anand Jain]

When an inode inherits property from its parent, we call btrfs_set_prop().
btrfs_set_prop() does an elaborate checks, which is not required in the
context of inheriting a property. Instead just open-code only the required
items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
now the only user of btrfs_set_prop() is gone, (except for the wraper
function btrfs_set_prop_trans()).

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

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 72a06c4d3c70..fb84e76f3b1d 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		if (!value)
 			continue;
 
+		/* may be removed */
+		ret = h->validate(value, strlen(value));
+		if (ret)
+			continue;
+
 		num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
 		ret = btrfs_block_rsv_add(root, trans->block_rsv,
 					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
 		if (ret)
-			goto out;
-		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,
+			return ret;
+
+		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, inode, h->xattr_name,
+					       NULL, 0, 0);
+			else
+				set_bit(BTRFS_INODE_HAS_PROPS,
+					&BTRFS_I(inode)->runtime_flags);
+		}
+
 		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);
 		if (ret)
-			goto out;
+			return ret;
 	}
-	ret = 0;
-out:
-	return ret;
+
+	return 0;
 }
 
 int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
-- 
1.8.3.1

[PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment generation

[Anand Jain]

When the property fails to pass the prop_handlers::validate() check, the
thread should exit with no changes in the kernel, but as we are starting
the transaction too early, we have just updated the generation even if
there is no change.

For example:
btrfs prop get /btrfs compression
 compression=lzo
sync
btrfs in dump-super /dev/sdb | grep "^generation"
 generation		32

Try to set an incomplete compression type

btrfs prop set /btrfs compression zli
  ERROR: failed to set compression for /btrfs: Invalid argument
sync

Set failed but generation is incremented
btrfs in dump-super /dev/sdb | grep "^generation"
 generation		33  <--

Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which
provides flexibility to start the transaction after the
prop_handlers::validate() has been checked.

As of now if the prop_handlers::apply() fails then we still increment
the generation, but if there is strong prop_handlers::validate() then
the prop_handlers::apply() can never fail.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 28ee9fe6edb4..0eeaf9a68082 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -284,8 +284,7 @@ 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_trans(inode, "btrfs.compression", NULL,
-					   0, 0);
+		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
 	} else if (fsflags & FS_COMPR_FL) {
@@ -303,14 +302,13 @@ 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_trans(inode, "btrfs.compression", comp,
-					   strlen(comp), 0);
+		ret = btrfs_set_prop(inode, "btrfs.compression", comp,
+				     strlen(comp), 0);
 		if (ret)
 			goto out_drop;
 
 	} else {
-		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
-					   0, 0);
+		ret = btrfs_set_prop(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 fb84e76f3b1d..99fa2459ba61 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -56,11 +56,12 @@ 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 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;
+	struct btrfs_trans_handle *trans;
 	int ret;
 
 	if (btrfs_root_readonly(BTRFS_I(inode)->root))
@@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		return -EINVAL;
 
 	if (value_len == 0) {
+		/* Its called to reset the property */
+		trans = btrfs_start_transaction(root, 2);
+		if (IS_ERR(trans))
+			return PTR_ERR(trans);
+
 		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
 				       NULL, 0, flags);
-		if (ret)
-			return ret;
-
-		ret = handler->apply(inode, NULL, 0);
-		ASSERT(ret == 0);
-
-		return ret;
+		if (!ret) {
+			ret = handler->apply(inode, NULL, 0);
+			ASSERT(ret == 0);
+		}
+		goto out;
 	}
 
 	ret = handler->validate(value, value_len);
 	if (ret)
 		return ret;
+
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
 	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
 			       value, value_len, flags);
 	if (ret)
-		return ret;
+		goto out;
+
 	ret = handler->apply(inode, value, value_len);
 	if (ret) {
+		/* Apply failed. Reset the property. */
 		btrfs_setxattr(trans, inode, handler->xattr_name,
 				 NULL, 0, flags);
-		return ret;
-	}
-
-	set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
-
-	return 0;
-}
-
-int btrfs_set_prop_trans(struct inode *inode, const char *name,
-			 const char *value, size_t value_len, int 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) {
+	} else {
+		set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
 		inode_inc_iversion(inode);
 		inode->i_ctime = current_time(inode);
 		set_bit(BTRFS_INODE_COPY_EVERYTHING,
@@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name,
 		ret = btrfs_update_inode(trans, root, inode);
 		BUG_ON(ret);
 	}
+
+out:
 	btrfs_end_transaction(trans);
 	return ret;
 }
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index b1a6b233b774..72bca072f9ba 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -10,8 +10,8 @@
 
 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 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 58579a4d2f22..abc150a56f8b 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -371,7 +371,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_trans(inode, name, value, size, flags);
+	return btrfs_set_prop(inode, name, value, size, flags);
 }
 
 static const struct xattr_handler btrfs_security_xattr_handler = {
-- 
1.8.3.1

Re: [PATCH] fstests: btrfs/048: amend property validation cases

[Nikolay Borisov]

On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> Add more property validation cases which are fixed by the patches [1]
>  [1]
>   btrfs: fix property validate fail should not increment generation
>   btrfs: open code btrfs_set_prop in inherit_prop
>   btrfs: fix vanished compression property after failed set
>   btrfs: fix zstd compression parameter
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> (As this fstest patch depends on the kernel patches,
>  this patch isn't sent to fstests mailing list yet).
> 

Doesn't matter, it's fine to have a test merged before respective kernel
patches have landed. In fact there is one btrfs test (can't remember
which one) authored by you which is failing, yet the respective kernel
patches have never been sent to the mailing list.

Re: [PATCH 2/4 RESEND] btrfs: fix vanished compression property after failed set

[Nikolay Borisov]

On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> The compression property resets to NULL, instead of the old value if we
> fail to set the new compression parameter.
> 
> btrfs prop get /btrfs compression
>   compression=lzo
> btrfs prop set /btrfs compression zli
>   ERROR: failed to set compression for /btrfs: Invalid argument
> btrfs prop get /btrfs compression
> 
> This is because the compression property ->validate() is successful for
> 'zli' as the strncmp() used the len passed from the userland.
> 
> Fix it by using the expected string length in strncmp().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

This mimics the code in prop_compression_apply and so if ->validate()
fails we just return without changing anything. So this LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/props.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 3cc007e3c7f8..72a06c4d3c70 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -275,11 +275,11 @@ static int prop_compression_validate(const char *value, size_t len)
>  	if (!value)
>  		return 0;
>  
> -	if (!strncmp("lzo", value, len))
> +	if (!strncmp("lzo", value, 3))
>  		return 0;
> -	else if (!strncmp("zlib", value, len))
> +	else if (!strncmp("zlib", value, 4))
>  		return 0;
> -	else if (!strncmp("zstd", value, len))
> +	else if (!strncmp("zstd", value, 4))
>  		return 0;
>  
>  	return -EINVAL;
> 

Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

[Nikolay Borisov]

On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> When an inode inherits property from its parent, we call btrfs_set_prop().
> btrfs_set_prop() does an elaborate checks, which is not required in the
> context of inheriting a property. Instead just open-code only the required
> items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
> now the only user of btrfs_set_prop() is gone, (except for the wraper
> function btrfs_set_prop_trans()).
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/props.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 72a06c4d3c70..fb84e76f3b1d 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>  		if (!value)
>  			continue;
>  
> +		/* may be removed */

This comment brings no value. Either explain *why* it can be removed or
remove it altogether.

> +		ret = h->validate(value, strlen(value));
> +		if (ret)
> +			continue;
> +
>  		num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
>  		ret = btrfs_block_rsv_add(root, trans->block_rsv,
>  					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
>  		if (ret)
> -			goto out;
> -		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,
> +			return ret;
> +
> +		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, inode, h->xattr_name,
> +					       NULL, 0, 0);
> +			else
> +				set_bit(BTRFS_INODE_HAS_PROPS,
> +					&BTRFS_I(inode)->runtime_flags);
> +		}
> +
>  		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);

(not related to this patch) but should the reservation be released after
setxattr is called, even if it succeeds, since we need to hold it until
the reservation is committed. E.g. if we have to inherit 2 props then in
the first iteration of the loop we reserve space, we call btrfs_Setxattr
with enough reserved space, then in the 2nd reservation we need another
reservation on top of the previous one, no ?

Otherwise the change per se looks good:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


>  		if (ret)
> -			goto out;
> +			return ret;
>  	}
> -	ret = 0;
> -out:
> -	return ret;
> +
> +	return 0;
>  }
>  
>  int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
> 

Re: [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment generation

[Nikolay Borisov]

On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> When the property fails to pass the prop_handlers::validate() check, the
> thread should exit with no changes in the kernel, but as we are starting
> the transaction too early, we have just updated the generation even if
> there is no change.
> 
> For example:
> btrfs prop get /btrfs compression
>  compression=lzo
> sync
> btrfs in dump-super /dev/sdb | grep "^generation"
>  generation		32
> 
> Try to set an incomplete compression type
> 
> btrfs prop set /btrfs compression zli
>   ERROR: failed to set compression for /btrfs: Invalid argument
> sync
> 
> Set failed but generation is incremented
> btrfs in dump-super /dev/sdb | grep "^generation"
>  generation		33  <--
> 
> Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which
> provides flexibility to start the transaction after the
> prop_handlers::validate() has been checked.
> 
> As of now if the prop_handlers::apply() fails then we still increment
> the generation, but if there is strong prop_handlers::validate() then
> the prop_handlers::apply() can never fail.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

SO what do we gain by this patch? Whether the transaction generation
number (actually in your changelog it will be an improvement to mention
transaction generation explicitly, because there is also inode
generation...) is incremented or not is an implementation detail. It can
be argued both ways whether it should or shouldn't be incremented? Does
this have to do with how btrfs' log works? I.e the transaction
generation is used to

Re: [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment generation

[David Sterba]

On Tue, Apr 02, 2019 at 04:45:23PM +0300, Nikolay Borisov wrote:
> 
> 
> On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> > When the property fails to pass the prop_handlers::validate() check, the
> > thread should exit with no changes in the kernel, but as we are starting
> > the transaction too early, we have just updated the generation even if
> > there is no change.
> > 
> > For example:
> > btrfs prop get /btrfs compression
> >  compression=lzo
> > sync
> > btrfs in dump-super /dev/sdb | grep "^generation"
> >  generation		32
> > 
> > Try to set an incomplete compression type
> > 
> > btrfs prop set /btrfs compression zli
> >   ERROR: failed to set compression for /btrfs: Invalid argument
> > sync
> > 
> > Set failed but generation is incremented
> > btrfs in dump-super /dev/sdb | grep "^generation"
> >  generation		33  <--
> > 
> > Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which
> > provides flexibility to start the transaction after the
> > prop_handlers::validate() has been checked.
> > 
> > As of now if the prop_handlers::apply() fails then we still increment
> > the generation, but if there is strong prop_handlers::validate() then
> > the prop_handlers::apply() can never fail.
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> SO what do we gain by this patch? Whether the transaction generation
> number (actually in your changelog it will be an improvement to mention
> transaction generation explicitly, because there is also inode
> generation...) is incremented or not is an implementation detail.

No, the problem is that the order of the preliminary checks and the
transaction start is reversed. So first the transaction, then do sanity
checks, that are otherwise don't depend on the transaction. Many other
places have been updatd to do the lighweight checks first and keep the
transaction region minimal.

Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

[David Sterba]

On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote:
> On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> > When an inode inherits property from its parent, we call btrfs_set_prop().
> > btrfs_set_prop() does an elaborate checks, which is not required in the
> > context of inheriting a property. Instead just open-code only the required
> > items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
> > now the only user of btrfs_set_prop() is gone, (except for the wraper
> > function btrfs_set_prop_trans()).
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > ---
> >  fs/btrfs/props.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> > index 72a06c4d3c70..fb84e76f3b1d 100644
> > --- a/fs/btrfs/props.c
> > +++ b/fs/btrfs/props.c
> > @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
> >  		if (!value)
> >  			continue;
> >  
> > +		/* may be removed */
> 
> This comment brings no value. Either explain *why* it can be removed or
> remove it altogether.
> 
> > +		ret = h->validate(value, strlen(value));
> > +		if (ret)
> > +			continue;
> > +
> >  		num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
> >  		ret = btrfs_block_rsv_add(root, trans->block_rsv,
> >  					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
> >  		if (ret)
> > -			goto out;
> > -		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,
> > +			return ret;
> > +
> > +		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, inode, h->xattr_name,
> > +					       NULL, 0, 0);
> > +			else
> > +				set_bit(BTRFS_INODE_HAS_PROPS,
> > +					&BTRFS_I(inode)->runtime_flags);
> > +		}
> > +
> >  		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);
> 
> (not related to this patch) but should the reservation be released after
> setxattr is called, even if it succeeds, since we need to hold it until
> the reservation is committed. E.g. if we have to inherit 2 props then in
> the first iteration of the loop we reserve space, we call btrfs_Setxattr
> with enough reserved space, then in the 2nd reservation we need another
> reservation on top of the previous one, no ?

I think that's the problem Josef's patches were supposed to fix.

Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

[David Sterba]

On Tue, Apr 02, 2019 at 06:07:41PM +0800, Anand Jain wrote:
> When an inode inherits property from its parent, we call btrfs_set_prop().
> btrfs_set_prop() does an elaborate checks, which is not required in the
> context of inheriting a property. Instead just open-code only the required
> items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
> now the only user of btrfs_set_prop() is gone, (except for the wraper
> function btrfs_set_prop_trans()).
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

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

Re: [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment generation

[David Sterba]

On Tue, Apr 02, 2019 at 06:07:42PM +0800, Anand Jain wrote:
> When the property fails to pass the prop_handlers::validate() check, the
> thread should exit with no changes in the kernel, but as we are starting
> the transaction too early, we have just updated the generation even if
> there is no change.

Agreed, this should be fixed.

> 
> For example:
> btrfs prop get /btrfs compression
>  compression=lzo
> sync
> btrfs in dump-super /dev/sdb | grep "^generation"
>  generation		32
> 
> Try to set an incomplete compression type
> 
> btrfs prop set /btrfs compression zli
>   ERROR: failed to set compression for /btrfs: Invalid argument
> sync
> 
> Set failed but generation is incremented
> btrfs in dump-super /dev/sdb | grep "^generation"
>  generation		33  <--
> 
> Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which
> provides flexibility to start the transaction after the
> prop_handlers::validate() has been checked.

Please split the changes, the collapsed function make it difficult to
review. It does not matter if you first clean up the functions and then
switch the transaction, or the other way around.

> As of now if the prop_handlers::apply() fails then we still increment
> the generation, but if there is strong prop_handlers::validate() then
> the prop_handlers::apply() can never fail.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c | 10 ++++------
>  fs/btrfs/props.c | 59 +++++++++++++++++++++++++-------------------------------
>  fs/btrfs/props.h |  4 ++--
>  fs/btrfs/xattr.c |  2 +-
>  4 files changed, 33 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 28ee9fe6edb4..0eeaf9a68082 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -284,8 +284,7 @@ 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_trans(inode, "btrfs.compression", NULL,
> -					   0, 0);
> +		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
>  		if (ret && ret != -ENODATA)
>  			goto out_drop;
>  	} else if (fsflags & FS_COMPR_FL) {
> @@ -303,14 +302,13 @@ 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_trans(inode, "btrfs.compression", comp,
> -					   strlen(comp), 0);
> +		ret = btrfs_set_prop(inode, "btrfs.compression", comp,
> +				     strlen(comp), 0);
>  		if (ret)
>  			goto out_drop;
>  
>  	} else {
> -		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
> -					   0, 0);
> +		ret = btrfs_set_prop(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 fb84e76f3b1d..99fa2459ba61 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -56,11 +56,12 @@ 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 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;
> +	struct btrfs_trans_handle *trans;
>  	int ret;
>  
>  	if (btrfs_root_readonly(BTRFS_I(inode)->root))
> @@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>  		return -EINVAL;
>  
>  	if (value_len == 0) {
> +		/* Its called to reset the property */
> +		trans = btrfs_start_transaction(root, 2);
> +		if (IS_ERR(trans))
> +			return PTR_ERR(trans);
> +
>  		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
>  				       NULL, 0, flags);
> -		if (ret)
> -			return ret;
> -
> -		ret = handler->apply(inode, NULL, 0);
> -		ASSERT(ret == 0);
> -
> -		return ret;
> +		if (!ret) {
> +			ret = handler->apply(inode, NULL, 0);
> +			ASSERT(ret == 0);
> +		}
> +		goto out;

This block would be better in a separate helper, now that it starts the
transaction, eg.

	if (value_len == 0)
		return do_reset_property(inode, handler, flags);

>  	}
>  
>  	ret = handler->validate(value, value_len);
>  	if (ret)
>  		return ret;
> +
> +	trans = btrfs_start_transaction(root, 2);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
>  	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
>  			       value, value_len, flags);
>  	if (ret)
> -		return ret;
> +		goto out;
> +
>  	ret = handler->apply(inode, value, value_len);
>  	if (ret) {
> +		/* Apply failed. Reset the property. */
>  		btrfs_setxattr(trans, inode, handler->xattr_name,
>  				 NULL, 0, flags);

So that's not new code, but I wonder whether this is correct at all.
Reset means to the original value or to empty value? The problem here is
that it's under transaction, so this should be followed by an abort.

It maybe is possible to safely revert the value without aborting, as the
inode is locked.

> -		return ret;
> -	}
> -
> -	set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
> -
> -	return 0;
> -}
> -
> -int btrfs_set_prop_trans(struct inode *inode, const char *name,
> -			 const char *value, size_t value_len, int 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) {
> +	} else {
> +		set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
>  		inode_inc_iversion(inode);
>  		inode->i_ctime = current_time(inode);
>  		set_bit(BTRFS_INODE_COPY_EVERYTHING,
> @@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name,
>  		ret = btrfs_update_inode(trans, root, inode);
>  		BUG_ON(ret);
>  	}
> +
> +out:
>  	btrfs_end_transaction(trans);
>  	return ret;
>  }

Re: [PATCH 1/4 RESEND] btrfs: fix zstd compression parameter

[David Sterba]

On Tue, Apr 02, 2019 at 06:07:38PM +0800, Anand Jain wrote:
> We let to pass zstd compression parameter even if its not fully written.
> For example:
>   btrfs prop set /btrfs compression zst
>   btrfs prop get /btrfs compression
>      compression=zst
> 
> zlib and lzo are fine.
> 
> Fix it by using the expected number of char in strncmp().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

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

Re: [PATCH 2/4 RESEND] btrfs: fix vanished compression property after failed set

[David Sterba]

On Tue, Apr 02, 2019 at 06:07:40PM +0800, Anand Jain wrote:
> The compression property resets to NULL, instead of the old value if we
> fail to set the new compression parameter.
> 
> btrfs prop get /btrfs compression
>   compression=lzo
> btrfs prop set /btrfs compression zli
>   ERROR: failed to set compression for /btrfs: Invalid argument
> btrfs prop get /btrfs compression
> 
> This is because the compression property ->validate() is successful for
> 'zli' as the strncmp() used the len passed from the userland.
> 
> Fix it by using the expected string length in strncmp().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

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

Re: [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment generation

[Anand Jain]

On 3/4/19 6:04 AM, David Sterba wrote:
> On Tue, Apr 02, 2019 at 06:07:42PM +0800, Anand Jain wrote:
>> When the property fails to pass the prop_handlers::validate() check, the
>> thread should exit with no changes in the kernel, but as we are starting
>> the transaction too early, we have just updated the generation even if
>> there is no change.
> 
> Agreed, this should be fixed.
> 
>>
>> For example:
>> btrfs prop get /btrfs compression
>>   compression=lzo
>> sync
>> btrfs in dump-super /dev/sdb | grep "^generation"
>>   generation		32
>>
>> Try to set an incomplete compression type
>>
>> btrfs prop set /btrfs compression zli
>>    ERROR: failed to set compression for /btrfs: Invalid argument
>> sync
>>
>> Set failed but generation is incremented
>> btrfs in dump-super /dev/sdb | grep "^generation"
>>   generation		33  <--
>>
>> Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which
>> provides flexibility to start the transaction after the
>> prop_handlers::validate() has been checked.
> 
> Please split the changes, the collapsed function make it difficult to
> review. It does not matter if you first clean up the functions and then
> switch the transaction, or the other way around.

  will do.

[1]
>> As of now if the prop_handlers::apply() fails then we still increment
>> the generation, but if there is strong prop_handlers::validate() then
>> the prop_handlers::apply() can never fail.



>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/ioctl.c | 10 ++++------
>>   fs/btrfs/props.c | 59 +++++++++++++++++++++++++-------------------------------
>>   fs/btrfs/props.h |  4 ++--
>>   fs/btrfs/xattr.c |  2 +-
>>   4 files changed, 33 insertions(+), 42 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 28ee9fe6edb4..0eeaf9a68082 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -284,8 +284,7 @@ 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_trans(inode, "btrfs.compression", NULL,
>> -					   0, 0);
>> +		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
>>   		if (ret && ret != -ENODATA)
>>   			goto out_drop;
>>   	} else if (fsflags & FS_COMPR_FL) {
>> @@ -303,14 +302,13 @@ 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_trans(inode, "btrfs.compression", comp,
>> -					   strlen(comp), 0);
>> +		ret = btrfs_set_prop(inode, "btrfs.compression", comp,
>> +				     strlen(comp), 0);
>>   		if (ret)
>>   			goto out_drop;
>>   
>>   	} else {
>> -		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
>> -					   0, 0);
>> +		ret = btrfs_set_prop(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 fb84e76f3b1d..99fa2459ba61 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -56,11 +56,12 @@ 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 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;
>> +	struct btrfs_trans_handle *trans;
>>   	int ret;
>>   
>>   	if (btrfs_root_readonly(BTRFS_I(inode)->root))
>> @@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>>   		return -EINVAL;
>>   
>>   	if (value_len == 0) {
>> +		/* Its called to reset the property */
>> +		trans = btrfs_start_transaction(root, 2);
>> +		if (IS_ERR(trans))
>> +			return PTR_ERR(trans);
>> +
>>   		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
>>   				       NULL, 0, flags);
>> -		if (ret)
>> -			return ret;
>> -
>> -		ret = handler->apply(inode, NULL, 0);
>> -		ASSERT(ret == 0);
>> -
>> -		return ret;
>> +		if (!ret) {
>> +			ret = handler->apply(inode, NULL, 0);
>> +			ASSERT(ret == 0);
>> +		}
>> +		goto out;
> 
> This block would be better in a separate helper, now that it starts the
> transaction, eg.
> 
> 	if (value_len == 0)
> 		return do_reset_property(inode, handler, flags);

  I was thinking same too. Will fix.

>>   	}
>>   
>>   	ret = handler->validate(value, value_len);
>>   	if (ret)
>>   		return ret;
>> +
>> +	trans = btrfs_start_transaction(root, 2);
>> +	if (IS_ERR(trans))
>> +		return PTR_ERR(trans);
>> +
>>   	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
>>   			       value, value_len, flags);
>>   	if (ret)
>> -		return ret;
>> +		goto out;
>> +
>>   	ret = handler->apply(inode, value, value_len);
>>   	if (ret) {
>> +		/* Apply failed. Reset the property. */
>>   		btrfs_setxattr(trans, inode, handler->xattr_name,
>>   				 NULL, 0, flags);
> 
> So that's not new code, but I wonder whether this is correct at all.
> Reset means to the original value or to empty value? The problem here is
> that it's under transaction, so this should be followed by an abort.
> 
> It maybe is possible to safely revert the value without aborting, as the
> inode is locked.

  As I mentioned [1] (above), we have this bug in the original code which
  is impossible to achieve, because apply() can never as the validate()
  has passed. So we don't need additional check in apply() for which
  it may return fail? This works as of now with the compression property.
  It should be possible to follow the same design for other upcoming
  property readmirror and encryption.

Thanks, Anand

>> -		return ret;
>> -	}
>> -
>> -	set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
>> -
>> -	return 0;
>> -}
>> -
>> -int btrfs_set_prop_trans(struct inode *inode, const char *name,
>> -			 const char *value, size_t value_len, int 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) {
>> +	} else {
>> +		set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
>>   		inode_inc_iversion(inode);
>>   		inode->i_ctime = current_time(inode);
>>   		set_bit(BTRFS_INODE_COPY_EVERYTHING,
>> @@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name,
>>   		ret = btrfs_update_inode(trans, root, inode);
>>   		BUG_ON(ret);
>>   	}
>> +
>> +out:
>>   	btrfs_end_transaction(trans);
>>   	return ret;
>>   }

Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

[David Sterba]

On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote:
> > @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
> >  		if (!value)
> >  			continue;
> >  
> > +		/* may be removed */
> 
> This comment brings no value. Either explain *why* it can be removed or
> remove it altogether.

Comment replaced by 

                 /*                                                                                                                                                                                               
                  * This is not strictly necessary as the property should be                                                                                                                                      
                  * valid, but in case it isn't, don't propagate it futher.                                                                                                                                       
                  */

I'd keep it the because that's what the direct opencoding transformation would
do. If we want to remove it, then the reasoning should be provided.

Re: [PATCH] fstests: btrfs/048: amend property validation cases

[Anand Jain]

On 2/4/19 8:51 PM, Nikolay Borisov wrote:
> 
> 
> On 2.04.19 г. 13:07 ч., Anand Jain wrote:
>> Add more property validation cases which are fixed by the patches [1]
>>   [1]
>>    btrfs: fix property validate fail should not increment generation
>>    btrfs: open code btrfs_set_prop in inherit_prop
>>    btrfs: fix vanished compression property after failed set
>>    btrfs: fix zstd compression parameter
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> (As this fstest patch depends on the kernel patches,
>>   this patch isn't sent to fstests mailing list yet).
>>
> 
> Doesn't matter, it's fine to have a test merged before respective kernel
> patches have landed. 

  Ok.

> In fact there is one btrfs test (can't remember
> which one) authored by you which is failing, yet the respective kernel
> patches have never been sent to the mailing list.

  fstests is also a good place to record known issues. Otherwise we can
  create a group of known-issues with in fstests. What do you think?
  Currently we have these two fstests failing because its fix is not
  yet in the kernel.

btrfs/172:
  Exposed bug: Punch hole fails at ENOSPC only for the misaligned offset.
  Because punch hole has to zero a partial block in case of misaligned
  offset. Do you know how to fix? Of late I started believing its a
  limitation rather than bug. If we agree we can decompose and reuse
  btrfs/172 for something else.

btrfs/154:
  Exposed bug: Reappeared missing device fakes its assembly to the
  volume.
  Fix (as indicated in btrfs/154) V5 is in the mailing list, with
  accepted review comments.

Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

[Anand Jain]

On 3/4/19 7:24 AM, David Sterba wrote:
> On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote:
>>> @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>>>   		if (!value)
>>>   			continue;
>>>   
>>> +		/* may be removed */
>>
>> This comment brings no value. Either explain *why* it can be removed or
>> remove it altogether.
> 
> Comment replaced by
> 
>                   /*
>                    * This is not strictly necessary as the property should be
>                    * valid, but in case it isn't, don't propagate it futher.
>                    */
> 
> I'd keep it the because that's what the direct opencoding transformation would
> do. If we want to remove it, then the reasoning should be provided.

   Agreed with the reasoning.

Thanks, Anand