On Wed, Mar 16, 2011 at 04:50:30PM -0400, Chris Mason wrote:

Excerpts from Josef Bacik's message of 2011-03-16 16:04:23 -0400:

quoted

Currently if we have corrupted items things will blow up in spectacular ways.
So as we read in blocks and they are leaves, check the entire leaf to make sure
all of the items are correct and point to valid parts in the leaf for the item
data the are responsible for.  If the item is corrupt we will kick back EIO and
not read any of the copies since they are likely to not be correct either.  This
will catch generic corruptions, it will be up to the individual callers of
btrfs_search_slot to make sure their items are right.  Thanks,

Thanks for working on this, a few comments below.

quoted

Signed-off-by: Josef Bacik <redacted>

quoted

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 495b1ac..1694782 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c

@@ -331,6 +331,14 @@ static int btree_read_extent_buffer_pages(struct btrfs_root *root,
             !verify_parent_transid(io_tree, eb, parent_transid))
             return ret;
 
+        /*
+         * This buffer's crc is fine, but its contents are corrupted, so
+         * there is no reason to read the other copies, they won't be
+         * any less wrong.
+         */
+        if (test_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags))
+            return ret;
+

We should make sure to clear this when read_extent_buffer_pages starts?
At the very least it should get cleared if we delete the block.

Ok I can fix that.

quoted

         num_copies = btrfs_num_copies(&root->fs_info->mapping_tree,
                           eb->start, eb->len);
         if (num_copies == 1)

@@ -419,6 +427,76 @@ static int check_tree_block_fsid(struct btrfs_root *root,
     return ret;
 }
 
+#define CORRUPT(reason, eb, root, slot)                \
+    printk(KERN_CRIT "btrfs: corrupt leaf, %s: block=%llu,"    \
+           "root=%llu, slot=%d\n", reason,            \
+           (unsigned long long)btrfs_header_bytenr(eb),    \
+           (unsigned long long)root->objectid, slot)
+
+/*
+ * extra checking to make sure all the items in a leaf are
+ * well formed and in the proper order
+ */
+static int check_leaf(struct btrfs_root *root,
+              struct extent_buffer *leaf, int slot)
+{
+    struct btrfs_key key;
+    struct btrfs_key leaf_key;
+    u32 nritems = btrfs_header_nritems(leaf);
+
+    btrfs_item_key_to_cpu(leaf, &leaf_key, slot);
+    if (slot != 0 && slot < nritems - 1) {
+        btrfs_item_key_to_cpu(leaf, &key, slot - 1);
+        if (btrfs_comp_cpu_keys(&leaf_key, &key) <= 0) {
+            CORRUPT("offset bad key", leaf, root, slot);
+            return -EIO;
+        }
+        if (btrfs_item_offset_nr(leaf, slot - 1) !=
+               btrfs_item_end_nr(leaf, slot)) {
+            CORRUPT("slot offset bad", leaf, root, slot);
+            return -EIO;
+        }

Ok, this checks the item before our slot.

quoted

+    }
+    if (slot < nritems - 1) {
+        btrfs_item_key_to_cpu(leaf, &key, slot + 1);
+        if (btrfs_comp_cpu_keys(&leaf_key, &key) >= 0) {
+            CORRUPT("offset bad key", leaf, root, slot);
+            return -EIO;
+        }
+        if (btrfs_item_offset_nr(leaf, slot) !=
+            btrfs_item_end_nr(leaf, slot + 1)) {
+            CORRUPT("slot offset bad", leaf, root, slot);
+            return -EIO;
+        }

And this checks the item after our slot

quoted

+    }
+    if (btrfs_item_offset_nr(leaf, 0) + btrfs_item_size_nr(leaf, 0) !=
+        BTRFS_LEAF_DATA_SIZE(root)) {
+        CORRUPT("invalid offset size pair", leaf, root, slot);
+        return -EIO;

And this checks item zero.  But we're not checking to make sure
the offsets of the item headers are inside the leaf.  In your code they
all have to be consistent, but they might all point into funny places
(consistently).  I'm not sure if that is possible to do and have item
zero check out, but it seems like we could add one check to make sure
the offset is inside the block itself.

quoted

+    }
+
+    return 0;
+}
+
+static noinline int check_block(struct btrfs_root *root,
+                struct extent_buffer *eb)
+{
+    u32 nritems = btrfs_header_nritems(eb);
+    int slot;
+    int ret = 0;
+
+    if (nritems == 0)
+        return 0;
+
+    for (slot = 0; slot < nritems; slot++) {
+        ret = check_leaf(root, eb, slot);
+        if (ret)
+            break;
+    }

I might be missing something, but this looks like:

	for each item in the leaf {
		check the item before
		check the item after
		check item 0
	}

Why not:
	check item 0
	for each item in the leaf {
		check the item after
	}
	check the last item

Right that sounds good.  I'll fix this up tomorrow and resend.  Thanks,

Josef