[PATCH 0/4] btrfs-progs: check: Repair invalid inode mode in subvolume trees

[PATCH 0/4] btrfs-progs: check: Repair invalid inode mode in subvolume trees

[Qu Wenruo]

Before this patch, btrfs check can only repair bad free space cache
inode mode (as it was the first case detected by tree-checker and reported)

But now what may happen finally happened, we have user reorting bad
inode mode in subvolumes trees.

Although the reported get the fs fixed by removing the offending old
files, it's still a bad thing that "btrfs check" can't fix it.

This patch will bring the repair functionality to all inodes, along with
needed test image.

Qu Wenruo (4):
  btrfs-progs: check/common: Make repair_imode_common() to handle inodes
    in subvolume trees
  btrfs-progs: check/lowmem: Repair bad imode early
  btrfs-progs: check/original: Fix inode mode in subvolume trees
  btrfs-progs: tests/fsck: Add new images for inode mode repair
    functionality

 check/main.c                                  |  32 ++++--
 check/mode-common.c                           |  96 +++++++++++++++---
 check/mode-common.h                           |   2 +
 check/mode-lowmem.c                           |  39 +++++++
 .../039-bad-inode-mode/.lowmem_repairable     |   0
 .../bad_free_space_cache_imode.raw.xz}        | Bin
 .../bad_regular_file_imode.img.xz             | Bin 0 -> 2060 bytes
 7 files changed, 147 insertions(+), 22 deletions(-)
 create mode 100644 tests/fsck-tests/039-bad-inode-mode/.lowmem_repairable
 rename tests/fsck-tests/{039-bad-free-space-cache-inode-mode/test.raw.xz => 039-bad-inode-mode/bad_free_space_cache_imode.raw.xz} (100%)
 create mode 100644 tests/fsck-tests/039-bad-inode-mode/bad_regular_file_imode.img.xz

-- 
2.23.0

[PATCH 1/4] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees

[Qu Wenruo]

Before this patch, repair_imode_common() can only handle two types of
inodes:
- Free space cache inodes
- ROOT DIR inodes

For inodes in subvolume trees, the problem is how to determine the
correct imode, thus it was not implemented.

However there are more reports of incorrect imode in subvolume trees, we
need to support such fix.

So this patch adds a new function, detect_imode(), to detect (or call it
educated guess) imode for inodes in subvolume trees.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 83 insertions(+), 13 deletions(-)

diff --git a/check/mode-common.c b/check/mode-common.c
index 195b6efaa7aa..807d7daf98a6 100644
--- a/check/mode-common.c
+++ b/check/mode-common.c
@@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	return ret;
 }
 
+static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
+			u32 *imode_ret)
+{
+	struct btrfs_key key;
+	struct btrfs_inode_item *iitem;
+	const u32 priv = 0700;
+	u64 ino;
+	u32 imode = S_IFREG;
+	int ret = 0;
+
+	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+	ino = key.objectid;
+	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
+			       struct btrfs_inode_item);
+
+	/*
+	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
+	 * to BLK.
+	 */
+	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
+		imode = S_IFBLK;
+		goto out;
+	}
+
+	/* root inode */
+	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
+		imode = S_IFDIR;
+		goto out;
+	}
+
+	while (1) {
+		ret = btrfs_next_item(root, path);
+		if (ret > 0) {
+			/* No determining result found, falls back to REG */
+			ret = 0;
+			goto out;
+		}
+		if (ret < 0)
+			goto out;
+		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+		if (key.objectid != ino)
+			goto out;
+
+		/*
+		 * We ignore some types to make life easier:
+		 * - INODE_REF
+		 *   We need to do a full tree search, which can fail for
+		 *   corrupted fs, but not worthy compared to other easier
+		 *   to determine types.
+		 * - XATTR
+		 *   Both REG and DIR can have xattr, so not useful
+		 */
+		switch (key.type) {
+		case BTRFS_DIR_ITEM_KEY:
+		case BTRFS_DIR_INDEX_KEY:
+			imode = S_IFDIR;
+			goto out;
+		case BTRFS_EXTENT_DATA_KEY:
+			/*
+			 * Both REG and LINK could have EXTENT_DATA.
+			 * We just fall back to REG as user can inspect the
+			 * content.
+			 */
+			imode = S_IFREG;
+			goto out;
+		}
+	}
+
+out:
+	/* Set default value even when something wrong happened */
+	*imode_ret = (imode | priv);
+	return ret;
+}
+
 /*
  * Reset the inode mode of the inode specified by @path.
  *
@@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
 	u32 imode;
 	int ret;
 
-	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
-		error(
-		"repair inode mode outside of root tree is not supported yet");
-		return -ENOTTY;
-	}
 	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
 	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
-	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
-	    !is_fstree(key.objectid)) {
-		error("unsupported ino %llu", key.objectid);
-		return -ENOTTY;
+	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
+		/* In root tree we only have two possible imode */
+		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
+			imode = S_IFDIR | 0755;
+		else
+			imode = S_IFREG | 0600;
+	} else {
+		detect_imode(root, path, &imode);
+		/* Ignore error returned, just use the default value returned */
 	}
-	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
-		imode = 040755;
-	else
-		imode = 0100600;
 
 	trans = btrfs_start_transaction(root, 1);
 	if (IS_ERR(trans)) {
-- 
2.23.0

[PATCH 2/4] btrfs-progs: check/lowmem: Repair bad imode early

[Qu Wenruo]

For lowmem mode, if we hit a bad inode mode, normally it is reported
when we checking the DIR_INDEX/DIR_ITEM of the parent inode.

If we didn't repair at that timing, the error will be recorded even we
fixed it later.

So this patch will check for INODE_ITEM_MISMATCH error type, and if it's
really caused by invalid imode, repair it and clear the error.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-lowmem.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 5f7f101daab1..5d0c520217fa 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -1550,6 +1550,35 @@ static int lowmem_delete_corrupted_dir_item(struct btrfs_root *root,
 	return ret;
 }
 
+static int try_repair_imode(struct btrfs_root *root, u64 ino)
+{
+	struct btrfs_inode_item *iitem;
+	struct btrfs_path path;
+	struct btrfs_key key;
+	int ret;
+
+	key.objectid = ino;
+	key.type = BTRFS_INODE_ITEM_KEY;
+	key.offset = 0;
+	btrfs_init_path(&path);
+
+	ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0)
+		goto out;
+	iitem = btrfs_item_ptr(path.nodes[0], path.slots[0],
+			       struct btrfs_inode_item);
+	if (!is_valid_imode(btrfs_inode_mode(path.nodes[0], iitem))) {
+		ret = repair_imode_common(root, &path);
+	} else {
+		ret = -ENOTTY;
+	}
+out:
+	btrfs_release_path(&path);
+	return ret;
+}
+
 /*
  * Call repair_inode_item_missing and repair_ternary_lowmem to repair
  *
@@ -1574,6 +1603,16 @@ static int repair_dir_item(struct btrfs_root *root, struct btrfs_key *di_key,
 			err &= ~(INODE_ITEM_MISMATCH | INODE_ITEM_MISSING);
 	}
 
+	if (err & INODE_ITEM_MISMATCH) {
+		/*
+		 * INODE_ITEM mismatch can be caused by bad imode,
+		 * so check if it's a bad imode, then repair if possible.
+		 */
+		ret = try_repair_imode(root, ino);
+		if (!ret)
+			err &= ~INODE_ITEM_MISMATCH;
+	}
+
 	if (err & ~(INODE_ITEM_MISMATCH | INODE_ITEM_MISSING)) {
 		ret = repair_ternary_lowmem(root, dirid, ino, index, namebuf,
 					    name_len, filetype, err);
-- 
2.23.0

[PATCH 3/4] btrfs-progs: check/original: Fix inode mode in subvolume trees

[Qu Wenruo]

To make original mode to repair imode error in subvolume trees, this
patch will do:
- Remove the show-stopper checks for root->objectid.
  Now repair_imode_original() will accept inodes in subvolume trees.

- Export detect_imode() for original mode
  Due to the call requirement, original mode must use an existing trans
  handler to do the repair, thus we need to re-implement most of the
  work done in repair_imode_common().

- Make repair_imode_original() to use detect_imode.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c        | 32 +++++++++++++++++++++++---------
 check/mode-common.c |  4 ++--
 check/mode-common.h |  2 ++
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/check/main.c b/check/main.c
index 2e16b4e6f05b..8987d13c72e0 100644
--- a/check/main.c
+++ b/check/main.c
@@ -2771,18 +2771,31 @@ static int repair_imode_original(struct btrfs_trans_handle *trans,
 				 struct btrfs_path *path,
 				 struct inode_record *rec)
 {
+	struct btrfs_key key;
 	int ret;
 	u32 imode;
 
-	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID)
-		return -ENOTTY;
-	if (rec->ino != BTRFS_ROOT_TREE_DIR_OBJECTID || !is_fstree(rec->ino))
-		return -ENOTTY;
+	key.objectid = rec->ino;
+	key.type = BTRFS_INODE_ITEM_KEY;
+	key.offset = 0;
 
-	if (rec->ino == BTRFS_ROOT_TREE_DIR_OBJECTID)
-		imode = 040755;
-	else
-		imode = 0100600;
+	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0)
+		return ret;
+
+	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
+		/* In root tree we only have two possible imode */
+		if (rec->ino == BTRFS_ROOT_TREE_OBJECTID)
+			imode = S_IFDIR | 0755;
+		else
+			imode = S_IFREG | 0600;
+	} else {
+		detect_imode(root, path, &imode);
+		/* Ignore error returned, just use the default value returned */
+	}
+	btrfs_release_path(path);
 	ret = reset_imode(trans, root, path, rec->ino, imode);
 	if (ret < 0)
 		return ret;
@@ -2810,7 +2823,8 @@ static int try_repair_inode(struct btrfs_root *root, struct inode_record *rec)
 			     I_ERR_FILE_NBYTES_WRONG |
 			     I_ERR_INLINE_RAM_BYTES_WRONG |
 			     I_ERR_MISMATCH_DIR_HASH |
-			     I_ERR_UNALIGNED_EXTENT_REC)))
+			     I_ERR_UNALIGNED_EXTENT_REC |
+			     I_ERR_INVALID_IMODE)))
 		return rec->errors;
 
 	/*
diff --git a/check/mode-common.c b/check/mode-common.c
index 807d7daf98a6..ab451749e20c 100644
--- a/check/mode-common.c
+++ b/check/mode-common.c
@@ -836,8 +836,8 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	return ret;
 }
 
-static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
-			u32 *imode_ret)
+int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
+		 u32 *imode_ret)
 {
 	struct btrfs_key key;
 	struct btrfs_inode_item *iitem;
diff --git a/check/mode-common.h b/check/mode-common.h
index 161b84a8deb0..67db89f20edb 100644
--- a/check/mode-common.h
+++ b/check/mode-common.h
@@ -126,6 +126,8 @@ int delete_corrupted_dir_item(struct btrfs_trans_handle *trans,
 			      struct btrfs_root *root,
 			      struct btrfs_key *di_key, char *namebuf,
 			      u32 namelen);
+int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
+		 u32 *imode_ret);
 int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		struct btrfs_path *path, u64 ino, u32 mode);
 int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path);
-- 
2.23.0

[PATCH 4/4] btrfs-progs: tests/fsck: Add new images for inode mode repair functionality

[Qu Wenruo]

Add new test image for imode repair in subvolume trees.

Also rename the existing test case 039-bad-free-space-cache-inode-mode
to 039-bad-inode-mode, since now we can fix all bad imode.

And add the beacon file for lowmem test.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../039-bad-inode-mode/.lowmem_repairable        |   0
 .../bad_free_space_cache_imode.raw.xz}           | Bin
 .../bad_regular_file_imode.img.xz                | Bin 0 -> 2060 bytes
 3 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/fsck-tests/039-bad-inode-mode/.lowmem_repairable
 rename tests/fsck-tests/{039-bad-free-space-cache-inode-mode/test.raw.xz => 039-bad-inode-mode/bad_free_space_cache_imode.raw.xz} (100%)
 create mode 100644 tests/fsck-tests/039-bad-inode-mode/bad_regular_file_imode.img.xz

diff --git a/tests/fsck-tests/039-bad-inode-mode/.lowmem_repairable b/tests/fsck-tests/039-bad-inode-mode/.lowmem_repairable
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/tests/fsck-tests/039-bad-free-space-cache-inode-mode/test.raw.xz b/tests/fsck-tests/039-bad-inode-mode/bad_free_space_cache_imode.raw.xz
similarity index 100%
rename from tests/fsck-tests/039-bad-free-space-cache-inode-mode/test.raw.xz
rename to tests/fsck-tests/039-bad-inode-mode/bad_free_space_cache_imode.raw.xz
diff --git a/tests/fsck-tests/039-bad-inode-mode/bad_regular_file_imode.img.xz b/tests/fsck-tests/039-bad-inode-mode/bad_regular_file_imode.img.xz
new file mode 100644
index 0000000000000000000000000000000000000000..bbdd17de1fb9e59d26d2b0502fff326f019be011
GIT binary patch
literal 2060
zcmV+n2=n*-H+ooF000E$*0e?f03iV!0000G&sfah5B~?rT>wRyj;C3^v%$$4d1oRm
zhA1@4!K86y=JF%i%5FwXCo1%)6gh&*5vGw=-Bj#y{x65M*-1+G4!lf`bW+BqyX<nI
zX>0#KDM;y(0T@FT;0J$&unGzx5VPj~*lxWv?pH4e3>)mmv4?lb0ocO~C8+S@`kgat
z91vZ;`v*9#EUF{=4NuC!E&oT`>E^~9dAS)tCUIor_@+y!<YSP>^G{j*i;LP1iJ3HZ
zhJHIrdnKRlC=rZGe3RyEt5&kZu;FB**-BspaqNBpGT}!X^+6~Y3bJ;(UPEFMhAq+w
zlILfo586KJT53zTWcBehL$SZAS8IYWDIoueT3wZWIGy!CsO@@ag2c833P~k7qvO{O
zF{y99tegz900OE+h4%!{IpqjVN4nb<;*|=aFX_DOnItK<l5wBY4?Jn+rX#hq6ACV^
zzy}R5JPb>mGi}Vh1wv5ddUBexpxV>&nQ!6;C5YM!ofWvH^HoQzwcz!%{r`zl%&Tr+
zI*Joz5WZ2XIS;$uV>0`ZQ0lI<G~+|%%h3CcjOP*Q)-!p1?s&Wc{(dM!(IC{UvLkGv
zxpV_Emu6~0v!GHHxr0ob;;LpM$JF?&H;&d|umK4k$~odM=`WX#_DOHaa{g&j&4fA#
zD}rHi)}Zk4?Wxd<L5#6<o{T6E0+>$%+Sfe0;%dehBQdp18OfsmF<4f3N=o_**GS2%
zxwKW$JcC0H97RoRkwZ4r5!NiI?O|Vwj)=U(o-$t#EbiwCO<?^1VkT}ECpy;+VD6fw
zgW_jd{(NT4^0ptVU?h}G^wjEWh(K+<q~wdk7t&KbCQNaXi+B8hpx>x~5%BX<dlv$K
z==)s!J7x&BAVR3^>&r8)W}5)Gk?jtGoiw%)fpQT?)=|fti!8$9S-$>BU}`H5*xycG
zV@sq5nuWMb8C9X(e(B4zIp5WtHmmmM^VPgzaKx1*z4$DXGVUbB&?h_{hmG35m(BhF
zIJHz)7dT%kyn!v5qmjYb;hd(*q&D@-IBBQ_%IM{U4P0K{ANn0KHB?Cun)R28fzw$i
z)Nv7&;F{omy*<$vpc}Mw$Zrw@oMe;aSl>PDFoTZZw8WA`hvuIu*2zTN+-oH&P}aA9
zE@J=fvLIYM{)|4h3|t(lFK4DdpE_h`L}g;tY<?v5z(K4eW(-65*QVVxC}PkSG+Q(>
zp@IvIq83TK7Cu%efxZc5-BF|+_H3^8^O=1kyx<Qn*KFm#e|<t;KG8yI4$l^zU*%Km
zxeIB3Gx5(@p>QXhJww)q0g_}55`xsdcWe1{(Iohyh@IN!!mtp5y(+Uwe%tM>13W@F
zMs|@j``W4CEJS)W0ylf&8d=JFw1E5D@+-7&lxdW(Cv3R}Mvu0X(~8Sj^<N9=Z0~~u
z>J$tG6lZJfB$wbXVN=HJjy#ch5-!2l<iD3*ZhI^@g_Zc`C&CJ$Z>8|LBft<ARU8)|
zz;y;ai}bD+G8d2|!fBi~O@>(pm;z*|J|=aH9O~1)c#G@N3hP(Y^g5t3<`r|2ij}IC
z29yEU`W12ApHuTY8CHL$QJ;ENl3iAvsMCO~tW}YVelOso6!VPND7pQWfWyd{>=>+Y
z{V}{KXqtMAfWaX7xgeW_4wFGgFl9uSVi2d8DjMve%9l;m^#&{=+U993Mg~fr_BktZ
z-2~61uNMNp5`dsSV>AB2lHM61C75J&sW*8zaj3LJ=z)T-mo)|ZJl%Y}qL;Bx?0(5f
zUMUX%MeZ1<Lq^Gtbf4}HmpH%p;II1_4aA<OC*jPRmE}St;Rt~w+Gv|uIgo$f$Lecc
z?nHVcG9gVAj>N6A(U)uvA$)U+iu`;38hXZ|D^!{(NKMDUK^Hg_4LRbCgD)0~^}jnU
zNkES-dD|N9WrZ~RyRmuEC$MbO7@u`o>X92HiH^PA*AIzl;WFPpGkhnuh$r7c7P*eO
z<yCbrKge#qAEl|xN2toOGPCOG0-A9D%g=P%y7kfy17gd-jZzxflTBq|Ctk?boWGkU
z0gN7_^L@PUdsKWd^=Zar#}KJ1n#3~|_Rt2O85ho2m`2lcLwi%IyC2^pBBP7vr&htq
z8<<MvK8tQw^^f-TXgqa}0batcF~ZuJNh|5PAVIvcETxbk*R`xK??-mxfPeJJCV|oP
z>8$;CRw@4nMlwN-^Vlq?Auh*t*(b9HZQl8OL#h&1@0x@5MW38n5DJ)e9F??$47by;
zpW_6%iNOK^u~92%wFLV5PIetrV23i^IDi4<ba%=gQ{NJ|6_lT^r{*~n7{&pz1G?>o
z$VbC7U_;WALf6$n`=3l>Jgm+dg^m-k2^k+=6-ZJdx`H6>bi||dv&=m%z&P>VqaKSz
zMqC{6maun1Jdy)}yZ(!I$};ywt@h4YfRm2$#9S~}4}`@UPVn)n;pZ$uI}rgWpR;|X
zEf><ioEs~-Tm;tsr`|^wKaGAtZ_&Tu+$|~$Ey2XtZBLI+ca?cHHSp!Lw{N|zFmLZC
z@vF0yGxls@bPTA#Qc~d`I1Xm@9|50%Zz(pP#wE}XKC03pXEh&LidD52YmYc*kk}~g
zKLtwh*04AM(45<!VI6K!sarjD0hoYABdHI}yj})3m1(_Yverl$<Fsw1a{)b`Dr8RK
qm|_3`0000$H3dx@SJO%W0p$;XAOHa7IhfM1#Ao{g000001X)^qgznh@

literal 0
HcmV?d00001

-- 
2.23.0

Re: [PATCH 1/4] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees

[Nikolay Borisov]

On 3.09.19 г. 11:24 ч., Qu Wenruo wrote:
> Before this patch, repair_imode_common() can only handle two types of
> inodes:
> - Free space cache inodes
> - ROOT DIR inodes
> 
> For inodes in subvolume trees, the problem is how to determine the
> correct imode, thus it was not implemented.
> 
> However there are more reports of incorrect imode in subvolume trees, we
> need to support such fix.
> 
> So this patch adds a new function, detect_imode(), to detect (or call it
> educated guess) imode for inodes in subvolume trees.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 83 insertions(+), 13 deletions(-)
> 
> diff --git a/check/mode-common.c b/check/mode-common.c
> index 195b6efaa7aa..807d7daf98a6 100644
> --- a/check/mode-common.c
> +++ b/check/mode-common.c
> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  	return ret;
>  }
>  
> +static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
> +			u32 *imode_ret)
> +{
> +	struct btrfs_key key;
> +	struct btrfs_inode_item *iitem;
> +	const u32 priv = 0700;
> +	u64 ino;
> +	u32 imode = S_IFREG;
> +	int ret = 0;
> +
> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +	ino = key.objectid;
> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
> +			       struct btrfs_inode_item);
> +
> +	/*
> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
> +	 * to BLK.
> +	 */
> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
> +		imode = S_IFBLK;
> +		goto out;
> +	}
> +
> +	/* root inode */
> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
> +		imode = S_IFDIR;
> +		goto out;
> +	}
> +
> +	while (1) {
> +		ret = btrfs_next_item(root, path);
> +		if (ret > 0) {
> +			/* No determining result found, falls back to REG */
> +			ret = 0;
> +			goto out;
> +		}
> +		if (ret < 0)
> +			goto out;
> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +		if (key.objectid != ino)
> +			goto out;
> +
> +		/*
> +		 * We ignore some types to make life easier:
> +		 * - INODE_REF
> +		 *   We need to do a full tree search, which can fail for
> +		 *   corrupted fs, but not worthy compared to other easier
> +		 *   to determine types.
> +		 * - XATTR
> +		 *   Both REG and DIR can have xattr, so not useful
> +		 */
> +		switch (key.type) {
> +		case BTRFS_DIR_ITEM_KEY:
> +		case BTRFS_DIR_INDEX_KEY:
> +			imode = S_IFDIR;
> +			goto out;
> +		case BTRFS_EXTENT_DATA_KEY:
> +			/*
> +			 * Both REG and LINK could have EXTENT_DATA.
> +			 * We just fall back to REG as user can inspect the
> +			 * content.
> +			 */
> +			imode = S_IFREG;
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	/* Set default value even when something wrong happened */
> +	*imode_ret = (imode | priv);
> +	return ret;
> +}
> +
>  /*
>   * Reset the inode mode of the inode specified by @path.
>   *
> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>  	u32 imode;
>  	int ret;
>  
> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
> -		error(
> -		"repair inode mode outside of root tree is not supported yet");
> -		return -ENOTTY;
> -	}
>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
> -	    !is_fstree(key.objectid)) {
> -		error("unsupported ino %llu", key.objectid);
> -		return -ENOTTY;
> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
> +		/* In root tree we only have two possible imode */
> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
> +			imode = S_IFDIR | 0755;
> +		else
> +			imode = S_IFREG | 0600;
> +	} else {
> +		detect_imode(root, path, &imode);
> +		/* Ignore error returned, just use the default value returned */

Is this safe enough though? What if due to an error a directory is
corrected to be file? Let's not forget the context we are oprating here
- a broken fs so it's possible (if not likely) that any of the search
functions inside detect_imode could return negative error value. OTOH
I'd expect the transaction commit to fail if that's the case e.g. faulty
device.

>  	}
> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
> -		imode = 040755;
> -	else
> -		imode = 0100600;
>  
>  	trans = btrfs_start_transaction(root, 1);
>  	if (IS_ERR(trans)) {
> 

Re: [PATCH 1/4] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees

[Qu Wenruo]

On 2019/9/4 下午3:37, Nikolay Borisov wrote:
>
>
> On 3.09.19 г. 11:24 ч., Qu Wenruo wrote:
>> Before this patch, repair_imode_common() can only handle two types of
>> inodes:
>> - Free space cache inodes
>> - ROOT DIR inodes
>>
>> For inodes in subvolume trees, the problem is how to determine the
>> correct imode, thus it was not implemented.
>>
>> However there are more reports of incorrect imode in subvolume trees, we
>> need to support such fix.
>>
>> So this patch adds a new function, detect_imode(), to detect (or call it
>> educated guess) imode for inodes in subvolume trees.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/check/mode-common.c b/check/mode-common.c
>> index 195b6efaa7aa..807d7daf98a6 100644
>> --- a/check/mode-common.c
>> +++ b/check/mode-common.c
>> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>  	return ret;
>>  }
>>
>> +static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
>> +			u32 *imode_ret)
>> +{
>> +	struct btrfs_key key;
>> +	struct btrfs_inode_item *iitem;
>> +	const u32 priv = 0700;
>> +	u64 ino;
>> +	u32 imode = S_IFREG;
>> +	int ret = 0;
>> +
>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> +	ino = key.objectid;
>> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
>> +			       struct btrfs_inode_item);
>> +
>> +	/*
>> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>> +	 * to BLK.
>> +	 */
>> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
>> +		imode = S_IFBLK;
>> +		goto out;
>> +	}
>> +
>> +	/* root inode */
>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>> +		imode = S_IFDIR;
>> +		goto out;
>> +	}
>> +
>> +	while (1) {
>> +		ret = btrfs_next_item(root, path);
>> +		if (ret > 0) {
>> +			/* No determining result found, falls back to REG */
>> +			ret = 0;
>> +			goto out;
>> +		}
>> +		if (ret < 0)
>> +			goto out;
>> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> +		if (key.objectid != ino)
>> +			goto out;
>> +
>> +		/*
>> +		 * We ignore some types to make life easier:
>> +		 * - INODE_REF
>> +		 *   We need to do a full tree search, which can fail for
>> +		 *   corrupted fs, but not worthy compared to other easier
>> +		 *   to determine types.
>> +		 * - XATTR
>> +		 *   Both REG and DIR can have xattr, so not useful
>> +		 */
>> +		switch (key.type) {
>> +		case BTRFS_DIR_ITEM_KEY:
>> +		case BTRFS_DIR_INDEX_KEY:
>> +			imode = S_IFDIR;
>> +			goto out;
>> +		case BTRFS_EXTENT_DATA_KEY:
>> +			/*
>> +			 * Both REG and LINK could have EXTENT_DATA.
>> +			 * We just fall back to REG as user can inspect the
>> +			 * content.
>> +			 */
>> +			imode = S_IFREG;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	/* Set default value even when something wrong happened */
>> +	*imode_ret = (imode | priv);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * Reset the inode mode of the inode specified by @path.
>>   *
>> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>>  	u32 imode;
>>  	int ret;
>>
>> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
>> -		error(
>> -		"repair inode mode outside of root tree is not supported yet");
>> -		return -ENOTTY;
>> -	}
>>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
>> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
>> -	    !is_fstree(key.objectid)) {
>> -		error("unsupported ino %llu", key.objectid);
>> -		return -ENOTTY;
>> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
>> +		/* In root tree we only have two possible imode */
>> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
>> +			imode = S_IFDIR | 0755;
>> +		else
>> +			imode = S_IFREG | 0600;
>> +	} else {
>> +		detect_imode(root, path, &imode);
>> +		/* Ignore error returned, just use the default value returned */
>
> Is this safe enough though?

It depends. But I'd say if we failed in detect_imode(), then it doesn't
matter whatever the type we're putting here.

> What if due to an error a directory is
> corrected to be file?

If a failure happens, it means btrfs-progs fails to read the next leaf.
Then it really doesn't make much sense whatever the type is.

But I get your point, indeed we should error out without trying out to
fix the inode.

I'll change this behavior in next version.

Thanks,
Qu

> Let's not forget the context we are oprating here
> - a broken fs so it's possible (if not likely) that any of the search
> functions inside detect_imode could return negative error value. OTOH
> I'd expect the transaction commit to fail if that's the case e.g. faulty
> device.
>
>>  	}
>> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
>> -		imode = 040755;
>> -	else
>> -		imode = 0100600;
>>
>>  	trans = btrfs_start_transaction(root, 1);
>>  	if (IS_ERR(trans)) {
>>

Re: [PATCH 1/4] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees

[Nikolay Borisov]

On 4.09.19 г. 11:30 ч., Qu Wenruo wrote:
> 
> 
> On 2019/9/4 下午3:37, Nikolay Borisov wrote:
>>
>>
>> On 3.09.19 г. 11:24 ч., Qu Wenruo wrote:
>>> Before this patch, repair_imode_common() can only handle two types of
>>> inodes:
>>> - Free space cache inodes
>>> - ROOT DIR inodes
>>>
>>> For inodes in subvolume trees, the problem is how to determine the
>>> correct imode, thus it was not implemented.
>>>
>>> However there are more reports of incorrect imode in subvolume trees, we
>>> need to support such fix.
>>>
>>> So this patch adds a new function, detect_imode(), to detect (or call it
>>> educated guess) imode for inodes in subvolume trees.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/check/mode-common.c b/check/mode-common.c
>>> index 195b6efaa7aa..807d7daf98a6 100644
>>> --- a/check/mode-common.c
>>> +++ b/check/mode-common.c
>>> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>>  	return ret;
>>>  }
>>>
>>> +static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
>>> +			u32 *imode_ret)
>>> +{
>>> +	struct btrfs_key key;
>>> +	struct btrfs_inode_item *iitem;
>>> +	const u32 priv = 0700;
>>> +	u64 ino;
>>> +	u32 imode = S_IFREG;
>>> +	int ret = 0;
>>> +
>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +	ino = key.objectid;
>>> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
>>> +			       struct btrfs_inode_item);
>>> +
>>> +	/*
>>> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>>> +	 * to BLK.
>>> +	 */
>>> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
>>> +		imode = S_IFBLK;
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* root inode */
>>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>>> +		imode = S_IFDIR;
>>> +		goto out;
>>> +	}
>>> +
>>> +	while (1) {
>>> +		ret = btrfs_next_item(root, path);
>>> +		if (ret > 0) {
>>> +			/* No determining result found, falls back to REG */
>>> +			ret = 0;
>>> +			goto out;
>>> +		}
>>> +		if (ret < 0)
>>> +			goto out;
>>> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +		if (key.objectid != ino)
>>> +			goto out;
>>> +
>>> +		/*
>>> +		 * We ignore some types to make life easier:
>>> +		 * - INODE_REF
>>> +		 *   We need to do a full tree search, which can fail for
>>> +		 *   corrupted fs, but not worthy compared to other easier
>>> +		 *   to determine types.
>>> +		 * - XATTR
>>> +		 *   Both REG and DIR can have xattr, so not useful
>>> +		 */
>>> +		switch (key.type) {
>>> +		case BTRFS_DIR_ITEM_KEY:
>>> +		case BTRFS_DIR_INDEX_KEY:
>>> +			imode = S_IFDIR;
>>> +			goto out;
>>> +		case BTRFS_EXTENT_DATA_KEY:
>>> +			/*
>>> +			 * Both REG and LINK could have EXTENT_DATA.
>>> +			 * We just fall back to REG as user can inspect the
>>> +			 * content.
>>> +			 */
>>> +			imode = S_IFREG;
>>> +			goto out;
>>> +		}
>>> +	}
>>> +
>>> +out:
>>> +	/* Set default value even when something wrong happened */
>>> +	*imode_ret = (imode | priv);
>>> +	return ret;
>>> +}
>>> +
>>>  /*
>>>   * Reset the inode mode of the inode specified by @path.
>>>   *
>>> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>>>  	u32 imode;
>>>  	int ret;
>>>
>>> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
>>> -		error(
>>> -		"repair inode mode outside of root tree is not supported yet");
>>> -		return -ENOTTY;
>>> -	}
>>>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
>>> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
>>> -	    !is_fstree(key.objectid)) {
>>> -		error("unsupported ino %llu", key.objectid);
>>> -		return -ENOTTY;
>>> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
>>> +		/* In root tree we only have two possible imode */
>>> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
>>> +			imode = S_IFDIR | 0755;
>>> +		else
>>> +			imode = S_IFREG | 0600;
>>> +	} else {
>>> +		detect_imode(root, path, &imode);
>>> +		/* Ignore error returned, just use the default value returned */
>>
>> Is this safe enough though?
> 
> It depends. But I'd say if we failed in detect_imode(), then it doesn't
> matter whatever the type we're putting here.
> 
>> What if due to an error a directory is
>> corrected to be file?
> 
> If a failure happens, it means btrfs-progs fails to read the next leaf.
> Then it really doesn't make much sense whatever the type is.
> 
> But I get your point, indeed we should error out without trying out to
> fix the inode.
> 
> I'll change this behavior in next version.

IMO the goal of btrfs-progs should be to make a broken filesystem better
and not replace one type of breakage with potentially another. In such
cases we ought to break out gracefully.

> 
> Thanks,
> Qu
> 
>> Let's not forget the context we are oprating here
>> - a broken fs so it's possible (if not likely) that any of the search
>> functions inside detect_imode could return negative error value. OTOH
>> I'd expect the transaction commit to fail if that's the case e.g. faulty
>> device.
>>
>>>  	}
>>> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
>>> -		imode = 040755;
>>> -	else
>>> -		imode = 0100600;
>>>
>>>  	trans = btrfs_start_transaction(root, 1);
>>>  	if (IS_ERR(trans)) {
>>>
> 

Re: [PATCH 1/4] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees

[Qu Wenruo]

On 2019/9/4 下午6:53, Nikolay Borisov wrote:
>
>
> On 4.09.19 г. 11:30 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/9/4 下午3:37, Nikolay Borisov wrote:
>>>
>>>
>>> On 3.09.19 г. 11:24 ч., Qu Wenruo wrote:
>>>> Before this patch, repair_imode_common() can only handle two types of
>>>> inodes:
>>>> - Free space cache inodes
>>>> - ROOT DIR inodes
>>>>
>>>> For inodes in subvolume trees, the problem is how to determine the
>>>> correct imode, thus it was not implemented.
>>>>
>>>> However there are more reports of incorrect imode in subvolume trees, we
>>>> need to support such fix.
>>>>
>>>> So this patch adds a new function, detect_imode(), to detect (or call it
>>>> educated guess) imode for inodes in subvolume trees.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/check/mode-common.c b/check/mode-common.c
>>>> index 195b6efaa7aa..807d7daf98a6 100644
>>>> --- a/check/mode-common.c
>>>> +++ b/check/mode-common.c
>>>> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>>>  	return ret;
>>>>  }
>>>>
>>>> +static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
>>>> +			u32 *imode_ret)
>>>> +{
>>>> +	struct btrfs_key key;
>>>> +	struct btrfs_inode_item *iitem;
>>>> +	const u32 priv = 0700;
>>>> +	u64 ino;
>>>> +	u32 imode = S_IFREG;
>>>> +	int ret = 0;
>>>> +
>>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>> +	ino = key.objectid;
>>>> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
>>>> +			       struct btrfs_inode_item);
>>>> +
>>>> +	/*
>>>> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>>>> +	 * to BLK.
>>>> +	 */
>>>> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
>>>> +		imode = S_IFBLK;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	/* root inode */
>>>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>>>> +		imode = S_IFDIR;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	while (1) {
>>>> +		ret = btrfs_next_item(root, path);
>>>> +		if (ret > 0) {
>>>> +			/* No determining result found, falls back to REG */
>>>> +			ret = 0;
>>>> +			goto out;
>>>> +		}
>>>> +		if (ret < 0)
>>>> +			goto out;
>>>> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>> +		if (key.objectid != ino)
>>>> +			goto out;
>>>> +
>>>> +		/*
>>>> +		 * We ignore some types to make life easier:
>>>> +		 * - INODE_REF
>>>> +		 *   We need to do a full tree search, which can fail for
>>>> +		 *   corrupted fs, but not worthy compared to other easier
>>>> +		 *   to determine types.
>>>> +		 * - XATTR
>>>> +		 *   Both REG and DIR can have xattr, so not useful
>>>> +		 */
>>>> +		switch (key.type) {
>>>> +		case BTRFS_DIR_ITEM_KEY:
>>>> +		case BTRFS_DIR_INDEX_KEY:
>>>> +			imode = S_IFDIR;
>>>> +			goto out;
>>>> +		case BTRFS_EXTENT_DATA_KEY:
>>>> +			/*
>>>> +			 * Both REG and LINK could have EXTENT_DATA.
>>>> +			 * We just fall back to REG as user can inspect the
>>>> +			 * content.
>>>> +			 */
>>>> +			imode = S_IFREG;
>>>> +			goto out;
>>>> +		}
>>>> +	}
>>>> +
>>>> +out:
>>>> +	/* Set default value even when something wrong happened */
>>>> +	*imode_ret = (imode | priv);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  /*
>>>>   * Reset the inode mode of the inode specified by @path.
>>>>   *
>>>> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>>>>  	u32 imode;
>>>>  	int ret;
>>>>
>>>> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
>>>> -		error(
>>>> -		"repair inode mode outside of root tree is not supported yet");
>>>> -		return -ENOTTY;
>>>> -	}
>>>>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
>>>> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
>>>> -	    !is_fstree(key.objectid)) {
>>>> -		error("unsupported ino %llu", key.objectid);
>>>> -		return -ENOTTY;
>>>> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
>>>> +		/* In root tree we only have two possible imode */
>>>> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
>>>> +			imode = S_IFDIR | 0755;
>>>> +		else
>>>> +			imode = S_IFREG | 0600;
>>>> +	} else {
>>>> +		detect_imode(root, path, &imode);
>>>> +		/* Ignore error returned, just use the default value returned */
>>>
>>> Is this safe enough though?
>>
>> It depends. But I'd say if we failed in detect_imode(), then it doesn't
>> matter whatever the type we're putting here.
>>
>>> What if due to an error a directory is
>>> corrected to be file?
>>
>> If a failure happens, it means btrfs-progs fails to read the next leaf.
>> Then it really doesn't make much sense whatever the type is.
>>
>> But I get your point, indeed we should error out without trying out to
>> fix the inode.
>>
>> I'll change this behavior in next version.
>
> IMO the goal of btrfs-progs should be to make a broken filesystem better
> and not replace one type of breakage with potentially another. In such
> cases we ought to break out gracefully.

Totally agreed.

BTW, I'll also implement the INODE_REF lookup method to provide the most
accurate way to recover.

The planned recovery would be:
- INODE_REF lookup
  If everything goes fine, that's it and call it a day.
  If it failed, try the rest.
- INODE rdev lookup
  For this case, either BLK or CHR, but it doesn't really matter nor
  would cause problem.
- DIR_INDEX/ITEM lookup
- EXTENT_DATA lookup
  Both REG and LNK would be possible but falls back to REG won't cause
  further problem.

Thanks,
Qu
>
>>
>> Thanks,
>> Qu
>>
>>> Let's not forget the context we are oprating here
>>> - a broken fs so it's possible (if not likely) that any of the search
>>> functions inside detect_imode could return negative error value. OTOH
>>> I'd expect the transaction commit to fail if that's the case e.g. faulty
>>> device.
>>>
>>>>  	}
>>>> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
>>>> -		imode = 040755;
>>>> -	else
>>>> -		imode = 0100600;
>>>>
>>>>  	trans = btrfs_start_transaction(root, 1);
>>>>  	if (IS_ERR(trans)) {
>>>>
>>