Re: [PATCH v4 08/68] btrfs: inode: sink parameter @start and @len for check_data_csum()

[David Sterba <dsterba@suse.cz>]

On Tue, Oct 27, 2020 at 08:50:15AM +0800, Qu Wenruo wrote:
> On 2020/10/27 上午8:13, David Sterba wrote:
> > On Wed, Oct 21, 2020 at 02:24:54PM +0800, Qu Wenruo wrote:
> >> For check_data_csum(), the page we're using is directly from inode
> >> mapping, thus it has valid page_offset().
> >>
> >> We can use (page_offset() + pg_off) to replace @start parameter
> >> completely, while the @len should always be sectorsize.
> >>
> >> Since we're here, also add some comment, as there are quite some
> >> confusion in words like start/offset, without explaining whether it's
> >> file_offset or logical bytenr.
> >>
> >> This should not affect the existing behavior, as for current sectorsize
> >> == PAGE_SIZE case, @pgoff should always be 0, and len is always
> >> PAGE_SIZE (or sectorsize from the dio read path).
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  fs/btrfs/inode.c | 27 +++++++++++++++++++--------
> >>  1 file changed, 19 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> index 2a56d3b8eff4..24fbf2c46e56 100644
> >> --- a/fs/btrfs/inode.c
> >> +++ b/fs/btrfs/inode.c
> >> @@ -2791,17 +2791,30 @@ void btrfs_writepage_endio_finish_ordered(struct page *page, u64 start,
> >>  	btrfs_queue_work(wq, &ordered_extent->work);
> >>  }
> >>  
> >> +/*
> >> + * Verify the checksum of one sector of uncompressed data.
> >> + *
> >> + * @inode:	The inode.
> >> + * @io_bio:	The btrfs_io_bio which contains the csum.
> >> + * @icsum:	The csum offset (by number of sectors).
> > 
> > This is not true, it's the index to the checksum array, where size of
> > the element is fs_info::csum_size. The offset can be calculated but it's
> > not the thing that's passed as argument.

> Isn't the offset by sectors the same?

Offset by sectors reads as something expressed in sector-sized units.
> 
> If it's 1, it means we need to skip 1 csum which is in csum_size.

Yes, so you see the difference sector vs csum_size. I understand what
you meant by that but reading the comment without going to the code can
confuse somebody.