diff mbox series

[14/32] btrfs: inode: make btrfs_readpage_end_io_hook() follow sector size

Message ID 20201103133108.148112-15-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series [01/32] btrfs: extent_io: remove the extent_start/extent_len for end_bio_extent_readpage() | expand

Commit Message

Qu Wenruo Nov. 3, 2020, 1:30 p.m. UTC 
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, 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 case, we do the loop to iterate each sector and if we found
any error, we return error.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Nikolay Borisov Nov. 5, 2020, 2:28 p.m. UTC | #1 
On 3.11.20 г. 15:30 ч., 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, 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 case, we do the loop to iterate each sector and if we found
> any error, we return error.

You refer to btrfs_readpage_end_io_hook but you actually change
btrfs_verity_data_csum. I guess the changelog needs adjusting.

> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/inode.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c54e0ed0b938..0432ca58eade 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2888,9 +2888,11 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
>  			   struct page *page, u64 start, u64 end, int mirror)
>  {
>  	size_t offset = start - page_offset(page);
> +	size_t pg_off;

nit: For offsets we should be using a more self-descriptive type such as
loff_t

>  	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;
>  
>  	if (PageChecked(page)) {
>  		ClearPageChecked(page);
> @@ -2910,7 +2912,15 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
>  	}
>  
>  	phy_offset >>= root->fs_info->sectorsize_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)
> +			return -EIO;
> +	}
> +	return 0;
>  }
>  
>  /*
>
David Sterba Nov. 6, 2020, 7:16 p.m. UTC | #2 
On Thu, Nov 05, 2020 at 04:28:12PM +0200, Nikolay Borisov wrote:
> 
> 
> On 3.11.20 г. 15:30 ч., 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, 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 case, we do the loop to iterate each sector and if we found
> > any error, we return error.
> 
> You refer to btrfs_readpage_end_io_hook but you actually change
> btrfs_verity_data_csum. I guess the changelog needs adjusting.
> 
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >  fs/btrfs/inode.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c54e0ed0b938..0432ca58eade 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -2888,9 +2888,11 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
> >  			   struct page *page, u64 start, u64 end, int mirror)
> >  {
> >  	size_t offset = start - page_offset(page);
> > +	size_t pg_off;
> 
> nit: For offsets we should be using a more self-descriptive type such as
> loff_t

loff_t is meant for file offsets and is an overkill when it's used for
offset in page that's fine with an u32.
David Sterba Nov. 6, 2020, 7:20 p.m. UTC | #3 
On Fri, Nov 06, 2020 at 08:16:08PM +0100, David Sterba wrote:
> On Thu, Nov 05, 2020 at 04:28:12PM +0200, Nikolay Borisov wrote:
> > On 3.11.20 г. 15:30 ч., Qu Wenruo wrote:
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -2888,9 +2888,11 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
> > >  			   struct page *page, u64 start, u64 end, int mirror)
> > >  {
> > >  	size_t offset = start - page_offset(page);
> > > +	size_t pg_off;
> > 
> > nit: For offsets we should be using a more self-descriptive type such as
> > loff_t
> 
> loff_t is meant for file offsets and is an overkill when it's used for
> offset in page that's fine with an u32.

Ok, so page_offset also uses loff_t but there are now mixed loff_t,
size_t and u64 in the function so I'd rather make it all u64.
David Sterba Nov. 6, 2020, 7:28 p.m. UTC | #4 
On Tue, Nov 03, 2020 at 09:30:50PM +0800, 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, 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 case, we do the loop to iterate each sector and if we found
> any error, we return error.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/inode.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c54e0ed0b938..0432ca58eade 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2888,9 +2888,11 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
>  			   struct page *page, 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;
>  
>  	if (PageChecked(page)) {
>  		ClearPageChecked(page);
> @@ -2910,7 +2912,15 @@ int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
>  	}
>  
>  	phy_offset >>= root->fs_info->sectorsize_bits;
> -	return check_data_csum(inode, io_bio, phy_offset, page, offset);
> +	for (pg_off = offset; pg_off < end - page_offset(page);

You can reause offset and not add pg_off.

> +	     pg_off += sectorsize, phy_offset++) {

phy_offsset is u64

> +		int ret;
> +
> +		ret = check_data_csum(inode, io_bio, phy_offset, page, pg_off);

Here it's passed as 'int icsum' to check_data_csum and pg_off is 'int
pgoff', like what's on here ...

> +		if (ret < 0)
> +			return -EIO;
> +	}
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.29.2
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c54e0ed0b938..0432ca58eade 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2888,9 +2888,11 @@  int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
 			   struct page *page, 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;
 
 	if (PageChecked(page)) {
 		ClearPageChecked(page);
@@ -2910,7 +2912,15 @@  int btrfs_verify_data_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
 	}
 
 	phy_offset >>= root->fs_info->sectorsize_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)
+			return -EIO;
+	}
+	return 0;
 }
 
 /*