btrfs-progs: mode-lowmem: Fix false alert about orphan inode

PATCH In review Qu Wenruo 3 participants 7 messages Feb 5, 2018 – Mar 12, 2018 Btrfs file system

Share & tools

Qu Wenruo

author

2018-02-05 06:47 UTC #0 lore raw

PATCH 1/4 btrfs-progs: mode-lowmem: Fix false alert about orphan inode

Btrfs can delay inode deletion and in that case btrfs will unlink the victim inode from its parent dir, and insert a marker to info btrfs to delete it later.

In that case, such victim inode will have nlinks == 0, but is still completely valid.
Original mode won't report such problem, but lowmem mode doesn't check the ORPHAN_ITEM key for such inode, and can report false alert like:
------
ERROR: root 257 INODE[28891726] is orphan item
------

Fix such false alert by checking orphan item for inode whose nlink is 0.

Signed-off-by Qu Wenruo <[email protected]>

---
 check/mode-lowmem.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

check/mode-lowmem.c +22 -2

index 62bcf3d2e126..b11a6d77d102 100644
@@ -1861,6 +1861,24 @@ out:
 	return ret;
 }
 
+static bool has_orphan_item(struct btrfs_root *root, u64 ino)
+{
+	struct btrfs_path path;
+	struct btrfs_key key;
+	int ret;
+
+	btrfs_init_path(&path);
+	key.objectid = BTRFS_ORPHAN_OBJECTID;
+	key.type = BTRFS_ORPHAN_ITEM_KEY;
+	key.offset = ino;
+
+	ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
+	btrfs_release_path(&path);
+	if (ret == 0)
+		return true;
+	return false;
+}
+
 /*
  * Check INODE_ITEM and related ITEMs (the same inode number)
  * 1. check link count

@@ -1890,6 +1908,7 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path,
 	u64 extent_size = 0;
 	unsigned int dir;
 	unsigned int nodatasum;
+	bool is_orphan = false;
 	int slot;
 	int ret;
 	int err = 0;

@@ -2040,10 +2059,11 @@ out:
 				      root->objectid, inode_id, nlink, refs);
 			}
 		} else if (!nlink) {
-			if (repair)
+			is_orphan = has_orphan_item(root, inode_id);
+			if (!is_orphan && repair)
 				ret = repair_inode_orphan_item_lowmem(root,
 							      path, inode_id);
-			if (!repair || ret) {
+			if (!is_orphan && (!repair || ret)) {
 				err |= ORPHAN_ITEM;
 				error("root %llu INODE[%llu] is orphan item",
 				      root->objectid, inode_id);

Signature · 1 line

2.16.1

Qu Wenruo

author

2018-02-05 06:47 UTC #1 lore raw

PATCH 2/4 btrfs-progs: fsck-tests: Add test case with valid orphan inode

Regression test for false alerts in lowmem mode.

Signed-off-by Qu Wenruo <[email protected]>

---
 .../029-valid-orphan-item/orphan_inode.img.xz         | Bin 0 -> 1620 bytes
 tests/fsck-tests/029-valid-orphan-item/test.sh        |  18 ++++++++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 tests/fsck-tests/029-valid-orphan-item/orphan_inode.img.xz
 create mode 100755 tests/fsck-tests/029-valid-orphan-item/test.sh

tests/fsck-tests/029-valid-orphan-item/orphan_inode.img.xz new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..26e4cf8a370f64fef0701fbbcb6d4d2a824f9e4b
GIT binary patch

tests/fsck-tests/029-valid-orphan-item/test.sh new file +18 -0

new file mode 100755
index 000000000000..997eb70e671c
@@ -0,0 +1,18 @@
+#!/bin/bash
+# To check if btrfs check can handle valid orphan items without problem
+# Orphan item is a marker used to delete inode/root.
+# Orphan inode/root will has no referencer to it, and will have an orphan item,
+# which should not be reported as error.
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+
+check_image() {
+	local image
+
+	image=$1
+	run_check "$TOP/btrfs" check "$image"
+}
+
+check_all_images

Signature · 1 line

2.16.1

Qu Wenruo

author

2018-02-05 06:47 UTC #2 lore raw

PATCH 3/4 btrfs-progs: gitignore: Ignore *.restored test image

In reply to Qu Wenruo

Signed-off-by Qu Wenruo <[email protected]>

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

.gitignore +1 -0

index 8e607f6e3ce2..7f1e1950cb77 100644
@@ -48,6 +48,7 @@ library-test-static
 /tests/test-console.txt
 /tests/test.img
 /tests/mnt/
+*.restored
 
 aclocal.m4
 autom4te.cache

Signature · 1 line

2.16.1

Qu Wenruo

author

2018-02-05 06:47 UTC #3 lore raw

PATCH 4/4 btrfs-progs: gitignore: Ignore patches

In reply to Qu Wenruo

Although it's not a good practice to format all patches under project directory, sometimes lazy bones like me just like to put patches under project directory.

Just ignore such patches.

Signed-off-by Qu Wenruo <[email protected]>

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

.gitignore +1 -0

index 7f1e1950cb77..fa2f9c40b7a9 100644
@@ -79,3 +79,4 @@ stamp-h.in
 stamp-h1
 config/*
 
+*.patch

Signature · 1 line

2.16.1

Su Yue

Reviewed-by on v1 1/4

2018-02-05 06:55 UTC #4 lore raw

In reply to Qu Wenruo

Quoted 35 lines · Qu Wenruo

Btrfs can delay inode deletion and in that case btrfs will unlink the victim inode from its parent dir, and insert a marker to info btrfs to delete it later.
In that case, such victim inode will have nlinks == 0, but is still completely valid.
Original mode won't report such problem, but lowmem mode doesn't check the ORPHAN_ITEM key for such inode, and can report false alert like:
------
ERROR: root 257 INODE[28891726] is orphan item
------
Fix such false alert by checking orphan item for inode whose nlink is 0.
Signed-off-by Qu Wenruo <[email protected]>
---
  check/mode-lowmem.c | 24 ++++++++++++++++++++++--
  1 file changed, 22 insertions(+), 2 deletions(-)
check/mode-lowmem.c +8 -0
index 62bcf3d2e126..b11a6d77d102 100644
@@ -1861,6 +1861,24 @@ out:
  	return ret;
  }
 
+static bool has_orphan_item(struct btrfs_root *root, u64 ino)
+{
+	struct btrfs_path path;
+	struct btrfs_key key;
+	int ret;
+
+	btrfs_init_path(&path);
+	key.objectid = BTRFS_ORPHAN_OBJECTID;

check/mode-lowmem.c out: · line 1881

Quoted 10 lines

check/mode-lowmem.c +10 -0

+	key.type = BTRFS_ORPHAN_ITEM_KEY;
+	key.offset = ino;
+
+	ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
+	btrfs_release_path(&path);
+	if (ret == 0)
+		return true;
+	return false;
+}
+

Suggestion:
Maybe "is_orphan_item" is a better name?
Anyway,

Reviewed-by Su Yue <[email protected]>

Quoted 26 lines

check/mode-lowmem.c +4 -2

  /*
   * Check INODE_ITEM and related ITEMs (the same inode number)
   * 1. check link count

@@ -1890,6 +1908,7 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path,
  	u64 extent_size = 0;
  	unsigned int dir;
  	unsigned int nodatasum;
+	bool is_orphan = false;
  	int slot;
  	int ret;
  	int err = 0;

@@ -2040,10 +2059,11 @@ out:
  				      root->objectid, inode_id, nlink, refs);
  			}
  		} else if (!nlink) {
-			if (repair)
+			is_orphan = has_orphan_item(root, inode_id);
+			if (!is_orphan && repair)
  				ret = repair_inode_orphan_item_lowmem(root,
  							      path, inode_id);
-			if (!repair || ret) {
+			if (!is_orphan && (!repair || ret)) {
  				err |= ORPHAN_ITEM;
  				error("root %llu INODE[%llu] is orphan item",
  				      root->objectid, inode_id);

David Sterba

2018-03-12 18:12 UTC #5 lore raw

Quoted 43 lines · Su Yue

Quoted 40 lines · Qu Wenruo
Btrfs can delay inode deletion and in that case btrfs will unlink the victim inode from its parent dir, and insert a marker to info btrfs to delete it later.
In that case, such victim inode will have nlinks == 0, but is still completely valid.
Original mode won't report such problem, but lowmem mode doesn't check the ORPHAN_ITEM key for such inode, and can report false alert like:
------
ERROR: root 257 INODE[28891726] is orphan item
------
Fix such false alert by checking orphan item for inode whose nlink is 0.
Signed-off-by Qu Wenruo <[email protected]>
---
  check/mode-lowmem.c | 24 ++++++++++++++++++++++--
  1 file changed, 22 insertions(+), 2 deletions(-)
check/mode-lowmem.c +13 -0
index 62bcf3d2e126..b11a6d77d102 100644
@@ -1861,6 +1861,24 @@ out:
  	return ret;
  }
 
+static bool has_orphan_item(struct btrfs_root *root, u64 ino)
+{
+	struct btrfs_path path;
+	struct btrfs_key key;
+	int ret;
+
+	btrfs_init_path(&path);
+	key.objectid = BTRFS_ORPHAN_OBJECTID;
+	key.type = BTRFS_ORPHAN_ITEM_KEY;
+	key.offset = ino;
+
+	ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
+	btrfs_release_path(&path);

Quoted 8 lines

check/mode-lowmem.c out: · line 1881
Quoted 5 lines
check/mode-lowmem.c +5 -0
+	if (ret == 0)
+		return true;
+	return false;
+}
+
Suggestion:
Maybe "is_orphan_item" is a better name?

The inode cannot be an orphan item itself, the orphan status is denoted by the orphan item so the inode does have the item attached. So has_orphan_item seems appropriate.

Quoted 1 line

Reviewed-by Su Yue <[email protected]>

Thanks.

David Sterba

2018-03-12 18:21 UTC #6 lore raw

In reply to Qu Wenruo

Quoted 12 lines · Qu Wenruo

Btrfs can delay inode deletion and in that case btrfs will unlink the victim inode from its parent dir, and insert a marker to info btrfs to delete it later.
In that case, such victim inode will have nlinks == 0, but is still completely valid.
Original mode won't report such problem, but lowmem mode doesn't check the ORPHAN_ITEM key for such inode, and can report false alert like:
------
ERROR: root 257 INODE[28891726] is orphan item
------

Quoted 3 lines

Fix such false alert by checking orphan item for inode whose nlink is 0.
Signed-off-by Qu Wenruo <[email protected]>

1-4 applied, thanks.