[PATCH 1/5] btrfs-progs: Cleanup open-coded btrfs_chunk_item_size

[PATCH 1/5] btrfs-progs: Cleanup open-coded btrfs_chunk_item_size

[Qu Wenruo]

In btrfs_check_chunk_valid() we calculates chunk item using open code.

use btrfs_chunk_item_size() to replace them.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 volumes.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/volumes.c b/volumes.c
index b350e259..62e23aee 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1685,6 +1685,7 @@ int btrfs_check_chunk_valid(struct btrfs_root *root,
 	u16 num_stripes;
 	u16 sub_stripes;
 	u64 type;
+	u32 chunk_ondisk_size;
 
 	length = btrfs_chunk_length(leaf, chunk);
 	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
@@ -1724,16 +1725,16 @@ int btrfs_check_chunk_valid(struct btrfs_root *root,
 			BTRFS_BLOCK_GROUP_PROFILE_MASK) & type);
 		return -EIO;
 	}
+
+	chunk_ondisk_size = btrfs_chunk_item_size(num_stripes);
 	/*
 	 * Btrfs_chunk contains at least one stripe, and for sys_chunk
 	 * it can't exceed the system chunk array size
 	 * For normal chunk, it should match its chunk item size.
 	 */
 	if (num_stripes < 1 ||
-	    (slot == -1 && sizeof(struct btrfs_stripe) * num_stripes >
-	     BTRFS_SYSTEM_CHUNK_ARRAY_SIZE) ||
-	    (slot >= 0 && sizeof(struct btrfs_stripe) * (num_stripes - 1) >
-	     btrfs_item_size_nr(leaf, slot))) {
+	    (slot == -1 && chunk_ondisk_size > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE) ||
+	    (slot >= 0 && chunk_ondisk_size > btrfs_item_size_nr(leaf, slot))) {
 		error("invalid num_stripes: %u", num_stripes);
 		return -EIO;
 	}
-- 
2.12.2

[PATCH 2/5] btrfs-progs: Enhance chunk item validation check

[Qu Wenruo]

btrfs_check_chunk_valid() doesn't check if
1) chunk flag has conflicting flags
   For example chunk type DATA|METADATA|RAID1|RAID10 is completely
   invalid, while current check_chunk_valid() can't detect it.
2) num_stripes is invalid for RAID10
   Num_stripes 5 is not valid for RAID10.

This patch will enhance btrfs_check_chunk_valid() to handle above cases.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 volumes.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index 62e23aee..1e352869 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1725,6 +1725,19 @@ int btrfs_check_chunk_valid(struct btrfs_root *root,
 			BTRFS_BLOCK_GROUP_PROFILE_MASK) & type);
 		return -EIO;
 	}
+	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
+		error("missing chunk type flag: %llu", type);
+		return -EIO;
+	}
+	if (!is_power_of_2(type & BTRFS_BLOCK_GROUP_PROFILE_MASK)) {
+		error("conflicting chunk type detected: %llu", type);
+		return -EIO;
+	}
+	if ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) &&
+	    !is_power_of_2(type & BTRFS_BLOCK_GROUP_PROFILE_MASK)) {
+		error("conflicting chunk profile detected: %llu", type);
+		return -EIO;
+	}
 
 	chunk_ondisk_size = btrfs_chunk_item_size(num_stripes);
 	/*
@@ -1741,7 +1754,8 @@ int btrfs_check_chunk_valid(struct btrfs_root *root,
 	/*
 	 * Device number check against profile
 	 */
-	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes == 0) ||
+	if ((type & BTRFS_BLOCK_GROUP_RAID10 && (sub_stripes != 2 ||
+		  !IS_ALIGNED(num_stripes, sub_stripes))) ||
 	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
 	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
 	    (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
-- 
2.12.2

[PATCH 3/5] btrfs-progs: check: Reuse btrfs_check_chunk_valid in lowmem mode

[Qu Wenruo]

Before this patch, btrfs check lowmem mode manually checks found chunk
item, even we already have the generic chunk validation checker,
btrfs_check_chunk_valid().

This patch will use btrfs_check_chunk_valid() to replace open-coded
chunk validation checker in check_chunk_item().

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 cmds-check.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 17b7efbf..58b73608 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -11110,11 +11110,9 @@ static int check_chunk_item(struct btrfs_fs_info *fs_info,
 	struct btrfs_block_group_item *bi;
 	struct btrfs_block_group_item bg_item;
 	struct btrfs_dev_extent *ptr;
-	u32 sectorsize = btrfs_super_sectorsize(fs_info->super_copy);
 	u64 length;
 	u64 chunk_end;
 	u64 type;
-	u64 profile;
 	int num_stripes;
 	u64 offset;
 	u64 objectid;
@@ -11126,25 +11124,15 @@ static int check_chunk_item(struct btrfs_fs_info *fs_info,
 	chunk = btrfs_item_ptr(eb, slot, struct btrfs_chunk);
 	length = btrfs_chunk_length(eb, chunk);
 	chunk_end = chunk_key.offset + length;
-	if (!IS_ALIGNED(length, sectorsize)) {
-		error("chunk[%llu %llu) not aligned to %u",
-			chunk_key.offset, chunk_end, sectorsize);
-		err |= BYTES_UNALIGNED;
+	ret = btrfs_check_chunk_valid(extent_root, eb, chunk, slot,
+				      chunk_key.offset);
+	if (ret < 0) {
+		error("chunk[%llu %llu) is invalid", chunk_key.offset,
+			chunk_end);
+		err |= BYTES_UNALIGNED | UNKNOWN_TYPE;
 		goto out;
 	}
-
 	type = btrfs_chunk_type(eb, chunk);
-	profile = type & BTRFS_BLOCK_GROUP_PROFILE_MASK;
-	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
-		error("chunk[%llu %llu) has no chunk type",
-			chunk_key.offset, chunk_end);
-		err |= UNKNOWN_TYPE;
-	}
-	if (profile && (profile & (profile - 1))) {
-		error("chunk[%llu %llu) multiple profiles detected: %llx",
-			chunk_key.offset, chunk_end, profile);
-		err |= UNKNOWN_TYPE;
-	}
 
 	bg_key.objectid = chunk_key.offset;
 	bg_key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
-- 
2.12.2

[PATCH 4/5] btrfs-progs: Introduce function to get correct stripe length

[Qu Wenruo]

Introduce a new function, btrfs_get_chunk_stripe_len() to get correct
stripe length.
This is very handy for lowmem mode, which checks the mapping between
device extent and chunk item.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 volumes.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 volumes.h |  3 +++
 2 files changed, 47 insertions(+)

diff --git a/volumes.c b/volumes.c
index 1e352869..8f529051 100644
--- a/volumes.c
+++ b/volumes.c
@@ -2284,3 +2284,47 @@ out:
 
 	return ret;
 }
+
+/*
+ * Get stripe length from chunk item and its stripe items
+ *
+ * Caller should only call this function after validating the chunk item
+ * by using btrfs_check_chunk_valid().
+ */
+u64 btrfs_stripe_length(struct btrfs_fs_info *fs_info,
+			struct extent_buffer *leaf,
+			struct btrfs_chunk *chunk)
+{
+	u64 stripe_len;
+	u64 chunk_len;
+	u32 num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
+	u64 profile = btrfs_chunk_type(leaf, chunk) &
+		      BTRFS_BLOCK_GROUP_PROFILE_MASK;
+
+	chunk_len = btrfs_chunk_length(leaf, chunk);
+
+	switch (profile) {
+	case 0: /* Single profile */
+	case BTRFS_BLOCK_GROUP_RAID1:
+	case BTRFS_BLOCK_GROUP_DUP:
+		stripe_len = chunk_len;
+		break;
+	case BTRFS_BLOCK_GROUP_RAID0:
+		stripe_len = chunk_len / num_stripes;
+		break;
+	case BTRFS_BLOCK_GROUP_RAID5:
+		stripe_len = chunk_len / (num_stripes - 1);
+		break;
+	case BTRFS_BLOCK_GROUP_RAID6:
+		stripe_len = chunk_len / (num_stripes - 2);
+		break;
+	case BTRFS_BLOCK_GROUP_RAID10:
+		stripe_len = chunk_len / (num_stripes /
+				btrfs_chunk_sub_stripes(leaf, chunk));
+		break;
+	default:
+		/* Invalid chunk profile found */
+		BUG_ON(1);
+	}
+	return stripe_len;
+}
diff --git a/volumes.h b/volumes.h
index 699b0bae..fc0a775b 100644
--- a/volumes.h
+++ b/volumes.h
@@ -246,4 +246,7 @@ int btrfs_check_chunk_valid(struct btrfs_root *root,
 			    struct extent_buffer *leaf,
 			    struct btrfs_chunk *chunk,
 			    int slot, u64 logical);
+u64 btrfs_stripe_length(struct btrfs_fs_info *fs_info,
+			struct extent_buffer *leaf,
+			struct btrfs_chunk *chunk);
 #endif
-- 
2.12.2

[PATCH 5/5] btrfs-progs: lowmem check: Fix false alert on missing chunk or dev extent

[Qu Wenruo]

When checking chunk or dev extent, lowmem mode uses chunk length as dev
extent length, and if they mismatch, report missing chunk or dev extent
like:
------
ERROR: chunk[256 4324327424) stripe 0 did not find the related dev extent
ERROR: chunk[256 4324327424) stripe 1 did not find the related dev extent
ERROR: chunk[256 4324327424) stripe 2 did not find the related dev extent
------

However, only for Single/DUP/RAID1 profiles chunk length is the same as
dev extent length.
For other profiles, this will cause tons of false alert.

Fix it by using correct stripe length when checking chunk and dev extent
items.

Reported-by: Kai Krakow <hurikhan77@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
Test cases for this bug will follow as another patchset, since we need
some infrastructure update to support multi-device mkfs/check/mount.
---
 cmds-check.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 58b73608..eb3eb9e9 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -10872,7 +10872,12 @@ static int check_dev_extent_item(struct btrfs_fs_info *fs_info,
 
 	l = path.nodes[0];
 	chunk = btrfs_item_ptr(l, path.slots[0], struct btrfs_chunk);
-	if (btrfs_chunk_length(l, chunk) != length)
+	ret = btrfs_check_chunk_valid(chunk_root, l, chunk, path.slots[0],
+				      chunk_key.offset);
+	if (ret < 0)
+		goto out;
+
+	if (btrfs_stripe_length(fs_info, l, chunk) != length)
 		goto out;
 
 	num_stripes = btrfs_chunk_num_stripes(l, chunk);
@@ -11112,6 +11117,7 @@ static int check_chunk_item(struct btrfs_fs_info *fs_info,
 	struct btrfs_dev_extent *ptr;
 	u64 length;
 	u64 chunk_end;
+	u64 stripe_len;
 	u64 type;
 	int num_stripes;
 	u64 offset;
@@ -11161,6 +11167,7 @@ static int check_chunk_item(struct btrfs_fs_info *fs_info,
 	}
 
 	num_stripes = btrfs_chunk_num_stripes(eb, chunk);
+	stripe_len = btrfs_stripe_length(fs_info, eb, chunk);
 	for (i = 0; i < num_stripes; i++) {
 		btrfs_release_path(&path);
 		btrfs_init_path(&path);
@@ -11180,7 +11187,7 @@ static int check_chunk_item(struct btrfs_fs_info *fs_info,
 		offset = btrfs_dev_extent_chunk_offset(leaf, ptr);
 		if (objectid != chunk_key.objectid ||
 		    offset != chunk_key.offset ||
-		    btrfs_dev_extent_length(leaf, ptr) != length)
+		    btrfs_dev_extent_length(leaf, ptr) != stripe_len)
 			goto not_match_dev;
 		continue;
 not_match_dev:
-- 
2.12.2

Re: [PATCH 2/5] btrfs-progs: Enhance chunk item validation check

[David Sterba]

On Mon, May 15, 2017 at 04:27:39PM +0800, Qu Wenruo wrote:
> btrfs_check_chunk_valid() doesn't check if
> 1) chunk flag has conflicting flags
>    For example chunk type DATA|METADATA|RAID1|RAID10 is completely
>    invalid, while current check_chunk_valid() can't detect it.
> 2) num_stripes is invalid for RAID10
>    Num_stripes 5 is not valid for RAID10.
> 
> This patch will enhance btrfs_check_chunk_valid() to handle above cases.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Most tests fail with this patch, mkfs or from restored images created.
Simple test-check or test-misc fails in the first test. Have I missed
some other patch or test update?

Re: [PATCH 2/5] btrfs-progs: Enhance chunk item validation check

[Qu Wenruo]

At 05/30/2017 01:39 AM, David Sterba wrote:
> On Mon, May 15, 2017 at 04:27:39PM +0800, Qu Wenruo wrote:
>> btrfs_check_chunk_valid() doesn't check if
>> 1) chunk flag has conflicting flags
>>     For example chunk type DATA|METADATA|RAID1|RAID10 is completely
>>     invalid, while current check_chunk_valid() can't detect it.
>> 2) num_stripes is invalid for RAID10
>>     Num_stripes 5 is not valid for RAID10.
>>
>> This patch will enhance btrfs_check_chunk_valid() to handle above cases.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> 
> Most tests fail with this patch, mkfs or from restored images created.
> Simple test-check or test-misc fails in the first test. Have I missed
> some other patch or test update?
> 
> 
Sorry, I formatted patch based on an old branch, which doesn't have the 
extra fix for single profile.

I'll update the branch and add the needed test case in next branch, 
along with infrastructure update to make loop device setup more easy to use.

Thanks,
Qu