| Message ID | c77fd4fd93c34a6d229765088ce0a88f7f8718d4.1680047473.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: scrub: use a more reader friendly code to implement scrub_simple_mirror() | expand |
> +struct btrfs_bio *btrfs_scrub_bio_alloc(blk_opf_t opf, > + struct btrfs_fs_info *fs_info, > + btrfs_bio_end_io_t end_io, void *private) > +{ > + struct btrfs_bio *bbio; > + struct bio *bio; > + > + bio = bio_alloc_bioset(NULL, BTRFS_STRIPE_LEN >> fs_info->sectorsize_bits, > + opf, GFP_NOFS, &btrfs_bioset); > + bbio = btrfs_bio(bio); > + memset(bbio, 0, offsetof(struct btrfs_bio, bio)); > + bbio->fs_info = fs_info; > + bbio->end_io = end_io; > + bbio->private = private; > + atomic_set(&bbio->pending_ios, 1); > + return bbio; As mentioned in the last round, I'm not too happy with this. With the inode and file_offset being optional now we might as well drop them as arguments from btrfs_bio_alloc/btrfs_bio_init and just pass a nr_vecs instead and make this new allocator obsolete, and it would be a much better result. I'd prefer to just do it now rather than doing another series changing it a little later. > + /* > + * Inode and offset into it that this I/O operates on. > + * > + * @inode can be NULL for callers who don't want any advanced features > + * like read-time repair. > + */ > struct btrfs_inode *inode; > u64 file_offset; I don't think these negative comments are nice for the reader. I'd do: /* * Inode and offset into it that this I/O operates on. * Only set for data I/O. */ > + /* > + * For cases where callers only want to read/write from a logical > + * bytenr, in that case @inode can be NULL, and we need such > + * @fs_info pointer to grab the corresponding fs_info. > + * > + * Should always be populated. > + */ > + struct btrfs_fs_info *fs_info; Again here, this comment only makes sense for people following the development history of this particular patch series. Once that is in the reason why people use an inode before is irrelevant. The only useful bit left here is that it must always be populated, but I'm not even sure I'd add that. So all we might need is: /* File system that this I/O operates on. */ What would be good in this patch is to replace the existing bbio->inode->root->fs_info dereferences with bbio->fs_info ASAP, though.
On 2023/3/30 07:32, Christoph Hellwig wrote: >> +struct btrfs_bio *btrfs_scrub_bio_alloc(blk_opf_t opf, >> + struct btrfs_fs_info *fs_info, >> + btrfs_bio_end_io_t end_io, void *private) >> +{ >> + struct btrfs_bio *bbio; >> + struct bio *bio; >> + >> + bio = bio_alloc_bioset(NULL, BTRFS_STRIPE_LEN >> fs_info->sectorsize_bits, >> + opf, GFP_NOFS, &btrfs_bioset); >> + bbio = btrfs_bio(bio); >> + memset(bbio, 0, offsetof(struct btrfs_bio, bio)); >> + bbio->fs_info = fs_info; >> + bbio->end_io = end_io; >> + bbio->private = private; >> + atomic_set(&bbio->pending_ios, 1); >> + return bbio; > > As mentioned in the last round, I'm not too happy with this. With > the inode and file_offset being optional now we might as well drop them > as arguments from btrfs_bio_alloc/btrfs_bio_init and just pass a nr_vecs > instead and make this new allocator obsolete, and it would be a much > better result. I would be happier if we drop @inode and @file_offset from btrfs_bio_alloc(). In that case, I'm completely fine to drop the new allocator. A quick glance into the code base, and it shows btrfs_bio_alloc/btrfs_bio_init() are not called that frequently, thus the change may be small enough to be done in the series. But as my usual tendency, it would still be better to have some ASSERT()s to make sure we're properly populating @inode for needed call sites. Or we can easily pass some bbio, which should go through csum verification, without a valid @inode pointer, and to be treated as NODATACSUM. Such problem can be very hard to detect. Any good suggestions? > > I'd prefer to just do it now rather than doing another series changing > it a little later. > >> + /* >> + * Inode and offset into it that this I/O operates on. >> + * >> + * @inode can be NULL for callers who don't want any advanced features >> + * like read-time repair. >> + */ >> struct btrfs_inode *inode; >> u64 file_offset; > > I don't think these negative comments are nice for the reader. I'd do: > > /* > * Inode and offset into it that this I/O operates on. > * Only set for data I/O. > */ > >> + /* >> + * For cases where callers only want to read/write from a logical >> + * bytenr, in that case @inode can be NULL, and we need such >> + * @fs_info pointer to grab the corresponding fs_info. >> + * >> + * Should always be populated. >> + */ >> + struct btrfs_fs_info *fs_info; > > Again here, this comment only makes sense for people following the > development history of this particular patch series. Once that is in > the reason why people use an inode before is irrelevant. The only > useful bit left here is that it must always be populated, but I'm not > even sure I'd add that. So all we might need is: > > /* File system that this I/O operates on. */ > > What would be good in this patch is to replace the > existing bbio->inode->root->fs_info dereferences with bbio->fs_info > ASAP, though. Indeed, the comments and extra @fs_info chain would be updated in the next version. Thanks, Qu
On Thu, Mar 30, 2023 at 07:39:43AM +0800, Qu Wenruo wrote: > But as my usual tendency, it would still be better to have some ASSERT()s to > make sure we're properly populating @inode for needed call sites. Well, where would you place them? The only place where we could do it would be in btrfs_submit_chunk, but without having the inode there is no way to know if we would have needed one. > Or we can easily pass some bbio, which should go through csum verification, > without a valid @inode pointer, and to be treated as NODATACSUM. > Such problem can be very hard to detect. That's what we have tests or that check that csums were there and used to detect and fix problems. In the new world order I'd rather see bbio->inode != NULL as flag to run the checksumming and repair infrastructure. Especially as with a bit more work I'll be able to never set bbio->inode for all metadata I/O either, and possibly get rid of the btree inode entirely.
On 2023/3/30 07:47, Christoph Hellwig wrote: > On Thu, Mar 30, 2023 at 07:39:43AM +0800, Qu Wenruo wrote: >> But as my usual tendency, it would still be better to have some ASSERT()s to >> make sure we're properly populating @inode for needed call sites. > > Well, where would you place them? The only place where we could do it > would be in btrfs_submit_chunk, but without having the inode there is > no way to know if we would have needed one. Can we do something like "if (bbio->file_offset) ASSERT(bbio->inode);"? > >> Or we can easily pass some bbio, which should go through csum verification, >> without a valid @inode pointer, and to be treated as NODATACSUM. >> Such problem can be very hard to detect. > > That's what we have tests or that check that csums were there and used > to detect and fix problems. OK, I guess I'd better change my ASSERT()s happy behavior... Thanks, Qu > > In the new world order I'd rather see bbio->inode != NULL as flag to > run the checksumming and repair infrastructure. Especially as with a > bit more work I'll be able to never set bbio->inode for all metadata > I/O either, and possibly get rid of the btree inode entirely.
On Thu, Mar 30, 2023 at 07:51:10AM +0800, Qu Wenruo wrote: > On 2023/3/30 07:47, Christoph Hellwig wrote: > > On Thu, Mar 30, 2023 at 07:39:43AM +0800, Qu Wenruo wrote: > > > But as my usual tendency, it would still be better to have some ASSERT()s to > > > make sure we're properly populating @inode for needed call sites. > > > > Well, where would you place them? The only place where we could do it > > would be in btrfs_submit_chunk, but without having the inode there is > > no way to know if we would have needed one. > > Can we do something like "if (bbio->file_offset) ASSERT(bbio->inode);"? We could do it, but it's also a bit confusing as NULL is a perfectly valid file offset as well. But I guess the inverse would make a little more sense ASSERT(bbio->inode || bbio->file_offset == 0);
diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c index cf09c6271edb..c1edadc17260 100644 --- a/fs/btrfs/bio.c +++ b/fs/btrfs/bio.c @@ -36,6 +36,7 @@ void btrfs_bio_init(struct btrfs_bio *bbio, struct btrfs_inode *inode, { memset(bbio, 0, offsetof(struct btrfs_bio, bio)); bbio->inode = inode; + bbio->fs_info = inode->root->fs_info; bbio->end_io = end_io; bbio->private = private; atomic_set(&bbio->pending_ios, 1); @@ -61,6 +62,30 @@ struct btrfs_bio *btrfs_bio_alloc(unsigned int nr_vecs, blk_opf_t opf, return bbio; } +/* + * Allocate a scrub specific btrfs_bio structure. + * + * This btrfs_bio would not go through any the btrfs special handling like + * checksum verification nor read-repair. + */ +struct btrfs_bio *btrfs_scrub_bio_alloc(blk_opf_t opf, + struct btrfs_fs_info *fs_info, + btrfs_bio_end_io_t end_io, void *private) +{ + struct btrfs_bio *bbio; + struct bio *bio; + + bio = bio_alloc_bioset(NULL, BTRFS_STRIPE_LEN >> fs_info->sectorsize_bits, + opf, GFP_NOFS, &btrfs_bioset); + bbio = btrfs_bio(bio); + memset(bbio, 0, offsetof(struct btrfs_bio, bio)); + bbio->fs_info = fs_info; + bbio->end_io = end_io; + bbio->private = private; + atomic_set(&bbio->pending_ios, 1); + return bbio; +} + static struct btrfs_bio *btrfs_split_bio(struct btrfs_fs_info *fs_info, struct btrfs_bio *orig_bbio, u64 map_length, bool use_append) diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h index dbf125f6fa33..3b97ce54140a 100644 --- a/fs/btrfs/bio.h +++ b/fs/btrfs/bio.h @@ -30,7 +30,12 @@ typedef void (*btrfs_bio_end_io_t)(struct btrfs_bio *bbio); * passed to btrfs_submit_bio for mapping to the physical devices. */ struct btrfs_bio { - /* Inode and offset into it that this I/O operates on. */ + /* + * Inode and offset into it that this I/O operates on. + * + * @inode can be NULL for callers who don't want any advanced features + * like read-time repair. + */ struct btrfs_inode *inode; u64 file_offset; @@ -58,6 +63,15 @@ struct btrfs_bio { atomic_t pending_ios; struct work_struct end_io_work; + /* + * For cases where callers only want to read/write from a logical + * bytenr, in that case @inode can be NULL, and we need such + * @fs_info pointer to grab the corresponding fs_info. + * + * Should always be populated. + */ + struct btrfs_fs_info *fs_info; + /* * This member must come last, bio_alloc_bioset will allocate enough * bytes for entire btrfs_bio but relies on bio being last. @@ -78,6 +92,9 @@ void btrfs_bio_init(struct btrfs_bio *bbio, struct btrfs_inode *inode, struct btrfs_bio *btrfs_bio_alloc(unsigned int nr_vecs, blk_opf_t opf, struct btrfs_inode *inode, btrfs_bio_end_io_t end_io, void *private); +struct btrfs_bio *btrfs_scrub_bio_alloc(blk_opf_t opf, + struct btrfs_fs_info *fs_info, + btrfs_bio_end_io_t end_io, void *private); static inline void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status) {
Currently we're doing a lot of work for btrfs_bio: - Checksum verification for data read bios - Bio splits if it crosses stripe boundary - Read repair for data read bios However for the incoming scrub patches, we don't want those extra functionality at all, just pure logical + mirror -> physical mapping ability. Thus here we introduce: - btrfs_bio::fs_info This is for the new scrub specific btrfs_bio, which would not populate btrfs_bio::inode. Thus we need such new member to grab a fs_info This new member would always be populated. - btrfs_scrub_bio_alloc() helper The main differences between this and btrfs_bio_alloc() are: * No need for nr_vecs As we know scrub bio should not cross stripe boundary. * Use @fs_info to replace @inode parameter - An extra ASSERT() to make sure btrfs_bio::fs_info is populated Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/bio.c | 25 +++++++++++++++++++++++++ fs/btrfs/bio.h | 19 ++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-)