[PATCH 1/2] btrfs: export a helper for compression hard check

[PATCH 1/2] btrfs: export a helper for compression hard check

[Chung-Chiang Cheng]

inode_can_compress will be used outside of inode.c to check the
availability of setting compression flag by xattr. This patch moves
this function as an internal helper and renames it to
btrfs_inode_can_compress.

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/btrfs/btrfs_inode.h | 11 +++++++++++
 fs/btrfs/inode.c       | 15 ++-------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 47e72d72f7d0..32131a5d321b 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -384,6 +384,17 @@ static inline bool btrfs_inode_in_log(struct btrfs_inode *inode, u64 generation)
 	return ret;
 }
 
+/*
+ * Check if the inode has flags compatible with compression
+ */
+static inline bool btrfs_inode_can_compress(const struct btrfs_inode *inode)
+{
+	if (inode->flags & BTRFS_INODE_NODATACOW ||
+	    inode->flags & BTRFS_INODE_NODATASUM)
+		return false;
+	return true;
+}
+
 struct btrfs_dio_private {
 	struct inode *inode;
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 17d5557f98ec..99725e5508f9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -480,17 +480,6 @@ static noinline int add_async_extent(struct async_chunk *cow,
 	return 0;
 }
 
-/*
- * Check if the inode has flags compatible with compression
- */
-static inline bool inode_can_compress(struct btrfs_inode *inode)
-{
-	if (inode->flags & BTRFS_INODE_NODATACOW ||
-	    inode->flags & BTRFS_INODE_NODATASUM)
-		return false;
-	return true;
-}
-
 /*
  * Check if the inode needs to be submitted to compression, based on mount
  * options, defragmentation, properties or heuristics.
@@ -500,7 +489,7 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 
-	if (!inode_can_compress(inode)) {
+	if (!btrfs_inode_can_compress(inode)) {
 		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
 			KERN_ERR "BTRFS: unexpected compression for ino %llu\n",
 			btrfs_ino(inode));
@@ -2020,7 +2009,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 		       (zoned && btrfs_is_data_reloc_root(inode->root)));
 		ret = run_delalloc_nocow(inode, locked_page, start, end,
 					 page_started, nr_written);
-	} else if (!inode_can_compress(inode) ||
+	} else if (!btrfs_inode_can_compress(inode) ||
 		   !inode_need_compress(inode, start, end)) {
 		if (zoned)
 			ret = run_delalloc_zoned(inode, locked_page, start, end,
-- 
2.34.1

[PATCH 2/2] btrfs: do not allow compression on nodatacow files

[Chung-Chiang Cheng]

Compression and nodatacow are mutually exclusive. A similar issue was
fixed by commit f37c563bab429 ("btrfs: add missing check for nocow and
compression inode flags"). Besides ioctl, there is another way to
enable/disable/reset compression directly via xattr. The following
steps will result in a invalid combination.

  $ touch bar
  $ chattr +C bar
  $ lsattr bar
  ---------------C-- bar
  $ setfattr -n btrfs.compression -v zstd bar
  $ lsattr bar
  --------c------C-- bar

To align with the logic in check_fsflags, nocompress will also be
unacceptable after this patch, to prevent mix any compression-related
options with nodatacow.

  $ touch bar
  $ chattr +C bar
  $ lsattr bar
  ---------------C-- bar
  $ setfattr -n btrfs.compression -v zstd bar
  setfattr: bar: Invalid argument
  $ setfattr -n btrfs.compression -v no bar
  setfattr: bar: Invalid argument

Reported-by: Jayce Lin <jaycelin@synology.com>
Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/btrfs/props.c | 16 +++++++++++-----
 fs/btrfs/props.h |  3 ++-
 fs/btrfs/xattr.c |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 1a6d2d5b4b33..5a6f87744c28 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -17,7 +17,8 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
 struct prop_handler {
 	struct hlist_node node;
 	const char *xattr_name;
-	int (*validate)(const char *value, size_t len);
+	int (*validate)(const struct btrfs_inode *inode, const char *value,
+			size_t len);
 	int (*apply)(struct inode *inode, const char *value, size_t len);
 	const char *(*extract)(struct inode *inode);
 	int inheritable;
@@ -55,7 +56,8 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
-int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
+int btrfs_validate_prop(const struct btrfs_inode *inode, const char *name,
+			const char *value, size_t value_len)
 {
 	const struct prop_handler *handler;
 
@@ -69,7 +71,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
 	if (value_len == 0)
 		return 0;
 
-	return handler->validate(value, value_len);
+	return handler->validate(inode, value, value_len);
 }
 
 int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
@@ -252,8 +254,12 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
 	return ret;
 }
 
-static int prop_compression_validate(const char *value, size_t len)
+static int prop_compression_validate(const struct btrfs_inode *inode,
+				     const char *value, size_t len)
 {
+	if (!btrfs_inode_can_compress(inode))
+		return -EINVAL;
+
 	if (!value)
 		return 0;
 
@@ -364,7 +370,7 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		 * This is not strictly necessary as the property should be
 		 * valid, but in case it isn't, don't propagate it further.
 		 */
-		ret = h->validate(value, strlen(value));
+		ret = h->validate(BTRFS_I(inode), value, strlen(value));
 		if (ret)
 			continue;
 
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 40b2c65b518c..2b2ac15ab788 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -13,7 +13,8 @@ void __init btrfs_props_init(void);
 int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		   const char *name, const char *value, size_t value_len,
 		   int flags);
-int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
+int btrfs_validate_prop(const struct btrfs_inode *inode, 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 99abf41b89b9..9632d0ff2038 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -403,7 +403,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 
 	name = xattr_full_name(handler, name);
-	ret = btrfs_validate_prop(name, value, size);
+	ret = btrfs_validate_prop(BTRFS_I(inode), name, value, size);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH 2/2] btrfs: do not allow compression on nodatacow files

[Nikolay Borisov]

On 15.04.22 г. 11:04 ч., Chung-Chiang Cheng wrote:
> Compression and nodatacow are mutually exclusive. A similar issue was
> fixed by commit f37c563bab429 ("btrfs: add missing check for nocow and
> compression inode flags"). Besides ioctl, there is another way to
> enable/disable/reset compression directly via xattr. The following
> steps will result in a invalid combination.
> 
>    $ touch bar
>    $ chattr +C bar
>    $ lsattr bar
>    ---------------C-- bar
>    $ setfattr -n btrfs.compression -v zstd bar
>    $ lsattr bar
>    --------c------C-- bar
> 
> To align with the logic in check_fsflags, nocompress will also be
> unacceptable after this patch, to prevent mix any compression-related
> options with nodatacow.
> 
>    $ touch bar
>    $ chattr +C bar
>    $ lsattr bar
>    ---------------C-- bar
>    $ setfattr -n btrfs.compression -v zstd bar
>    setfattr: bar: Invalid argument
>    $ setfattr -n btrfs.compression -v no bar
>    setfattr: bar: Invalid argument

Also send a test to verify we don't regress this behavior in the future.

> 
> Reported-by: Jayce Lin <jaycelin@synology.com>
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
> ---
>   fs/btrfs/props.c | 16 +++++++++++-----
>   fs/btrfs/props.h |  3 ++-
>   fs/btrfs/xattr.c |  2 +-
>   3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 1a6d2d5b4b33..5a6f87744c28 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -17,7 +17,8 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
>   struct prop_handler {
>   	struct hlist_node node;
>   	const char *xattr_name;
> -	int (*validate)(const char *value, size_t len);
> +	int (*validate)(const struct btrfs_inode *inode, const char *value,
> +			size_t len);
>   	int (*apply)(struct inode *inode, const char *value, size_t len);
>   	const char *(*extract)(struct inode *inode);
>   	int inheritable;
> @@ -55,7 +56,8 @@ find_prop_handler(const char *name,
>   	return NULL;
>   }
>   
> -int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
> +int btrfs_validate_prop(const struct btrfs_inode *inode, const char *name,
> +			const char *value, size_t value_len)
>   {
>   	const struct prop_handler *handler;
>   
> @@ -69,7 +71,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
>   	if (value_len == 0)
>   		return 0;
>   
> -	return handler->validate(value, value_len);
> +	return handler->validate(inode, value, value_len);
>   }
>   
>   int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
> @@ -252,8 +254,12 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
>   	return ret;
>   }
>   
> -static int prop_compression_validate(const char *value, size_t len)
> +static int prop_compression_validate(const struct btrfs_inode *inode,
> +				     const char *value, size_t len)
>   {
> +	if (!btrfs_inode_can_compress(inode))
> +		return -EINVAL;
> +
>   	if (!value)
>   		return 0;
>   
> @@ -364,7 +370,7 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>   		 * This is not strictly necessary as the property should be
>   		 * valid, but in case it isn't, don't propagate it further.
>   		 */
> -		ret = h->validate(value, strlen(value));
> +		ret = h->validate(BTRFS_I(inode), value, strlen(value));
>   		if (ret)
>   			continue;
>   
> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
> index 40b2c65b518c..2b2ac15ab788 100644
> --- a/fs/btrfs/props.h
> +++ b/fs/btrfs/props.h
> @@ -13,7 +13,8 @@ void __init btrfs_props_init(void);
>   int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>   		   const char *name, const char *value, size_t value_len,
>   		   int flags);
> -int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
> +int btrfs_validate_prop(const struct btrfs_inode *inode, 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 99abf41b89b9..9632d0ff2038 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -403,7 +403,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
>   	struct btrfs_root *root = BTRFS_I(inode)->root;
>   
>   	name = xattr_full_name(handler, name);
> -	ret = btrfs_validate_prop(name, value, size);
> +	ret = btrfs_validate_prop(BTRFS_I(inode), name, value, size);
>   	if (ret)
>   		return ret;
>   

Re: [PATCH 1/2] btrfs: export a helper for compression hard check

[Nikolay Borisov]

On 15.04.22 г. 11:04 ч., Chung-Chiang Cheng wrote:
> inode_can_compress will be used outside of inode.c to check the
> availability of setting compression flag by xattr. This patch moves
> this function as an internal helper and renames it to
> btrfs_inode_can_compress.
> 
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>

The whole series LGTM:

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

Still, providin an fstest is highly desirable.

Re: [PATCH 1/2] btrfs: export a helper for compression hard check

[Filipe Manana]

On Fri, Apr 15, 2022 at 04:04:05PM +0800, Chung-Chiang Cheng wrote:
> inode_can_compress will be used outside of inode.c to check the
> availability of setting compression flag by xattr. This patch moves
> this function as an internal helper and renames it to
> btrfs_inode_can_compress.

Btw, the idea was to export the function in a patch following the bug fix
patch. That would imply temporarily duplicating the validation logic in
the bug fix patch, and then the followup patch would export
inode_can_compress() and make inode.c and props.c use it.

That makes the backport to stable easier.

Alternatively, IMO, since it's such a short and trivial change, both
patches could be combined in a single patch. Not everyone might agree
with that however.

Either way, I'm fine with it, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> 
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
> ---
>  fs/btrfs/btrfs_inode.h | 11 +++++++++++
>  fs/btrfs/inode.c       | 15 ++-------------
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 47e72d72f7d0..32131a5d321b 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -384,6 +384,17 @@ static inline bool btrfs_inode_in_log(struct btrfs_inode *inode, u64 generation)
>  	return ret;
>  }
>  
> +/*
> + * Check if the inode has flags compatible with compression
> + */
> +static inline bool btrfs_inode_can_compress(const struct btrfs_inode *inode)
> +{
> +	if (inode->flags & BTRFS_INODE_NODATACOW ||
> +	    inode->flags & BTRFS_INODE_NODATASUM)
> +		return false;
> +	return true;
> +}
> +
>  struct btrfs_dio_private {
>  	struct inode *inode;
>  
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 17d5557f98ec..99725e5508f9 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -480,17 +480,6 @@ static noinline int add_async_extent(struct async_chunk *cow,
>  	return 0;
>  }
>  
> -/*
> - * Check if the inode has flags compatible with compression
> - */
> -static inline bool inode_can_compress(struct btrfs_inode *inode)
> -{
> -	if (inode->flags & BTRFS_INODE_NODATACOW ||
> -	    inode->flags & BTRFS_INODE_NODATASUM)
> -		return false;
> -	return true;
> -}
> -
>  /*
>   * Check if the inode needs to be submitted to compression, based on mount
>   * options, defragmentation, properties or heuristics.
> @@ -500,7 +489,7 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>  
> -	if (!inode_can_compress(inode)) {
> +	if (!btrfs_inode_can_compress(inode)) {
>  		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
>  			KERN_ERR "BTRFS: unexpected compression for ino %llu\n",
>  			btrfs_ino(inode));
> @@ -2020,7 +2009,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>  		       (zoned && btrfs_is_data_reloc_root(inode->root)));
>  		ret = run_delalloc_nocow(inode, locked_page, start, end,
>  					 page_started, nr_written);
> -	} else if (!inode_can_compress(inode) ||
> +	} else if (!btrfs_inode_can_compress(inode) ||
>  		   !inode_need_compress(inode, start, end)) {
>  		if (zoned)
>  			ret = run_delalloc_zoned(inode, locked_page, start, end,
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] btrfs: do not allow compression on nodatacow files

[Filipe Manana]

On Fri, Apr 15, 2022 at 04:04:06PM +0800, Chung-Chiang Cheng wrote:
> Compression and nodatacow are mutually exclusive. A similar issue was
> fixed by commit f37c563bab429 ("btrfs: add missing check for nocow and
> compression inode flags"). Besides ioctl, there is another way to
> enable/disable/reset compression directly via xattr. The following
> steps will result in a invalid combination.
> 
>   $ touch bar
>   $ chattr +C bar
>   $ lsattr bar
>   ---------------C-- bar
>   $ setfattr -n btrfs.compression -v zstd bar
>   $ lsattr bar
>   --------c------C-- bar
> 
> To align with the logic in check_fsflags, nocompress will also be
> unacceptable after this patch, to prevent mix any compression-related
> options with nodatacow.
> 
>   $ touch bar
>   $ chattr +C bar
>   $ lsattr bar
>   ---------------C-- bar
>   $ setfattr -n btrfs.compression -v zstd bar
>   setfattr: bar: Invalid argument
>   $ setfattr -n btrfs.compression -v no bar
>   setfattr: bar: Invalid argument
> 
> Reported-by: Jayce Lin <jaycelin@synology.com>
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/props.c | 16 +++++++++++-----
>  fs/btrfs/props.h |  3 ++-
>  fs/btrfs/xattr.c |  2 +-
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 1a6d2d5b4b33..5a6f87744c28 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -17,7 +17,8 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
>  struct prop_handler {
>  	struct hlist_node node;
>  	const char *xattr_name;
> -	int (*validate)(const char *value, size_t len);
> +	int (*validate)(const struct btrfs_inode *inode, const char *value,
> +			size_t len);
>  	int (*apply)(struct inode *inode, const char *value, size_t len);
>  	const char *(*extract)(struct inode *inode);
>  	int inheritable;
> @@ -55,7 +56,8 @@ find_prop_handler(const char *name,
>  	return NULL;
>  }
>  
> -int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
> +int btrfs_validate_prop(const struct btrfs_inode *inode, const char *name,
> +			const char *value, size_t value_len)
>  {
>  	const struct prop_handler *handler;
>  
> @@ -69,7 +71,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
>  	if (value_len == 0)
>  		return 0;
>  
> -	return handler->validate(value, value_len);
> +	return handler->validate(inode, value, value_len);
>  }
>  
>  int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
> @@ -252,8 +254,12 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
>  	return ret;
>  }
>  
> -static int prop_compression_validate(const char *value, size_t len)
> +static int prop_compression_validate(const struct btrfs_inode *inode,
> +				     const char *value, size_t len)
>  {
> +	if (!btrfs_inode_can_compress(inode))
> +		return -EINVAL;
> +
>  	if (!value)
>  		return 0;
>  
> @@ -364,7 +370,7 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>  		 * This is not strictly necessary as the property should be
>  		 * valid, but in case it isn't, don't propagate it further.
>  		 */
> -		ret = h->validate(value, strlen(value));
> +		ret = h->validate(BTRFS_I(inode), value, strlen(value));
>  		if (ret)
>  			continue;
>  
> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
> index 40b2c65b518c..2b2ac15ab788 100644
> --- a/fs/btrfs/props.h
> +++ b/fs/btrfs/props.h
> @@ -13,7 +13,8 @@ void __init btrfs_props_init(void);
>  int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>  		   const char *name, const char *value, size_t value_len,
>  		   int flags);
> -int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
> +int btrfs_validate_prop(const struct btrfs_inode *inode, 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 99abf41b89b9..9632d0ff2038 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -403,7 +403,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  
>  	name = xattr_full_name(handler, name);
> -	ret = btrfs_validate_prop(name, value, size);
> +	ret = btrfs_validate_prop(BTRFS_I(inode), name, value, size);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] btrfs: export a helper for compression hard check

[David Sterba]

On Fri, Apr 15, 2022 at 04:04:05PM +0800, Chung-Chiang Cheng wrote:
> inode_can_compress will be used outside of inode.c to check the
> availability of setting compression flag by xattr. This patch moves
> this function as an internal helper and renames it to
> btrfs_inode_can_compress.
> 
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>

Patches 1 and 2 added to misc-next, thanks.