Re: [RFC - PATCH] btrfs: do not write corrupted metadata blocks to disk
From: Filipe Manana <fdmanana@kernel.org>
Date: 2016-02-22 01:05:35
On Sun, Feb 21, 2016 at 3:36 PM, Alex Lyakas [off-list ref] wrote:
csum_dirty_buffer was issuing a warning in case the extent buffer did not look alright, but was still returning success. Let's return error in this case, and also add two additional sanity checks on the extent buffer header. We had btrfs metadata corruption, and after looking at the logs we saw that WARN_ON(found_start != start) has been triggered. We are still investigating
There's a warning for WARN_ON(found_start != start || !PageUptodate(page)) Are you sure it triggered only because of found_start != start and not because of !PageUptodate(page) (or both)?
which component trashed the cache page which belonged to btrfs. But btrfs only issued a warning, and as a result, the corrupted metadata block went to disk. I think we should return an error in such case that the extent buffer doesn't look alright.
I think so too.
quoted hunk ↗ jump to hunk
The caller up the chain may BUG_ON on this, for example flush_epd_write_bio will, but it is better than to have a silent metadata corruption on disk. Note: this patch has been properly tested on 3.18 kernel only. Signed-off-by: Alex Lyakas <redacted> --- fs/btrfs/disk-io.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 4545e2e..701e706 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c@@ -508,22 +508,32 @@ static int csum_dirty_buffer(struct btrfs_fs_info*fs_info, struct page *page) { u64 start = page_offset(page); u64 found_start; struct extent_buffer *eb; eb = (struct extent_buffer *)page->private; if (page != eb->pages[0]) return 0; found_start = btrfs_header_bytenr(eb); if (WARN_ON(found_start != start || !PageUptodate(page))) - return 0; - csum_tree_block(fs_info, eb, 0); + return -EUCLEAN; +#ifdef CONFIG_BTRFS_ASSERT
A bit odd to surround these with CONFIG_BTRFS_ASSERT if we don't do assertions. I would remove this #ifdef ... #endif or do the memcmp calls inside ASSERT().
+ if (WARN_ON(memcmp_extent_buffer(eb, fs_info->fsid, + (unsigned long)btrfs_header_fsid(), BTRFS_FSID_SIZE))) + return -EUCLEAN; + if (WARN_ON(memcmp_extent_buffer(eb, fs_info->fsid, + (unsigned long)btrfs_header_chunk_tree_uuid(eb), + BTRFS_FSID_SIZE)))
This second comparison doesn't seem correct. Second argument to memcmp_extent_buffer should be fs_info->chunk_tree_uuid, which shouldn't be the same as the fsid (take a look at utils.c:make_btrfs() in the tools, both uuids are generated by different calls to uuid_generate()) - did you make your tests only before adding this comparison?. Also you should use BTRFS_UUID_SIZE instead of BTRFS_FSID_SIZE (even if both have the same value).
+ return -EUCLEAN; +#endif + if (csum_tree_block(fs_info, eb, 0)) + return -EUCLEAN;
I would just return the real error from csum_tree_block() - currently it returns 1 for all possible failures instead of its real possible failures: -ENOMEM or -EINVAL. Thanks.
return 0;
}
static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,
struct extent_buffer *eb)
{
struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
u8 fsid[BTRFS_UUID_SIZE];
int ret = 1;
--
1.9.1