From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Christoph Hellwig <hch@lst.de>, David Sterba <dsterba@suse.com>,
Josef Bacik <josef@toxicpanda.com>, Qu Wenruo <wqu@suse.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 4/5] btrfs: remove the raid56_parity_{write,recover} return value
Date: Thu, 16 Jun 2022 09:05:22 +0800 [thread overview]
Message-ID: <281a06be-aba0-bcce-5681-dc81b0245124@gmx.com> (raw)
In-Reply-To: <20220615151515.888424-5-hch@lst.de>
On 2022/6/15 23:15, Christoph Hellwig wrote:
> Follow the pattern of the main bio submission helper and do not return
> an error and always consume the bio. This allows these functions to
> consume the btrfs_bio_counter_ from the caller and thus avoid additional
> roundtrips on that counter.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/btrfs/raid56.c | 51 ++++++++++++++++++++++++----------------------
> fs/btrfs/raid56.h | 6 +++---
> fs/btrfs/scrub.c | 10 ++-------
> fs/btrfs/volumes.c | 19 ++++++++---------
> 4 files changed, 41 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index e071648f2c591..bd64147bd8bab 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1809,7 +1809,7 @@ static void rbio_add_bio(struct btrfs_raid_bio *rbio, struct bio *orig_bio)
> /*
> * our main entry point for writes from the rest of the FS.
> */
> -int raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
> +void raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
> {
> struct btrfs_fs_info *fs_info = bioc->fs_info;
> struct btrfs_raid_bio *rbio;
> @@ -1820,12 +1820,12 @@ int raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
> rbio = alloc_rbio(fs_info, bioc);
> if (IS_ERR(rbio)) {
> btrfs_put_bioc(bioc);
> - return PTR_ERR(rbio);
> + ret = PTR_ERR(rbio);
> + goto out_err;
out_err will call btrfs_bio_counter_dec(fs_info);
But at this branch, we don't yet have called
`btrfs_bio_counter_inc_noblocked()`.
Wouldn't this cause underflow?
Thanks,
Qu
> }
> rbio->operation = BTRFS_RBIO_WRITE;
> rbio_add_bio(rbio, bio);
>
> - btrfs_bio_counter_inc_noblocked(fs_info);
> rbio->generic_bio_cnt = 1;
>
> /*
> @@ -1835,8 +1835,8 @@ int raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
> if (rbio_is_full(rbio)) {
> ret = full_stripe_write(rbio);
> if (ret)
> - btrfs_bio_counter_dec(fs_info);
> - return ret;
> + goto out_err;
> + return;
> }
>
> cb = blk_check_plugged(btrfs_raid_unplug, fs_info, sizeof(*plug));
> @@ -1851,9 +1851,14 @@ int raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
> } else {
> ret = __raid56_parity_write(rbio);
> if (ret)
> - btrfs_bio_counter_dec(fs_info);
> + goto out_err;
> }
> - return ret;
> + return;
> +
> +out_err:
> + btrfs_bio_counter_dec(fs_info);
> + bio->bi_status = errno_to_blk_status(ret);
> + bio_endio(bio);
> }
>
> /*
> @@ -2199,8 +2204,8 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
> * so we assume the bio they send down corresponds to a failed part
> * of the drive.
> */
> -int raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> - int mirror_num, int generic_io)
> +void raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> + int mirror_num, bool generic_io)
> {
> struct btrfs_fs_info *fs_info = bioc->fs_info;
> struct btrfs_raid_bio *rbio;
> @@ -2209,13 +2214,14 @@ int raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> if (generic_io) {
> ASSERT(bioc->mirror_num == mirror_num);
> btrfs_bio(bio)->mirror_num = mirror_num;
> + } else {
> + btrfs_get_bioc(bioc);
> }
>
> rbio = alloc_rbio(fs_info, bioc);
> if (IS_ERR(rbio)) {
> - if (generic_io)
> - btrfs_put_bioc(bioc);
> - return PTR_ERR(rbio);
> + ret = PTR_ERR(rbio);
> + goto out_err;
> }
>
> rbio->operation = BTRFS_RBIO_READ_REBUILD;
> @@ -2227,18 +2233,12 @@ int raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> "%s could not find the bad stripe in raid56 so that we cannot recover any more (bio has logical %llu len %llu, bioc has map_type %llu)",
> __func__, bio->bi_iter.bi_sector << 9,
> (u64)bio->bi_iter.bi_size, bioc->map_type);
> - if (generic_io)
> - btrfs_put_bioc(bioc);
> kfree(rbio);
> - return -EIO;
> + goto out_err;
> }
>
> - if (generic_io) {
> - btrfs_bio_counter_inc_noblocked(fs_info);
> + if (generic_io)
> rbio->generic_bio_cnt = 1;
> - } else {
> - btrfs_get_bioc(bioc);
> - }
>
> /*
> * Loop retry:
> @@ -2257,8 +2257,6 @@ int raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> rbio->failb--;
> }
>
> - ret = lock_stripe_add(rbio);
> -
> /*
> * __raid56_parity_recover will end the bio with
> * any errors it hits. We don't want to return
> @@ -2266,15 +2264,20 @@ int raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> * will end up calling bio_endio with any nonzero
> * return
> */
> - if (ret == 0)
> + if (lock_stripe_add(rbio) == 0)
> __raid56_parity_recover(rbio);
> +
> /*
> * our rbio has been added to the list of
> * rbios that will be handled after the
> * currently lock owner is done
> */
> - return 0;
> + return;
>
> +out_err:
> + btrfs_put_bioc(bioc);
> + bio->bi_status = errno_to_blk_status(ret);
> + bio_endio(bio);
> }
>
> static void rmw_work(struct work_struct *work)
> diff --git a/fs/btrfs/raid56.h b/fs/btrfs/raid56.h
> index 9e4e0501e4e89..c94f503eb3832 100644
> --- a/fs/btrfs/raid56.h
> +++ b/fs/btrfs/raid56.h
> @@ -175,9 +175,9 @@ static inline int nr_data_stripes(const struct map_lookup *map)
>
> struct btrfs_device;
>
> -int raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> - int mirror_num, int generic_io);
> -int raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc);
> +void raid56_parity_recover(struct bio *bio, struct btrfs_io_context *bioc,
> + int mirror_num, bool generic_io);
> +void raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc);
>
> void raid56_add_scrub_pages(struct btrfs_raid_bio *rbio, struct page *page,
> unsigned int pgoff, u64 logical);
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 18986d062cf63..f091e57222082 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -1376,18 +1376,12 @@ static int scrub_submit_raid56_bio_wait(struct btrfs_fs_info *fs_info,
> struct scrub_sector *sector)
> {
> DECLARE_COMPLETION_ONSTACK(done);
> - int ret;
> - int mirror_num;
>
> bio->bi_iter.bi_sector = sector->logical >> 9;
> bio->bi_private = &done;
> bio->bi_end_io = scrub_bio_wait_endio;
> -
> - mirror_num = sector->sblock->sectors[0]->mirror_num;
> - ret = raid56_parity_recover(bio, sector->recover->bioc,
> - mirror_num, 0);
> - if (ret)
> - return ret;
> + raid56_parity_recover(bio, sector->recover->bioc,
> + sector->sblock->sectors[0]->mirror_num, false);
>
> wait_for_completion_io(&done);
> return blk_status_to_errno(bio->bi_status);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 739676944e94d..3a8c437bdd65b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6749,8 +6749,12 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> btrfs_bio_counter_inc_blocked(fs_info);
> ret = __btrfs_map_block(fs_info, btrfs_op(bio), logical,
> &map_length, &bioc, mirror_num, 1);
> - if (ret)
> - goto out_dec;
> + if (ret) {
> + btrfs_bio_counter_dec(fs_info);
> + bio->bi_status = errno_to_blk_status(ret);
> + bio_endio(bio);
> + return;
> + }
>
> total_devs = bioc->num_stripes;
> bioc->orig_bio = bio;
> @@ -6761,10 +6765,10 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> if ((bioc->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK) &&
> ((btrfs_op(bio) == BTRFS_MAP_WRITE) || (mirror_num > 1))) {
> if (btrfs_op(bio) == BTRFS_MAP_WRITE)
> - ret = raid56_parity_write(bio, bioc);
> + raid56_parity_write(bio, bioc);
> else
> - ret = raid56_parity_recover(bio, bioc, mirror_num, 1);
> - goto out_dec;
> + raid56_parity_recover(bio, bioc, mirror_num, true);
> + return;
> }
>
> if (map_length < length) {
> @@ -6779,12 +6783,7 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>
> submit_stripe_bio(bioc, bio, dev_nr, should_clone);
> }
> -out_dec:
> btrfs_bio_counter_dec(fs_info);
> - if (ret) {
> - bio->bi_status = errno_to_blk_status(ret);
> - bio_endio(bio);
> - }
> }
>
> static bool dev_args_match_fs_devices(const struct btrfs_dev_lookup_args *args,
next prev parent reply other threads:[~2022-06-16 1:05 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 15:15 cleanup btrfs bio submission Christoph Hellwig
2022-06-15 15:15 ` [PATCH 1/5] btrfs: remove a bunch of pointles stripe_len arguments Christoph Hellwig
2022-06-15 21:28 ` Qu Wenruo
2022-06-17 9:53 ` Johannes Thumshirn
2022-06-15 15:15 ` [PATCH 2/5] btrfs: return proper mapped length for RAID56 profiles in __btrfs_map_block() Christoph Hellwig
2022-06-17 9:54 ` Johannes Thumshirn
2022-06-15 15:15 ` [PATCH 3/5] btrfs: remove the btrfs_map_bio return value Christoph Hellwig
2022-06-15 16:59 ` Boris Burkov
2022-06-15 17:10 ` Christoph Hellwig
2022-06-16 1:02 ` Qu Wenruo
2022-06-17 9:55 ` Johannes Thumshirn
2022-06-15 15:15 ` [PATCH 4/5] btrfs: remove the raid56_parity_{write,recover} " Christoph Hellwig
2022-06-16 1:05 ` Qu Wenruo [this message]
2022-06-16 1:06 ` Qu Wenruo
2022-06-16 6:36 ` Christoph Hellwig
2022-06-16 9:53 ` Qu Wenruo
2022-06-16 10:54 ` Christoph Hellwig
2022-06-15 15:15 ` [PATCH 5/5] btrfs: remove bioc->stripes_pending Christoph Hellwig
2022-06-15 18:16 ` Boris Burkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=281a06be-aba0-bcce-5681-dc81b0245124@gmx.com \
--to=quwenruo.btrfs@gmx.com \
--cc=dsterba@suse.com \
--cc=hch@lst.de \
--cc=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=wqu@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.