| Message ID | 20220522114754.173685-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] btrfs: quit early if the fs has no RAID56 support for raid56 related checks | expand |
On 2022/5/22 19:47, Christoph Hellwig wrote: > From: Qu Wenruo <wqu@suse.com> > > Although we have several data csum verification code, we never have a > function really just to verify checksum for one sector. > > Function check_data_csum() do extra work for error reporting, thus it > requires a lot of extra things like file offset, bio_offset etc. > > Function btrfs_verify_data_csum() is even worse, it will utizlie page > checked flag, which means it can not be utilized for direct IO pages. > > Here we introduce a new helper, btrfs_check_sector_csum(), which really > only accept a sector in page, and expected checksum pointer. > > We use this function to implement check_data_csum(), and export it for > incoming patch. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > [hch: keep passing the csum array as an arguments, as the callers want > to print it, rename per request] Mind to constify the @csum_expected parameter? Thanks, Qu > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/compression.c | 13 ++++--------- > fs/btrfs/ctree.h | 2 ++ > fs/btrfs/inode.c | 38 ++++++++++++++++++++++++++++---------- > 3 files changed, 34 insertions(+), 19 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index f4564f32f6d93..6ab82e142f1f8 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -147,12 +147,10 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, > u64 disk_start) > { > struct btrfs_fs_info *fs_info = inode->root->fs_info; > - SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); > const u32 csum_size = fs_info->csum_size; > const u32 sectorsize = fs_info->sectorsize; > struct page *page; > unsigned int i; > - char *kaddr; > u8 csum[BTRFS_CSUM_SIZE]; > struct compressed_bio *cb = bio->bi_private; > u8 *cb_sum = cb->sums; > @@ -161,8 +159,6 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, > test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state)) > return 0; > > - shash->tfm = fs_info->csum_shash; > - > for (i = 0; i < cb->nr_pages; i++) { > u32 pg_offset; > u32 bytes_left = PAGE_SIZE; > @@ -175,12 +171,11 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, > /* Hash through the page sector by sector */ > for (pg_offset = 0; pg_offset < bytes_left; > pg_offset += sectorsize) { > - kaddr = kmap_atomic(page); > - crypto_shash_digest(shash, kaddr + pg_offset, > - sectorsize, csum); > - kunmap_atomic(kaddr); > + int ret; > > - if (memcmp(&csum, cb_sum, csum_size) != 0) { > + ret = btrfs_check_sector_csum(fs_info, page, pg_offset, > + csum, cb_sum); > + if (ret) { > btrfs_print_data_csum_error(inode, disk_start, > csum, cb_sum, cb->mirror_num); > if (btrfs_bio(bio)->device) > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 0e49b1a0c0716..8dd7d36b83ecb 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3253,6 +3253,8 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path); > /* inode.c */ > void btrfs_submit_data_bio(struct inode *inode, struct bio *bio, > int mirror_num, enum btrfs_compression_type compress_type); > +int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, > + u32 pgoff, u8 *csum, u8 *csum_expected); > unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio, > u32 bio_offset, struct page *page, > u64 start, u64 end); > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index da13bd0d10f12..e4acdec9ffc69 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3326,6 +3326,29 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, > finish_ordered_fn, uptodate); > } > > +/* > + * Verify the checksum for a single sector without any extra action that > + * depend on the type of I/O. > + */ > +int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, > + u32 pgoff, u8 *csum, u8 *csum_expected) > +{ > + SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); > + char *kaddr; > + > + ASSERT(pgoff + fs_info->sectorsize <= PAGE_SIZE); > + > + shash->tfm = fs_info->csum_shash; > + > + kaddr = kmap_local_page(page) + pgoff; > + crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum); > + kunmap_local(kaddr); > + > + if (memcmp(csum, csum_expected, fs_info->csum_size)) > + return -EIO; > + return 0; > +} > + > /* > * check_data_csum - verify checksum of one sector of uncompressed data > * @inode: inode > @@ -3336,14 +3359,15 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, > * @start: logical offset in the file > * > * The length of such check is always one sector size. > + * > + * When csum mismatch detected, we will also report the error and fill the > + * corrupted range with zero. (thus it needs the extra parameters) > */ > static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio, > u32 bio_offset, struct page *page, u32 pgoff, > u64 start) > { > 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; > const u32 csum_size = fs_info->csum_size; > unsigned int offset_sectors; > @@ -3355,16 +3379,10 @@ static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio, > offset_sectors = bio_offset >> fs_info->sectorsize_bits; > csum_expected = ((u8 *)bbio->csum) + offset_sectors * csum_size; > > - kaddr = kmap_atomic(page); > - shash->tfm = fs_info->csum_shash; > - > - crypto_shash_digest(shash, kaddr + pgoff, len, csum); > - kunmap_atomic(kaddr); > - > - if (memcmp(csum, csum_expected, csum_size)) > + if (btrfs_check_sector_csum(fs_info, page, pgoff, csum, csum_expected)) > goto zeroit; > - > return 0; > + > zeroit: > btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected, > bbio->mirror_num);
On Mon, May 23, 2022 at 08:38:13AM +0800, Qu Wenruo wrote: > > > On 2022/5/22 19:47, Christoph Hellwig wrote: >> From: Qu Wenruo <wqu@suse.com> >> >> Although we have several data csum verification code, we never have a >> function really just to verify checksum for one sector. >> >> Function check_data_csum() do extra work for error reporting, thus it >> requires a lot of extra things like file offset, bio_offset etc. >> >> Function btrfs_verify_data_csum() is even worse, it will utizlie page >> checked flag, which means it can not be utilized for direct IO pages. >> >> Here we introduce a new helper, btrfs_check_sector_csum(), which really >> only accept a sector in page, and expected checksum pointer. >> >> We use this function to implement check_data_csum(), and export it for >> incoming patch. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> [hch: keep passing the csum array as an arguments, as the callers want >> to print it, rename per request] > > Mind to constify the @csum_expected parameter? This would be the incremental diff, if Dave cares deeply he can fold it in: diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index d982ea62c521b..f01ce82af8ca9 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3262,7 +3262,7 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path); void btrfs_submit_data_bio(struct inode *inode, struct bio *bio, int mirror_num, enum btrfs_compression_type compress_type); int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, - u32 pgoff, u8 *csum, u8 *csum_expected); + u32 pgoff, u8 *csum, u8 * const csum_expected); unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio, u32 bio_offset, struct page *page, u64 start, u64 end); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index bfc0b0035b03c..c344ed0e057ac 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3330,7 +3330,7 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, * depend on the type of I/O. */ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, - u32 pgoff, u8 *csum, u8 *csum_expected) + u32 pgoff, u8 *csum, u8 * const csum_expected) { SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); char *kaddr;
On 2022/5/24 15:24, Christoph Hellwig wrote: > On Mon, May 23, 2022 at 08:38:13AM +0800, Qu Wenruo wrote: >> >> >> On 2022/5/22 19:47, Christoph Hellwig wrote: >>> From: Qu Wenruo <wqu@suse.com> >>> >>> Although we have several data csum verification code, we never have a >>> function really just to verify checksum for one sector. >>> >>> Function check_data_csum() do extra work for error reporting, thus it >>> requires a lot of extra things like file offset, bio_offset etc. >>> >>> Function btrfs_verify_data_csum() is even worse, it will utizlie page >>> checked flag, which means it can not be utilized for direct IO pages. >>> >>> Here we introduce a new helper, btrfs_check_sector_csum(), which really >>> only accept a sector in page, and expected checksum pointer. >>> >>> We use this function to implement check_data_csum(), and export it for >>> incoming patch. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> [hch: keep passing the csum array as an arguments, as the callers want >>> to print it, rename per request] >> >> Mind to constify the @csum_expected parameter? > > This would be the incremental diff, if Dave cares deeply he can fold > it in: > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index d982ea62c521b..f01ce82af8ca9 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3262,7 +3262,7 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path); > void btrfs_submit_data_bio(struct inode *inode, struct bio *bio, > int mirror_num, enum btrfs_compression_type compress_type); > int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, > - u32 pgoff, u8 *csum, u8 *csum_expected); > + u32 pgoff, u8 *csum, u8 * const csum_expected); Shouldn't it be "const u8 *" instead? Anyway, normally David will handle it manually if needed. I'm talking about this one just because my version is passing "const u8 *" for its csum_expected pointer, and caused warning here. Thanks, Qu > unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio, > u32 bio_offset, struct page *page, > u64 start, u64 end); > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index bfc0b0035b03c..c344ed0e057ac 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3330,7 +3330,7 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, > * depend on the type of I/O. > */ > int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, > - u32 pgoff, u8 *csum, u8 *csum_expected) > + u32 pgoff, u8 *csum, u8 * const csum_expected) > { > SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); > char *kaddr;
On Tue, May 24, 2022 at 04:07:41PM +0800, Qu Wenruo wrote: >> - u32 pgoff, u8 *csum, u8 *csum_expected); >> + u32 pgoff, u8 *csum, u8 * const csum_expected); > > Shouldn't it be "const u8 *" instead? No, that would bind to the pointer. But we care about the data.
On Tue, May 24, 2022 at 10:11:10AM +0200, Christoph Hellwig wrote: > On Tue, May 24, 2022 at 04:07:41PM +0800, Qu Wenruo wrote: > >> - u32 pgoff, u8 *csum, u8 *csum_expected); > >> + u32 pgoff, u8 *csum, u8 * const csum_expected); > > > > Shouldn't it be "const u8 *" instead? > > No, that would bind to the pointer. But we care about the data. Then it could be 'const u8 * const csum_expected'. The consts are not everywhere I would like them to see so in new code I care a bit more and add it when applying patches.
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index f4564f32f6d93..6ab82e142f1f8 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -147,12 +147,10 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, u64 disk_start) { struct btrfs_fs_info *fs_info = inode->root->fs_info; - SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); const u32 csum_size = fs_info->csum_size; const u32 sectorsize = fs_info->sectorsize; struct page *page; unsigned int i; - char *kaddr; u8 csum[BTRFS_CSUM_SIZE]; struct compressed_bio *cb = bio->bi_private; u8 *cb_sum = cb->sums; @@ -161,8 +159,6 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state)) return 0; - shash->tfm = fs_info->csum_shash; - for (i = 0; i < cb->nr_pages; i++) { u32 pg_offset; u32 bytes_left = PAGE_SIZE; @@ -175,12 +171,11 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, /* Hash through the page sector by sector */ for (pg_offset = 0; pg_offset < bytes_left; pg_offset += sectorsize) { - kaddr = kmap_atomic(page); - crypto_shash_digest(shash, kaddr + pg_offset, - sectorsize, csum); - kunmap_atomic(kaddr); + int ret; - if (memcmp(&csum, cb_sum, csum_size) != 0) { + ret = btrfs_check_sector_csum(fs_info, page, pg_offset, + csum, cb_sum); + if (ret) { btrfs_print_data_csum_error(inode, disk_start, csum, cb_sum, cb->mirror_num); if (btrfs_bio(bio)->device) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 0e49b1a0c0716..8dd7d36b83ecb 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3253,6 +3253,8 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path); /* inode.c */ void btrfs_submit_data_bio(struct inode *inode, struct bio *bio, int mirror_num, enum btrfs_compression_type compress_type); +int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, + u32 pgoff, u8 *csum, u8 *csum_expected); unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio, u32 bio_offset, struct page *page, u64 start, u64 end); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index da13bd0d10f12..e4acdec9ffc69 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3326,6 +3326,29 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, finish_ordered_fn, uptodate); } +/* + * Verify the checksum for a single sector without any extra action that + * depend on the type of I/O. + */ +int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page, + u32 pgoff, u8 *csum, u8 *csum_expected) +{ + SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); + char *kaddr; + + ASSERT(pgoff + fs_info->sectorsize <= PAGE_SIZE); + + shash->tfm = fs_info->csum_shash; + + kaddr = kmap_local_page(page) + pgoff; + crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum); + kunmap_local(kaddr); + + if (memcmp(csum, csum_expected, fs_info->csum_size)) + return -EIO; + return 0; +} + /* * check_data_csum - verify checksum of one sector of uncompressed data * @inode: inode @@ -3336,14 +3359,15 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode, * @start: logical offset in the file * * The length of such check is always one sector size. + * + * When csum mismatch detected, we will also report the error and fill the + * corrupted range with zero. (thus it needs the extra parameters) */ static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio, u32 bio_offset, struct page *page, u32 pgoff, u64 start) { 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; const u32 csum_size = fs_info->csum_size; unsigned int offset_sectors; @@ -3355,16 +3379,10 @@ static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio, offset_sectors = bio_offset >> fs_info->sectorsize_bits; csum_expected = ((u8 *)bbio->csum) + offset_sectors * csum_size; - kaddr = kmap_atomic(page); - shash->tfm = fs_info->csum_shash; - - crypto_shash_digest(shash, kaddr + pgoff, len, csum); - kunmap_atomic(kaddr); - - if (memcmp(csum, csum_expected, csum_size)) + if (btrfs_check_sector_csum(fs_info, page, pgoff, csum, csum_expected)) goto zeroit; - return 0; + zeroit: btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected, bbio->mirror_num);