diff mbox series

btrfs: Change how child btrfs_inode inherits xattr compression from parent after expanding btrfs_info member prop_compress

Message ID 1642941780-17773-1-git-send-email-zhanglikernel@gmail.com (mailing list archive)
State New, archived
Headers show
Series btrfs: Change how child btrfs_inode inherits xattr compression from parent after expanding btrfs_info member prop_compress | expand

Commit Message

Li Zhang Jan. 23, 2022, 12:43 p.m. UTC
Hi, the patch I submitted fails xfstest/btrfs/048.

[patch link]
https://lore.kernel.org/linux-btrfs/1642323009-1953-1-git-send-email-zhanglikernel@gmail.com/

This is due to the following reasons.

[Problem Description]
1. Expanded struct btrfs_info member prop_compress to record
compression type and level, but did not change prop_compress allocation and initialization.
2. Inheriting inode still considers the parent inode member prop_compress just
records the compression type, it is wrong to parse prop_compress to compressed xattr.

[Repair method]
1. If the struct btrfs_inode member prop_compress is assigned or initialized to
BTRFS_COMPRESS_NONE , it will combine the compression type and compression level 0.
2. If a btrfs_inode inherits properties from parent, resolve prop_compress
to xattr compression <compression_type>:<compression_type>

But this changes the way the child btrfs_inode inherits properties from the
parent node, because parsing prop_compress as xattr compression <compression_type>:<compression_type>
formats xattr compression and therefore changes the behavior of btrfs.

Specifically:

Old behavior:
$ mkdir <parent directory>
$ btrfs attribute set <parent_dir> compressed zstd
$ btrfs attribute gets <parent_dir>
compressed=zstd
$ mkdir <parent directory>/1
$ touch <parent directory>/2
$ btrfs property gets <parent_dir>/1
compressed=zstd
$ btrfs attribute gets <parent_dir>/2
compressed=zstd

New behavior:
$ mkdir <parent directory>
$ btrfs attribute set <parent_dir> compressed zstd
$ btrfs attribute gets <parent_dir>
compressed=zstd
$ mkdir <parent directory>/1
$ touch <parent directory>/2
$ btrfs property gets <parent_dir>/1
compressed=zstd:3
$ btrfs attribute gets <parent_dir>/2
compressed=zstd:3

[Puzzled]
1. Because it will format the child's btrfs_inode xattr compression,
it still fails the fstest/btrfs/048 test, what can I do to fix it completely?
2. The original way of parsing the struct btrfs_inode member prop_compress was
to get only the compression, so it could return a global compression description,
but now it will combine the compression type and compression level, so this patch returns a
dynamic memory buf and is used by the callee, which Is there a better way to return the compressed
description xattr string?

Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Li Zhang <zhanglikernel@gmail.com>
---
 fs/btrfs/inode.c |  2 +-
 fs/btrfs/props.c | 39 ++++++++++++++++++++++++++++++---------
 2 files changed, 31 insertions(+), 10 deletions(-)
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fb44899..16d40bd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8796,7 +8796,7 @@  struct inode *btrfs_alloc_inode(struct super_block *sb)
 		btrfs_init_metadata_block_rsv(fs_info, &ei->block_rsv,
 					      BTRFS_BLOCK_RSV_DELALLOC);
 	ei->runtime_flags = 0;
-	ei->prop_compress = BTRFS_COMPRESS_NONE;
+	ei->prop_compress = btrfs_compress_combine_type_level(BTRFS_COMPRESS_NONE, 0);
 	ei->defrag_compress = BTRFS_COMPRESS_NONE;
 
 	ei->delayed_node = NULL;
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f07be37..b6d8e6a 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -279,7 +279,7 @@  static int prop_compression_apply(struct inode *inode, const char *value,
 	if (len == 0) {
 		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
 		BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
-		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
+		BTRFS_I(inode)->prop_compress = btrfs_compress_combine_type_level(BTRFS_COMPRESS_NONE, 0);
 		ret = 0;
         goto out;
 	}
@@ -289,7 +289,7 @@  static int prop_compression_apply(struct inode *inode, const char *value,
 	    (len == 4 && strncmp("none", value, 4) == 0)) {
 		BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
 		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
-		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
+		BTRFS_I(inode)->prop_compress = btrfs_compress_combine_type_level(BTRFS_COMPRESS_NONE, 0);
         ret = 0;
         goto out;
 	}
@@ -332,15 +332,24 @@  static int prop_compression_apply(struct inode *inode, const char *value,
 
 static const char *prop_compression_extract(struct inode *inode)
 {
-	switch (BTRFS_I(inode)->prop_compress) {
+    char *type_level = NULL;
+    int type_level_buf_len = 50;
+    
+    type_level = (char *)kmalloc(type_level_buf_len, GFP_NOFS);
+    if (!type_level) {
+        return NULL;
+    }
+	switch (btrfs_compress_type(BTRFS_I(inode)->prop_compress)) {
 	case BTRFS_COMPRESS_ZLIB:
 	case BTRFS_COMPRESS_LZO:
 	case BTRFS_COMPRESS_ZSTD:
-		return btrfs_compress_type2str(BTRFS_I(inode)->prop_compress);
+		return btrfs_compress_type_level2str(btrfs_compress_type(BTRFS_I(inode)->prop_compress), btrfs_compress_level(BTRFS_I(inode)->prop_compress), type_level, type_level_buf_len);
 	default:
 		break;
 	}
-
+    if (type_level) {
+        kfree(type_level);
+    }
 	return NULL;
 }
 
@@ -371,22 +380,26 @@  static int inherit_props(struct btrfs_trans_handle *trans,
 	for (i = 0; i < ARRAY_SIZE(prop_handlers); i++) {
 		const struct prop_handler *h = &prop_handlers[i];
 		const char *value;
+        int is_compression_xattr = !strncmp(h->xattr_name, XATTR_BTRFS_PREFIX "compression", strlen(XATTR_BTRFS_PREFIX "compression"));
 		u64 num_bytes = 0;
 
 		if (!h->inheritable)
 			continue;
 
 		value = h->extract(parent);
+        btrfs_info(fs_info, "inherit_props values:%s", value);
 		if (!value)
 			continue;
-
 		/*
 		 * 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));
-		if (ret)
+		if (ret) {
+            if (is_compression_xattr)
+                kfree(value);
 			continue;
+        }
 
 		/*
 		 * Currently callers should be reserving 1 item for properties,
@@ -400,8 +413,11 @@  static int inherit_props(struct btrfs_trans_handle *trans,
 			ret = btrfs_block_rsv_add(fs_info, trans->block_rsv,
 						  num_bytes,
 						  BTRFS_RESERVE_NO_FLUSH);
-			if (ret)
+			if (ret) {
+                if (is_compression_xattr)
+                    kfree(value);
 				return ret;
+            }
 		}
 
 		ret = btrfs_setxattr(trans, inode, h->xattr_name, value,
@@ -419,10 +435,15 @@  static int inherit_props(struct btrfs_trans_handle *trans,
 		if (need_reserve) {
 			btrfs_block_rsv_release(fs_info, trans->block_rsv,
 					num_bytes, NULL);
-			if (ret)
+			if (ret) {
+                if (is_compression_xattr)
+                    kfree(value);
 				return ret;
+            }
 		}
 		need_reserve = true;
+        if (is_compression_xattr)
+            kfree(value);
 	}
 
 	return 0;