* [PATCH v2 1/2] btrfs-progs: handle orphan directories properly
2022-02-22 22:24 [PATCH v2 0/2][RESEND] btrfs-progs: handle orphan directories properly Josef Bacik
@ 2022-02-22 22:24 ` Josef Bacik
2022-02-22 22:24 ` [PATCH v2 2/2] btrfs-progs: add a test to check orphaned directories Josef Bacik
2022-03-08 17:51 ` [PATCH v2 0/2][RESEND] btrfs-progs: handle orphan directories properly David Sterba
2 siblings, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2022-02-22 22:24 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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 abe208df..e9ac94cc 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 f354a29b..0cdf24cd 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -2727,6 +2727,8 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
imode_to_type(mode), key.objectid,
key.offset);
}
+ if (is_orphan && key.type == BTRFS_DIR_INDEX_KEY)
+ break;
ret = check_dir_item(root, &key, path, &size);
err |= ret;
break;
@@ -2769,7 +2771,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);
@@ -2785,7 +2787,7 @@ out:
gfs_info->nodesize);
}
- if (isize != size) {
+ if (isize != size && !is_orphan) {
if (repair)
ret = repair_dir_isize_lowmem(root, path,
inode_id, size);
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2 2/2] btrfs-progs: add a test to check orphaned directories
2022-02-22 22:24 [PATCH v2 0/2][RESEND] btrfs-progs: handle orphan directories properly Josef Bacik
2022-02-22 22:24 ` [PATCH v2 1/2] " Josef Bacik
@ 2022-02-22 22:24 ` Josef Bacik
2022-03-08 17:51 ` [PATCH v2 0/2][RESEND] btrfs-progs: handle orphan directories properly David Sterba
2 siblings, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2022-02-22 22:24 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 0/2][RESEND] btrfs-progs: handle orphan directories properly
2022-02-22 22:24 [PATCH v2 0/2][RESEND] btrfs-progs: handle orphan directories properly Josef Bacik
2022-02-22 22:24 ` [PATCH v2 1/2] " Josef Bacik
2022-02-22 22:24 ` [PATCH v2 2/2] btrfs-progs: add a test to check orphaned directories Josef Bacik
@ 2022-03-08 17:51 ` David Sterba
2 siblings, 0 replies; 4+ messages in thread
From: David Sterba @ 2022-03-08 17:51 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
On Tue, Feb 22, 2022 at 05:24:29PM -0500, Josef Bacik wrote:
> v1->v2:
> - use "is_orphan" not "has_orphan_item" in the mode-lowmem code. Somehow the
> compiler didn't warn me of this until I switched to a different branch.
>
> --- Original email ---
> 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
Added to devel, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread