| Message ID | bdf1bf5c65679fdf39021e16a242094acd71b270.1600961206.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New rescue mount options | expand |
On 2020/9/24 下午11:32, Josef Bacik wrote: > When we move to being able to handle NULL csum_roots it'll be cleaner to > just check in btrfs_lookup_bio_sums instead of at all of the caller > locations, so push the NODATASUM check into it as well so it's unified. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> But an off-topic question inlined below: > --- > fs/btrfs/compression.c | 14 +++++--------- > fs/btrfs/file-item.c | 3 +++ > fs/btrfs/inode.c | 12 +++++++++--- > 3 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index eeface30facd..7e1eb57b923c 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -722,11 +722,9 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, > */ > refcount_inc(&cb->pending_bios); > > - if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { > - ret = btrfs_lookup_bio_sums(inode, comp_bio, > - (u64)-1, sums); > - BUG_ON(ret); /* -ENOMEM */ Is it really possible to have compressed extent without data csum? Won't nodatacsum disable compression? Or are we just here to handle some old compressed but not csumed data? Thanks, Qu > - } > + ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, > + sums); > + BUG_ON(ret); /* -ENOMEM */ > > nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size, > fs_info->sectorsize); > @@ -751,10 +749,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, > ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA); > BUG_ON(ret); /* -ENOMEM */ > > - if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { > - ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums); > - BUG_ON(ret); /* -ENOMEM */ > - } > + ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums); > + BUG_ON(ret); /* -ENOMEM */ > > ret = btrfs_map_bio(fs_info, comp_bio, mirror_num); > if (ret) { > diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c > index 8f4f2bd6d9b9..8083d71d6af6 100644 > --- a/fs/btrfs/file-item.c > +++ b/fs/btrfs/file-item.c > @@ -272,6 +272,9 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, > int count = 0; > u16 csum_size = btrfs_super_csum_size(fs_info->super_copy); > > + if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM) > + return BLK_STS_OK; > + > path = btrfs_alloc_path(); > if (!path) > return BLK_STS_RESOURCE; > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index d526833b5ec0..d262944c4297 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -2202,7 +2202,12 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio, > mirror_num, > bio_flags); > goto out; > - } else if (!skip_sum) { > + } else { > + /* > + * Lookup bio sums does extra checks around whether we > + * need to csum or not, which is why we ignore skip_sum > + * here. > + */ > ret = btrfs_lookup_bio_sums(inode, bio, (u64)-1, NULL); > if (ret) > goto out; > @@ -7781,7 +7786,6 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap, > struct bio *dio_bio, loff_t file_offset) > { > const bool write = (bio_op(dio_bio) == REQ_OP_WRITE); > - const bool csum = !(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM); > struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); > const bool raid56 = (btrfs_data_alloc_profile(fs_info) & > BTRFS_BLOCK_GROUP_RAID56_MASK); > @@ -7808,10 +7812,12 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap, > return BLK_QC_T_NONE; > } > > - if (!write && csum) { > + if (!write) { > /* > * Load the csums up front to reduce csum tree searches and > * contention when submitting bios. > + * > + * If we have csums disabled this will do nothing. > */ > status = btrfs_lookup_bio_sums(inode, dio_bio, file_offset, > dip->csums); >
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 9/24/20 8:39 PM, Qu Wenruo wrote: > > > On 2020/9/24 下午11:32, Josef Bacik wrote: >> When we move to being able to handle NULL csum_roots it'll be cleaner to >> just check in btrfs_lookup_bio_sums instead of at all of the caller >> locations, so push the NODATASUM check into it as well so it's unified. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > But an off-topic question inlined below: > >> --- >> fs/btrfs/compression.c | 14 +++++--------- >> fs/btrfs/file-item.c | 3 +++ >> fs/btrfs/inode.c | 12 +++++++++--- >> 3 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c >> index eeface30facd..7e1eb57b923c 100644 >> --- a/fs/btrfs/compression.c >> +++ b/fs/btrfs/compression.c >> @@ -722,11 +722,9 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, >> */ >> refcount_inc(&cb->pending_bios); >> >> - if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { >> - ret = btrfs_lookup_bio_sums(inode, comp_bio, >> - (u64)-1, sums); >> - BUG_ON(ret); /* -ENOMEM */ > > Is it really possible to have compressed extent without data csum? > Won't nodatacsum disable compression? > > Or are we just here to handle some old compressed but not csumed data? > We used to allow it, so I'm content to leave this here. Maybe at some point we'll allow it in the future, but IDK it doesn't hurt anything to handle it here. Thanks, Josef
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index eeface30facd..7e1eb57b923c 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -722,11 +722,9 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, */ refcount_inc(&cb->pending_bios); - if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { - ret = btrfs_lookup_bio_sums(inode, comp_bio, - (u64)-1, sums); - BUG_ON(ret); /* -ENOMEM */ - } + ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, + sums); + BUG_ON(ret); /* -ENOMEM */ nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size, fs_info->sectorsize); @@ -751,10 +749,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA); BUG_ON(ret); /* -ENOMEM */ - if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { - ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums); - BUG_ON(ret); /* -ENOMEM */ - } + ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums); + BUG_ON(ret); /* -ENOMEM */ ret = btrfs_map_bio(fs_info, comp_bio, mirror_num); if (ret) { diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c index 8f4f2bd6d9b9..8083d71d6af6 100644 --- a/fs/btrfs/file-item.c +++ b/fs/btrfs/file-item.c @@ -272,6 +272,9 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, int count = 0; u16 csum_size = btrfs_super_csum_size(fs_info->super_copy); + if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM) + return BLK_STS_OK; + path = btrfs_alloc_path(); if (!path) return BLK_STS_RESOURCE; diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index d526833b5ec0..d262944c4297 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -2202,7 +2202,12 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio, mirror_num, bio_flags); goto out; - } else if (!skip_sum) { + } else { + /* + * Lookup bio sums does extra checks around whether we + * need to csum or not, which is why we ignore skip_sum + * here. + */ ret = btrfs_lookup_bio_sums(inode, bio, (u64)-1, NULL); if (ret) goto out; @@ -7781,7 +7786,6 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap, struct bio *dio_bio, loff_t file_offset) { const bool write = (bio_op(dio_bio) == REQ_OP_WRITE); - const bool csum = !(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM); struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); const bool raid56 = (btrfs_data_alloc_profile(fs_info) & BTRFS_BLOCK_GROUP_RAID56_MASK); @@ -7808,10 +7812,12 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap, return BLK_QC_T_NONE; } - if (!write && csum) { + if (!write) { /* * Load the csums up front to reduce csum tree searches and * contention when submitting bios. + * + * If we have csums disabled this will do nothing. */ status = btrfs_lookup_bio_sums(inode, dio_bio, file_offset, dip->csums);
When we move to being able to handle NULL csum_roots it'll be cleaner to just check in btrfs_lookup_bio_sums instead of at all of the caller locations, so push the NODATASUM check into it as well so it's unified. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/compression.c | 14 +++++--------- fs/btrfs/file-item.c | 3 +++ fs/btrfs/inode.c | 12 +++++++++--- 3 files changed, 17 insertions(+), 12 deletions(-)