On Sun, Feb 21, 2016 at 3:36 PM, Alex Lyakas <[email protected]> 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 <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
