| Message ID | 20230531041740.375963-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] btrfs: remove BTRFS_MAP_DISCARD | expand |
On 2023/5/31 12:17, Christoph Hellwig wrote: > Pass a smap into __btrfs_map_block so that the usual case of a read that > doesn't require parity raid recovery doesn't need an extra memory > allocation for the btrfs_io_context. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Qu Wenruo <wqu@suse.com> I'm more curious on whether the check-integrity feature is still under heavy usage. It's from old time where we don't have a lot of sanity checks, but nowadays it looks less worthy and can cause extra burden to maintain. Thanks, Qu > --- > fs/btrfs/check-integrity.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c > index b4408037b823c5..fe15367000141a 100644 > --- a/fs/btrfs/check-integrity.c > +++ b/fs/btrfs/check-integrity.c > @@ -1459,13 +1459,13 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len, > struct btrfs_fs_info *fs_info = state->fs_info; > int ret; > u64 length; > - struct btrfs_io_context *multi = NULL; > + struct btrfs_io_context *bioc = NULL; > + struct btrfs_io_stripe smap, *map; > struct btrfs_device *device; > > length = len; > - ret = btrfs_map_block(fs_info, BTRFS_MAP_READ, > - bytenr, &length, &multi, mirror_num); > - > + ret = __btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc, > + NULL, &mirror_num, 0); > if (ret) { > block_ctx_out->start = 0; > block_ctx_out->dev_bytenr = 0; > @@ -1478,21 +1478,26 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len, > return ret; > } > > - device = multi->stripes[0].dev; > + if (bioc) > + map = &bioc->stripes[0]; > + else > + map = &smap; > + > + device = map->dev; > if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state) || > !device->bdev || !device->name) > block_ctx_out->dev = NULL; > else > block_ctx_out->dev = btrfsic_dev_state_lookup( > device->bdev->bd_dev); > - block_ctx_out->dev_bytenr = multi->stripes[0].physical; > + block_ctx_out->dev_bytenr = map->physical; > block_ctx_out->start = bytenr; > block_ctx_out->len = len; > block_ctx_out->datav = NULL; > block_ctx_out->pagev = NULL; > block_ctx_out->mem_to_free = NULL; > > - kfree(multi); > + kfree(bioc); > if (NULL == block_ctx_out->dev) { > ret = -ENXIO; > pr_info("btrfsic: error, cannot lookup dev (#1)!\n");
On 31.05.23 10:49, Qu Wenruo wrote: > > > On 2023/5/31 12:17, Christoph Hellwig wrote: >> Pass a smap into __btrfs_map_block so that the usual case of a read that >> doesn't require parity raid recovery doesn't need an extra memory >> allocation for the btrfs_io_context. >> >> Signed-off-by: Christoph Hellwig <hch@lst.de> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > I'm more curious on whether the check-integrity feature is still under > heavy usage. > > It's from old time where we don't have a lot of sanity checks, but > nowadays it looks less worthy and can cause extra burden to maintain. I was going to ask the same question. I wouldn't mind removing it at all.
On Wed, May 31, 2023 at 12:31:24PM +0000, Johannes Thumshirn wrote: > > I'm more curious on whether the check-integrity feature is still under > > heavy usage. > > > > It's from old time where we don't have a lot of sanity checks, but > > nowadays it looks less worthy and can cause extra burden to maintain. > > I was going to ask the same question. I wouldn't mind removing it > at all. I've never actively used it and defintively had my fair amount of run ins with the somewhat interesting kind of code there.
On 31.05.23 14:35, Christoph Hellwig wrote: > On Wed, May 31, 2023 at 12:31:24PM +0000, Johannes Thumshirn wrote: >>> I'm more curious on whether the check-integrity feature is still under >>> heavy usage. >>> >>> It's from old time where we don't have a lot of sanity checks, but >>> nowadays it looks less worthy and can cause extra burden to maintain. >> >> I was going to ask the same question. I wouldn't mind removing it >> at all. > > I've never actively used it and defintively had my fair amount of run > ins with the somewhat interesting kind of code there. > And the fact that it comes with a huge "DANGEROUS" in Kconfig doesn't make the situation better IMHO. I guess we should just schedule it for removal in 1-2 releases. The last "proper" code touching check-integrity.c was cf90c59e680e ("Btrfs: check-int: don't complain about balanced blocks") and that went in 9 years ago. Josef, David, what's your take on this?
diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c index b4408037b823c5..fe15367000141a 100644 --- a/fs/btrfs/check-integrity.c +++ b/fs/btrfs/check-integrity.c @@ -1459,13 +1459,13 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len, struct btrfs_fs_info *fs_info = state->fs_info; int ret; u64 length; - struct btrfs_io_context *multi = NULL; + struct btrfs_io_context *bioc = NULL; + struct btrfs_io_stripe smap, *map; struct btrfs_device *device; length = len; - ret = btrfs_map_block(fs_info, BTRFS_MAP_READ, - bytenr, &length, &multi, mirror_num); - + ret = __btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc, + NULL, &mirror_num, 0); if (ret) { block_ctx_out->start = 0; block_ctx_out->dev_bytenr = 0; @@ -1478,21 +1478,26 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len, return ret; } - device = multi->stripes[0].dev; + if (bioc) + map = &bioc->stripes[0]; + else + map = &smap; + + device = map->dev; if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state) || !device->bdev || !device->name) block_ctx_out->dev = NULL; else block_ctx_out->dev = btrfsic_dev_state_lookup( device->bdev->bd_dev); - block_ctx_out->dev_bytenr = multi->stripes[0].physical; + block_ctx_out->dev_bytenr = map->physical; block_ctx_out->start = bytenr; block_ctx_out->len = len; block_ctx_out->datav = NULL; block_ctx_out->pagev = NULL; block_ctx_out->mem_to_free = NULL; - kfree(multi); + kfree(bioc); if (NULL == block_ctx_out->dev) { ret = -ENXIO; pr_info("btrfsic: error, cannot lookup dev (#1)!\n");
Pass a smap into __btrfs_map_block so that the usual case of a read that doesn't require parity raid recovery doesn't need an extra memory allocation for the btrfs_io_context. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/check-integrity.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)