Message ID | 20201021062554.68132-9-wqu@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs: add basic rw support for subpage sector size | expand |
In $SUBJECT, will prefer s/sink/remove/ On 14:24 21/10, 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> Reviewed-by: Goldwyn Rodrigues <rgoldwyn@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). > + * @page: The page where the data to be verified is. > + * @pgoff: The offset inside the page. > + * > + * The length of such check is always one sector size. > + */ > static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio, > - int icsum, struct page *page, int pgoff, u64 start, > - size_t len) > + int icsum, struct page *page, int pgoff) > { > struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); > SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); > char *kaddr; > + u32 len = fs_info->sectorsize; > u16 csum_size = btrfs_super_csum_size(fs_info->super_copy); > u8 *csum_expected; > u8 csum[BTRFS_CSUM_SIZE]; > > + ASSERT(pgoff + len <= PAGE_SIZE); > + > csum_expected = ((u8 *)io_bio->csum) + icsum * csum_size; > > kaddr = kmap_atomic(page); > @@ -2815,8 +2828,8 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio, > kunmap_atomic(kaddr); > return 0; > zeroit: > - btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected, > - io_bio->mirror_num); > + btrfs_print_data_csum_error(BTRFS_I(inode), page_offset(page) + pgoff, > + csum, csum_expected, io_bio->mirror_num); > if (io_bio->device) > btrfs_dev_stat_inc_and_print(io_bio->device, > BTRFS_DEV_STAT_CORRUPTION_ERRS); > @@ -2855,8 +2868,7 @@ 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, start, > - (size_t)(end - start + 1)); > + return check_data_csum(inode, io_bio, phy_offset, page, offset); > } > > /* > @@ -7542,8 +7554,7 @@ static blk_status_t btrfs_check_read_dio_bio(struct inode *inode, > ASSERT(pgoff < PAGE_SIZE); > if (uptodate && > (!csum || !check_data_csum(inode, io_bio, icsum, > - bvec.bv_page, pgoff, > - start, sectorsize))) { > + bvec.bv_page, pgoff))) { > clean_io_failure(fs_info, failure_tree, io_tree, > start, bvec.bv_page, > btrfs_ino(BTRFS_I(inode)), > -- > 2.28.0 >
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.
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? If it's 1, it means we need to skip 1 csum which is in csum_size. Or again my bad words? Thanks, Qu
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.
On 2020/10/28 上午7:17, David Sterba wrote: > 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. > Any better naming alternative for that? Or maybe I can refactor the function by passing in the current file_offset into the function, and let check_data_csum() to calculate the csum offset by itself? Thanks, Qu
On Wed, Oct 28, 2020 at 08:57:07AM +0800, Qu Wenruo wrote: > On 2020/10/28 上午7:17, David Sterba wrote: > > 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: > >>>> +/* > >>>> + * 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. > > Any better naming alternative for that? > > Or maybe I can refactor the function by passing in the current > file_offset into the function, and let check_data_csum() to calculate > the csum offset by itself? It was only the parameter description that was a bit confusing, no need to change anything else here.
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). + * @page: The page where the data to be verified is. + * @pgoff: The offset inside the page. + * + * The length of such check is always one sector size. + */ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio, - int icsum, struct page *page, int pgoff, u64 start, - size_t len) + int icsum, struct page *page, int pgoff) { struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); char *kaddr; + u32 len = fs_info->sectorsize; u16 csum_size = btrfs_super_csum_size(fs_info->super_copy); u8 *csum_expected; u8 csum[BTRFS_CSUM_SIZE]; + ASSERT(pgoff + len <= PAGE_SIZE); + csum_expected = ((u8 *)io_bio->csum) + icsum * csum_size; kaddr = kmap_atomic(page); @@ -2815,8 +2828,8 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio, kunmap_atomic(kaddr); return 0; zeroit: - btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected, - io_bio->mirror_num); + btrfs_print_data_csum_error(BTRFS_I(inode), page_offset(page) + pgoff, + csum, csum_expected, io_bio->mirror_num); if (io_bio->device) btrfs_dev_stat_inc_and_print(io_bio->device, BTRFS_DEV_STAT_CORRUPTION_ERRS); @@ -2855,8 +2868,7 @@ 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, start, - (size_t)(end - start + 1)); + return check_data_csum(inode, io_bio, phy_offset, page, offset); } /* @@ -7542,8 +7554,7 @@ static blk_status_t btrfs_check_read_dio_bio(struct inode *inode, ASSERT(pgoff < PAGE_SIZE); if (uptodate && (!csum || !check_data_csum(inode, io_bio, icsum, - bvec.bv_page, pgoff, - start, sectorsize))) { + bvec.bv_page, pgoff))) { clean_io_failure(fs_info, failure_tree, io_tree, start, bvec.bv_page, btrfs_ino(BTRFS_I(inode)),
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(-)