Re: [PATCH 14/17] btrfs: make btrfs_readpage_end_io_hook() follow sector size

Goldwyn Rodrigues
2020-09-10T19:52:25Z

[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]

On 8:05 10/09, Qu Wenruo wrote:
>
>
> On 2020/9/10 上午1:34, Goldwyn Rodrigues wrote:
> > On 15:52 08/09, Qu Wenruo wrote:
> >> Currently btrfs_readpage_end_io_hook() just pass the whole page to
> >> check_data_csum(), which is fine since we only support sectorsize ==
> >> PAGE_SIZE.
> >>
> >> To support subpage RO support, we need to properly honor per-sector
> >> checksum verification, just like what we did in dio read path.
> >>
> >> This patch will do the csum verification in a for loop, starts with
> >> pg_off == start - page_offset(page), with sectorsize increasement for
> >> each loop.
> >>
> >> For sectorsize == PAGE_SIZE case, the pg_off will always be 0, and we
> >> will only finish with just one loop.
> >>
> >> For subpage, we do the proper loop.
> >>
> >> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >> fs/btrfs/inode.c | 15 ++++++++++++++-
> >> 1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> index 078735aa0f68..8bd14dda2067 100644
> >> --- a/fs/btrfs/inode.c
> >> +++ b/fs/btrfs/inode.c
> >> @@ -2851,9 +2851,12 @@ static int btrfs_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >> u64 start, u64 end, int mirror)
> >> {
> >> size_t offset = start - page_offset(page);
> >> + size_t pg_off;
> >> struct inode *inode = page->mapping->host;
> >> struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> >> struct btrfs_root *root = BTRFS_I(inode)->root;
> >> + u32 sectorsize = root->fs_info->sectorsize;
> >> + bool found_err = false;
> >>
> >> if (PageChecked(page)) {
> >> ClearPageChecked(page);
> >> @@ -2870,7 +2873,17 @@ static int btrfs_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >> }
> >>
> >> phy_offset >>= inode->i_sb->s_blocksize_bits;
> >> - return check_data_csum(inode, io_bio, phy_offset, page, offset);
> >> + for (pg_off = offset; pg_off < end - page_offset(page);
> >> + pg_off += sectorsize, phy_offset++) {
> >> + int ret;
> >> +
> >> + ret = check_data_csum(inode, io_bio, phy_offset, page, pg_off);
> >> + if (ret < 0)
> >> + found_err = true;
> >> + }
> >> + if (found_err)
> >> + return -EIO;
> >> + return 0;
> >> }
> >
> > We don't need found_err here. Just return ret when you encounter the
> > first error.
> >
> But that means, the whole range will be marked error, while some sectors
> may still contain good data and pass the csum checking.
>

It would have made sense if you are storing block-by-block value of the
validity of the page so it may be referenced later. The function is only
checking if the data read in the page is correct or not, whether a part
of the data is correct is of no consequence to the caller. The earlier it
returns on an error, the better.. rather than checking the whole range
just to return the same -EIO.

--
Goldwyn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]