On Tue, Feb 19, 2019 at 01:43:53PM +0100, Johannes Thumshirn wrote: > Currently csum_tree_block() does two things, first it as it's name > suggests it calculates the checksum for a tree-block. But it also writes > this checksum to disk or reads an extent_buffer from disk and compares the > checksum with the calculated checksum, depending on the verify argument. > > Furthermore one of the two callers passes in '1' for the verify argument, > the other one passes in '0'. > > For clarity and less layering violations, factor out the second stage in > csum_tree_block()'s callers. > > Suggested-by: Nikolay Borisov <[email protected]> > Reviewed-by: Qu Wenruo <[email protected]> > Reviewed-by: Nikolay Borisov <[email protected]> > Signed-off-by: Johannes Thumshirn <[email protected]> > > --- > > Changes to v2: > - Directly return -EINVAL instead of EUCLEAN > > Changes to v1: > - return error from csum_tree_buffer() in csum_dirty_buffer() instead of > EUCLEAN (Nikolay) > --- > fs/btrfs/disk-io.c | 51 +++++++++++++++++++++++++++------------------------ > 1 file changed, 27 insertions(+), 24 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 5216e7b3f9ad..77089283be51 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -552,7 +534,11 @@ static int csum_dirty_buffer(struct btrfs_fs_info > *fs_info, struct page *page) > ASSERT(memcmp_extent_buffer(eb, fs_info->fs_devices->metadata_uuid, > btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0); > > - return csum_tree_block(fs_info, eb, 0); > + if (WARN_ON(csum_tree_block(eb, result)))
I think the warn should go to csum_tree_block when the mapping function returns 1, there's even a comment explaining why it can't normally happen. The reason for the warning in csum_dirty_buffer is not very clear from the context. Otherwise ok, the verify or write semantics of a function that (by name) only calculates checksum is confusing. Thanks.
