[PATCH 0/2] btrfs-progs: handle orphan directories properly

[PATCH 0/2] btrfs-progs: handle orphan directories properly

[Josef Bacik]

Hello,

While implementing the garbage collection tree I started getting btrfsck errors
when a test would do rm -rf DIRECTORY and then immediately unmount.  This is
because we stop processing GC items during umount, and thus we left the
directory with an nlink == 0 and all of its children in the fs tree.

However this isn't a problem with just the GC tree, we can have this happen if
we fail to do the eviction work at evict time, and we leave the orphan entry in
place on disk.  btrfsck properly ignores problems with inodes that have orphan
items, except for directory inodes.

Fix this by making sure we don't add any backrefs we find from directory inodes
that we've find the inode item for and have an nlink == 0.

I generated a test image for this by simply skipping the
btrfs_truncate_inode_items() work in evict in a kernel and rm -rf'ing a
directory on a test file system.

With this patch both make test-check and make test-check-lowmem pass all the
tests, including the new testcase.  Thanks,

Josef

Josef Bacik (2):
  btrfs-progs: handle orphan directories properly
  btrfs-progs: add a test to check orphaned directories

 check/main.c                                  |  15 ++++++++++++-
 check/mode-lowmem.c                           |   6 ++++--
 .../052-orphan-directory/default.img.xz       | Bin 0 -> 1432 bytes
 tests/fsck-tests/052-orphan-directory/test.sh |  20 ++++++++++++++++++
 4 files changed, 38 insertions(+), 3 deletions(-)
 create mode 100644 tests/fsck-tests/052-orphan-directory/default.img.xz
 create mode 100755 tests/fsck-tests/052-orphan-directory/test.sh

-- 
2.26.3

[PATCH 1/2] btrfs-progs: handle orphan directories properly

[Josef Bacik]

When implementing the GC tree I started getting btrfsck errors when a
test rm -rf <directory> with files inside of it and immediately unmount,
leaving behind orphaned directory items that have GC items for them.

This made me realize that we don't actually handle this case currently
for our normal orphan path.  If we fail to clean everything up and leave
behind the orphan items we'll fail fsck.

Fix this by not processing any backrefs we find if we found an inode
item and its nlink is 0.  This allows us to pass the test case I've
provided to validate this patch.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/main.c        | 15 ++++++++++++++-
 check/mode-lowmem.c |  6 ++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/check/main.c b/check/main.c
index e67d2f72..966a68ab 100644
--- a/check/main.c
+++ b/check/main.c
@@ -1018,6 +1018,16 @@ static int merge_inode_recs(struct inode_record *src, struct inode_record *dst,
 	int ret = 0;
 
 	dst->merging = 1;
+
+	/*
+	 * If we wandered into a shared node while we were processing an inode
+	 * we may have added backrefs for a directory that had nlink == 0, so
+	 * skip adding these backrefs to our src inode if we have nlink == 0 and
+	 * we actually found the inode item.
+	 */
+	if (src->found_inode_item && src->nlink == 0)
+		goto skip_backrefs;
+
 	list_for_each_entry(backref, &src->backrefs, list) {
 		if (backref->found_dir_index) {
 			add_inode_backref(dst_cache, dst->ino, backref->dir,
@@ -1039,7 +1049,7 @@ static int merge_inode_recs(struct inode_record *src, struct inode_record *dst,
 					backref->ref_type, backref->errors);
 		}
 	}
-
+skip_backrefs:
 	if (src->found_dir_item)
 		dst->found_dir_item = 1;
 	if (src->found_file_extent)
@@ -1394,6 +1404,9 @@ static int process_dir_item(struct extent_buffer *eb,
 	rec = active_node->current;
 	rec->found_dir_item = 1;
 
+	if (rec->found_inode_item && rec->nlink == 0)
+		return 0;
+
 	di = btrfs_item_ptr(eb, slot, struct btrfs_dir_item);
 	total = btrfs_item_size_nr(eb, slot);
 	while (cur < total) {
diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 696ad215..66e291da 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -2726,6 +2726,8 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
 					imode_to_type(mode), key.objectid,
 					key.offset);
 			}
+			if (has_orphan_item && key.type == BTRFS_DIR_INDEX_KEY)
+				break;
 			ret = check_dir_item(root, &key, path, &size);
 			err |= ret;
 			break;
@@ -2768,7 +2770,7 @@ out:
 				&nlink);
 		}
 
-		if (nlink != 1) {
+		if (nlink > 1) {
 			err |= LINK_COUNT_ERROR;
 			error("root %llu DIR INODE[%llu] shouldn't have more than one link(%llu)",
 			      root->objectid, inode_id, nlink);
@@ -2784,7 +2786,7 @@ out:
 				gfs_info->nodesize);
 		}
 
-		if (isize != size) {
+		if (isize != size && !has_orphan_item) {
 			if (repair)
 				ret = repair_dir_isize_lowmem(root, path,
 							      inode_id, size);
-- 
2.26.3

[PATCH 2/2] btrfs-progs: add a test to check orphaned directories

[Josef Bacik]

When adding the GC support I noticed we were failing fsck when we had a
directory that hadn't been cleaned up yet because of rm -rf.  However
this isn't limited to extent-tree-v2, we'll actually fail in the same
way if we were unable to do the evict portion of the deletion and left
the orphan items around for everybody.

This is a valid file system, it'll be cleaned up properly at mount time,
so fsck shouldn't fail in this case.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 .../052-orphan-directory/default.img.xz       | Bin 0 -> 1432 bytes
 tests/fsck-tests/052-orphan-directory/test.sh |  20 ++++++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 tests/fsck-tests/052-orphan-directory/default.img.xz
 create mode 100755 tests/fsck-tests/052-orphan-directory/test.sh

diff --git a/tests/fsck-tests/052-orphan-directory/default.img.xz b/tests/fsck-tests/052-orphan-directory/default.img.xz
new file mode 100644
index 0000000000000000000000000000000000000000..4fac926c579034d29d51065f07ede7f1528f67f7
GIT binary patch
literal 1432
zcmV;J1!wyGH+ooF000E$*0e?f03iVu0001VFXf}+Q~w23T>wRyj;C3^v%$$4d1rE0
zjjaF1$3^@a-*CI>b`;<uyv9+%|Aw#cpU>Xhem%7aGMP*V_w!KDE5R0R%IUtZRb^s9
zi6n6B>Al(Z6(ck4q8BzTqu}wwoMuwumm_b;EKiEFys%e}Wy|!(G_fi_h+~MXJ*v!*
z@OLKOM3;uD5@h-)C*pAwm1MKZL8@)8Fx4fxHjW<>)`e6g5<m<CtSr!<2w8HLl<*5K
z`n!f&JLv?a$_ZLLC=nG|XcRibVhw7*BHnm6`G`3<_t%9}#MXW0@CnJOD&?lOm#^L%
zfd|(Y1b9?io(ZvCK~9-3uj_)}z(B?hL*f|EK-U>Zm!NQG2}Cra-*62)SQH>=$5N;D
zLM%3pZc}+`?rkRWv6>4}Fn$cATpC3hqVBR=9q-6#R0-)Cv(^T!T^t|EJMyV#*Lx0$
zf$^BY!nwl}m6w4}rp>Bh;*?$D`Z0yB_~Xov?rO4k_j5?t8Q&*8i?W?!?mAmKZCuI`
z%gJCR%)(WR?_bDW+`#$%Tt|AU_z6xe7R7ZS%%9Os&Lc?G$dNiKqoDnk;MffP4}V9N
zd=6Jgv3s~w8vCXvoJMalb-txXc$zKKONI6HCiC3{T0XyE?KUar>YVQnbBIDW@3mph
z+<M7xcXHT#Wxa*M5jUTC5Lyq&Q+6qm6YDn1q4vRi{nCRSg^H!Os==VMPLho9VK%?}
z>fJaSb-Bs2XRuNDn54yg<6>;z(;dbEEMA#YW?jQo?<7Z9tLF4#29`O%V)eC=fe{+@
zOTcDGQK~um*|~2NzP9juZ=P-Z4hvf^4+S(2(<DAl1GAsc%RK4vALtRb>_}HP90h+E
zUAt|w*rN(OiS2g_A_{WB!x!EI!)NN7AKz>y-4*7gusX@GuK)7rA^6nsyYhfmS=c2S
zmTvHr3dg>OxD-C-0OH&hil<fc?K;JY#nA!9C_L0-$X*x?u`{tTn-?>er`!mpohSrQ
zZ4_AJ-GO*{fJne0Aqn|}wma;<$xff>CSI8OA`{QC-tZAdFMafuQTWkEbU5qcPg@;n
zB=?3LtMx2@lD<V+5+P~PC1ghtoWlK(l7oTU2YppS=Wvg{jLk7HT?se15a=j>@fV?V
zp2$zK&{(*L`0ya5+$0hk<n}6W^A+7<uRFK@(EvI&?vM33llDliqM}SC-!f+<y(9XH
zQ?Y9p>=O~H)T}&J2bgEzK%`E=vRtkm2b_ZqBl9Hy3WWHN7ly#aWWD)V9wEY+vq6x*
zK{I25VZCb|ex+OQdZj+Oe8nQ22S6r}qgqZH@Q^pivE*;Tyug)0=#sr;J=xb6AXkNK
z4M-}MG^z0~YX(ExEW{9ZY`Vv%FW;{>7ZY?YmLY9Vb*ly4+t@btt*W$tHKqz2P(!(@
zLjby7n;<bZ#~!~oa1}}0?nft;9^JP*WCLMU*6=!JDY=jE9;>YV$$h}qS-=*#5>AjJ
zHZ+k~u~BE-A8o`$m>Aska*;l6_n{70@b{)EFyyUbN<*QPMB;FW4_|{Y)Nw+J5j?R+
z=H3KR<bu&~s$@8jsX+NbKQe?E*3=SSZhBxO0DfM`RHvAU6zDb{nXnXM(kDqj6N@4D
zxh71uWzgh4K>TkCRE@D>im$ZO;&`*U@4M@L<lt@yR5V!zyoLH_bBHAn(+W+V2sr?x
z(}Dpve0l#%gR&z-hL2rOrX*ocdrACf5D@FSf@>o)0mP1T447cNe(|C=OY>X-?7%+G
zrr;G*7ws0{$62R6#9X<aow&SY^pSs#_FB?{^0uIMe}IfUsqE8xK{1%d&7A`KH8>mq
m0001|VXy=UD-dA-0r3ies0jdS2iv@{#Ao{g000001X)_$1G{+u

literal 0
HcmV?d00001

diff --git a/tests/fsck-tests/052-orphan-directory/test.sh b/tests/fsck-tests/052-orphan-directory/test.sh
new file mode 100755
index 00000000..f3d380d9
--- /dev/null
+++ b/tests/fsck-tests/052-orphan-directory/test.sh
@@ -0,0 +1,20 @@
+#!/bin/bash
+# We could potentially have a directory and it's children with ORPHAN items left
+# for them without having been cleaned up.
+#
+# fsck shouldn't complain about this or attempt to do anything about it, the
+# orphan cleanup will do the correct thing.
+#
+# To create this image I simply modified the kernel to skip doing the
+# btrfs_truncate_inode_items() and removing the orphan item at evict time, and
+# then rm -rf'ed a directory.
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+
+check_image() {
+	run_check "$TOP/btrfs" check "$1"
+}
+
+check_all_images
-- 
2.26.3