On Sun, Feb 21, 2016 at 3:36 PM, Alex Lyakas <alex@xxxxxxxxxxxxxxxxx> 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.
> 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 <alex@xxxxxxxxxxxxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
