| Message ID | 20230605084519.580346-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs: factor out a btrfs_inode_is_nodatasum helper | expand |
On Mon, Jun 05, 2023 at 10:45:19AM +0200, Christoph Hellwig wrote: > Zoned file systems now need the ordereded_sums structure to record the > actual write location returned by zone append, so allocate dummy > structures without the csum array for them when the I/O doesn't use > checksums, and free them when completing the ordered_extent. > > Fixes: 5a1b7f2b6306 ("btrfs: optimize the logical to physical mapping for zoned writes") > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/bio.c | 4 ++++ > fs/btrfs/file-item.c | 12 ++++++++++-- > fs/btrfs/zoned.c | 11 ++++++++++- > 3 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c > index 627d06fbb4c425..2482739be49163 100644 > --- a/fs/btrfs/bio.c > +++ b/fs/btrfs/bio.c > @@ -668,6 +668,10 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) > ret = btrfs_bio_csum(bbio); > if (ret) > goto fail_put_bio; > + } else if (use_append) { > + ret = btrfs_csum_one_bio(bbio); > + if (ret) > + goto fail_put_bio; > } > } > > diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c > index 5e6603e76e5ac0..1308c369b1ebd8 100644 > --- a/fs/btrfs/file-item.c > +++ b/fs/btrfs/file-item.c > @@ -751,8 +751,16 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_bio *bbio) > sums->logical = bio->bi_iter.bi_sector << SECTOR_SHIFT; > index = 0; > > - shash->tfm = fs_info->csum_shash; > + /* > + * If we are called for a nodatasum inode, this means we are on a zoned > + * file system. In this case a ordered_csum structure needs to be > + * allocated to track the logical address actually written, but it does > + * notactually carry any checksums. > + */ > + if (btrfs_inode_is_nodatasum(inode)) So nodatasum in checksum bio implies zoned mode, this looks fragile. Why can't you check btrfs_is_zoned() instead? The comment is needed but I don't agree the condition should omit zoned mode itself. > + goto done; > > + shash->tfm = fs_info->csum_shash; > bio_for_each_segment(bvec, bio, iter) { > if (!ordered) { > ordered = btrfs_lookup_ordered_extent(inode, offset); > @@ -817,7 +825,7 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_bio *bbio) > > } > this_sum_bytes = 0; > - > +done: > /* > * The ->sums assignment is for zoned writes, where a bio never spans > * ordered extents and is only done unconditionally because that's cheaper > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index bbde4ddd475492..1f5497b9b2695c 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -1717,7 +1717,7 @@ void btrfs_finish_ordered_zoned(struct btrfs_ordered_extent *ordered) > if (!btrfs_zoned_split_ordered(ordered, logical, len)) { > set_bit(BTRFS_ORDERED_IOERR, &ordered->flags); > btrfs_err(fs_info, "failed to split ordered extent\n"); > - return; > + goto out; > } > logical = sum->logical; > len = sum->len; > @@ -1725,6 +1725,15 @@ void btrfs_finish_ordered_zoned(struct btrfs_ordered_extent *ordered) > > if (ordered->disk_bytenr != logical) > btrfs_rewrite_logical_zoned(ordered, logical); > + > +out: > + if (btrfs_inode_is_nodatasum(BTRFS_I(ordered->inode))) { The zoned mode here is implied by the calling function, open coding the condtions should not be a big deal, i.e. not needing the helper. > + while ((sum = list_first_entry_or_null(&ordered->list, > + typeof(*sum), list))) { > + list_del(&sum->list); > + kfree(sum); > + } > + } > } > > bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info, > -- > 2.39.2
On Tue, Jun 06, 2023 at 07:11:43PM +0200, David Sterba wrote: > > + /* > > + * If we are called for a nodatasum inode, this means we are on a zoned > > + * file system. In this case a ordered_csum structure needs to be > > + * allocated to track the logical address actually written, but it does > > + * notactually carry any checksums. > > + */ > > + if (btrfs_inode_is_nodatasum(inode)) > > So nodatasum in checksum bio implies zoned mode, this looks fragile. Why > can't you check btrfs_is_zoned() instead? The comment is needed but I > don't agree the condition should omit zoned mode itself. We can't check that instead, as for zoned mode with checksums we should not take this branch. If we want to check btrfs_is_zoned() here it would have to be an assert inside this branch to make things more clear. Alternatively we could be a "bool skip_csum" argument, which is what I did initially, but which felt more clumsy to me. > > > > if (ordered->disk_bytenr != logical) > > btrfs_rewrite_logical_zoned(ordered, logical); > > + > > +out: > > + if (btrfs_inode_is_nodatasum(BTRFS_I(ordered->inode))) { > > The zoned mode here is implied by the calling function, open coding the > condtions should not be a big deal, i.e. not needing the helper. The zoned mode is implied. But that's not what the helper checks for, it checks for the per-inode or per-fs nodatasum bits. But I can open code it if you prefer that.
diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c index 627d06fbb4c425..2482739be49163 100644 --- a/fs/btrfs/bio.c +++ b/fs/btrfs/bio.c @@ -668,6 +668,10 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) ret = btrfs_bio_csum(bbio); if (ret) goto fail_put_bio; + } else if (use_append) { + ret = btrfs_csum_one_bio(bbio); + if (ret) + goto fail_put_bio; } } diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c index 5e6603e76e5ac0..1308c369b1ebd8 100644 --- a/fs/btrfs/file-item.c +++ b/fs/btrfs/file-item.c @@ -751,8 +751,16 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_bio *bbio) sums->logical = bio->bi_iter.bi_sector << SECTOR_SHIFT; index = 0; - shash->tfm = fs_info->csum_shash; + /* + * If we are called for a nodatasum inode, this means we are on a zoned + * file system. In this case a ordered_csum structure needs to be + * allocated to track the logical address actually written, but it does + * notactually carry any checksums. + */ + if (btrfs_inode_is_nodatasum(inode)) + goto done; + shash->tfm = fs_info->csum_shash; bio_for_each_segment(bvec, bio, iter) { if (!ordered) { ordered = btrfs_lookup_ordered_extent(inode, offset); @@ -817,7 +825,7 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_bio *bbio) } this_sum_bytes = 0; - +done: /* * The ->sums assignment is for zoned writes, where a bio never spans * ordered extents and is only done unconditionally because that's cheaper diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index bbde4ddd475492..1f5497b9b2695c 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -1717,7 +1717,7 @@ void btrfs_finish_ordered_zoned(struct btrfs_ordered_extent *ordered) if (!btrfs_zoned_split_ordered(ordered, logical, len)) { set_bit(BTRFS_ORDERED_IOERR, &ordered->flags); btrfs_err(fs_info, "failed to split ordered extent\n"); - return; + goto out; } logical = sum->logical; len = sum->len; @@ -1725,6 +1725,15 @@ void btrfs_finish_ordered_zoned(struct btrfs_ordered_extent *ordered) if (ordered->disk_bytenr != logical) btrfs_rewrite_logical_zoned(ordered, logical); + +out: + if (btrfs_inode_is_nodatasum(BTRFS_I(ordered->inode))) { + while ((sum = list_first_entry_or_null(&ordered->list, + typeof(*sum), list))) { + list_del(&sum->list); + kfree(sum); + } + } } bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
Zoned file systems now need the ordereded_sums structure to record the actual write location returned by zone append, so allocate dummy structures without the csum array for them when the I/O doesn't use checksums, and free them when completing the ordered_extent. Fixes: 5a1b7f2b6306 ("btrfs: optimize the logical to physical mapping for zoned writes") Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/bio.c | 4 ++++ fs/btrfs/file-item.c | 12 ++++++++++-- fs/btrfs/zoned.c | 11 ++++++++++- 3 files changed, 24 insertions(+), 3 deletions(-)