by Yu Kuai

[permalink] [raw]

Subject: Re: [PATCH -next 1/8] md: move initialization and destruction of 'io_acct_set' to md.c

Hi,

在 2023/06/20 16:35, Xiao Ni 写道:
> On Mon, Jun 19, 2023 at 8:50 PM Yu Kuai <[email protected]> wrote:
>>
>> From: Yu Kuai <[email protected]>
>>
>> 'io_acct_set' is only used for raid0 and raid456, prepare to use it for
>> raid1 and raid10, so that io accounting from different levels can be
>> consistent.
>>
>> By the way, follow up patches will also use this io clone mechanism to
>> make sure 'active_io' represents in flight io, not io that is dispatching,
>> so that mddev_suspend will wait for io to be done as desgined.
>
> Hi Kuai
>
> typo error: s/desgined/designed/g
>
> Before this patch the personality uses ->quiesce method to wait until
> all inflight ios come back. But I like this solution. It makes the
> codes simpler. Not sure if it can cause problems because it changes
> the meaning of ->active_io. I'm doing regression tests to check.

Yes, actually this is the first step that I'm tring to synchronize io
and raid configuration, and I'm planing to use 'active_io' to check if
normal io exist, following are follow up plans:

1. add a new counter(perhaps we can reuse queue->q_usage_counter), and
grab it for sync io;
2. refactor and expand 'pers->quiesce', allow normal io and sync io
to be both quiesced;
3. call 'pers->quiesce' before raid configuration;
4. add a new helper to iterate rdev, grab the new counter first;
5. a lot of cleanups;

Thanks,
Kuai
>
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> drivers/md/md.c | 27 ++++++++++-----------------
>> drivers/md/md.h | 2 --
>> drivers/md/raid0.c | 16 ++--------------
>> drivers/md/raid5.c | 41 +++++++++++------------------------------
>> 4 files changed, 23 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 8d62f85d2ab0..42347289195a 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5886,6 +5886,13 @@ int md_run(struct mddev *mddev)
>> goto exit_bio_set;
>> }
>>
>> + if (!bioset_initialized(&mddev->io_acct_set)) {
>> + err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
>> + offsetof(struct md_io_acct, bio_clone), 0);
>> + if (err)
>> + goto exit_sync_set;
>> + }
>> +
>> spin_lock(&pers_lock);
>> pers = find_pers(mddev->level, mddev->clevel);
>> if (!pers || !try_module_get(pers->owner)) {
>> @@ -6063,6 +6070,8 @@ int md_run(struct mddev *mddev)
>> module_put(pers->owner);
>> md_bitmap_destroy(mddev);
>> abort:
>> + bioset_exit(&mddev->io_acct_set);
>> +exit_sync_set:
>> bioset_exit(&mddev->sync_set);
>> exit_bio_set:
>> bioset_exit(&mddev->bio_set);
>> @@ -6286,6 +6295,7 @@ static void __md_stop(struct mddev *mddev)
>> percpu_ref_exit(&mddev->active_io);
>> bioset_exit(&mddev->bio_set);
>> bioset_exit(&mddev->sync_set);
>> + bioset_exit(&mddev->io_acct_set);
>> }
>>
>> void md_stop(struct mddev *mddev)
>> @@ -8651,23 +8661,6 @@ void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
>> }
>> EXPORT_SYMBOL_GPL(md_submit_discard_bio);
>>
>> -int acct_bioset_init(struct mddev *mddev)
>> -{
>> - int err = 0;
>> -
>> - if (!bioset_initialized(&mddev->io_acct_set))
>> - err = bioset_init(&mddev->io_acct_set, BIO_POOL_SIZE,
>> - offsetof(struct md_io_acct, bio_clone), 0);
>> - return err;
>> -}
>> -EXPORT_SYMBOL_GPL(acct_bioset_init);
>> -
>> -void acct_bioset_exit(struct mddev *mddev)
>> -{
>> - bioset_exit(&mddev->io_acct_set);
>> -}
>> -EXPORT_SYMBOL_GPL(acct_bioset_exit);
>> -
>> static void md_end_io_acct(struct bio *bio)
>> {
>> struct md_io_acct *md_io_acct = bio->bi_private;
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index 7cab9c7c45b8..11299d94b239 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -776,8 +776,6 @@ extern void md_error(struct mddev *mddev, struct md_rdev *rdev);
>> extern void md_finish_reshape(struct mddev *mddev);
>> void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
>> struct bio *bio, sector_t start, sector_t size);
>> -int acct_bioset_init(struct mddev *mddev);
>> -void acct_bioset_exit(struct mddev *mddev);
>> void md_account_bio(struct mddev *mddev, struct bio **bio);
>>
>> extern bool __must_check md_flush_request(struct mddev *mddev, struct bio *bio);
>> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
>> index f8ee9a95e25d..38d9209cada1 100644
>> --- a/drivers/md/raid0.c
>> +++ b/drivers/md/raid0.c
>> @@ -365,7 +365,6 @@ static void raid0_free(struct mddev *mddev, void *priv)
>> struct r0conf *conf = priv;
>>
>> free_conf(mddev, conf);
>> - acct_bioset_exit(mddev);
>> }
>>
>> static int raid0_run(struct mddev *mddev)
>> @@ -380,16 +379,11 @@ static int raid0_run(struct mddev *mddev)
>> if (md_check_no_bitmap(mddev))
>> return -EINVAL;
>>
>> - if (acct_bioset_init(mddev)) {
>> - pr_err("md/raid0:%s: alloc acct bioset failed.\n", mdname(mddev));
>> - return -ENOMEM;
>> - }
>> -
>> /* if private is not null, we are here after takeover */
>> if (mddev->private == NULL) {
>> ret = create_strip_zones(mddev, &conf);
>> if (ret < 0)
>> - goto exit_acct_set;
>> + return ret;
>> mddev->private = conf;
>> }
>> conf = mddev->private;
>> @@ -420,15 +414,9 @@ static int raid0_run(struct mddev *mddev)
>>
>> ret = md_integrity_register(mddev);
>> if (ret)
>> - goto free;
>> + free_conf(mddev, conf);
>>
>> return ret;
>> -
>> -free:
>> - free_conf(mddev, conf);
>> -exit_acct_set:
>> - acct_bioset_exit(mddev);
>> - return ret;
>> }
>>
>> static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index f8bc74e16811..29cf5455d7a5 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -7787,19 +7787,12 @@ static int raid5_run(struct mddev *mddev)
>> struct md_rdev *rdev;
>> struct md_rdev *journal_dev = NULL;
>> sector_t reshape_offset = 0;
>> - int i, ret = 0;
>> + int i;
>> long long min_offset_diff = 0;
>> int first = 1;
>>
>> - if (acct_bioset_init(mddev)) {
>> - pr_err("md/raid456:%s: alloc acct bioset failed.\n", mdname(mddev));
>> + if (mddev_init_writes_pending(mddev) < 0)
>> return -ENOMEM;
>> - }
>> -
>> - if (mddev_init_writes_pending(mddev) < 0) {
>> - ret = -ENOMEM;
>> - goto exit_acct_set;
>> - }
>>
>> if (mddev->recovery_cp != MaxSector)
>> pr_notice("md/raid:%s: not clean -- starting background reconstruction\n",
>> @@ -7830,8 +7823,7 @@ static int raid5_run(struct mddev *mddev)
>> (mddev->bitmap_info.offset || mddev->bitmap_info.file)) {
>> pr_notice("md/raid:%s: array cannot have both journal and bitmap\n",
>> mdname(mddev));
>> - ret = -EINVAL;
>> - goto exit_acct_set;
>> + return -EINVAL;
>> }
>>
>> if (mddev->reshape_position != MaxSector) {
>> @@ -7856,15 +7848,13 @@ static int raid5_run(struct mddev *mddev)
>> if (journal_dev) {
>> pr_warn("md/raid:%s: don't support reshape with journal - aborting.\n",
>> mdname(mddev));
>> - ret = -EINVAL;
>> - goto exit_acct_set;
>> + return -EINVAL;
>> }
>>
>> if (mddev->new_level != mddev->level) {
>> pr_warn("md/raid:%s: unsupported reshape required - aborting.\n",
>> mdname(mddev));
>> - ret = -EINVAL;
>> - goto exit_acct_set;
>> + return -EINVAL;
>> }
>> old_disks = mddev->raid_disks - mddev->delta_disks;
>> /* reshape_position must be on a new-stripe boundary, and one
>> @@ -7880,8 +7870,7 @@ static int raid5_run(struct mddev *mddev)
>> if (sector_div(here_new, chunk_sectors * new_data_disks)) {
>> pr_warn("md/raid:%s: reshape_position not on a stripe boundary\n",
>> mdname(mddev));
>> - ret = -EINVAL;
>> - goto exit_acct_set;
>> + return -EINVAL;
>> }
>> reshape_offset = here_new * chunk_sectors;
>> /* here_new is the stripe we will write to */
>> @@ -7903,8 +7892,7 @@ static int raid5_run(struct mddev *mddev)
>> else if (mddev->ro == 0) {
>> pr_warn("md/raid:%s: in-place reshape must be started in read-only mode - aborting\n",
>> mdname(mddev));
>> - ret = -EINVAL;
>> - goto exit_acct_set;
>> + return -EINVAL;
>> }
>> } else if (mddev->reshape_backwards
>> ? (here_new * chunk_sectors + min_offset_diff <=
>> @@ -7914,8 +7902,7 @@ static int raid5_run(struct mddev *mddev)
>> /* Reading from the same stripe as writing to - bad */
>> pr_warn("md/raid:%s: reshape_position too early for auto-recovery - aborting.\n",
>> mdname(mddev));
>> - ret = -EINVAL;
>> - goto exit_acct_set;
>> + return -EINVAL;
>> }
>> pr_debug("md/raid:%s: reshape will continue\n", mdname(mddev));
>> /* OK, we should be able to continue; */
>> @@ -7939,10 +7926,8 @@ static int raid5_run(struct mddev *mddev)
>> else
>> conf = mddev->private;
>>
>> - if (IS_ERR(conf)) {
>> - ret = PTR_ERR(conf);
>> - goto exit_acct_set;
>> - }
>> + if (IS_ERR(conf))
>> + return PTR_ERR(conf);
>>
>> if (test_bit(MD_HAS_JOURNAL, &mddev->flags)) {
>> if (!journal_dev) {
>> @@ -8140,10 +8125,7 @@ static int raid5_run(struct mddev *mddev)
>> free_conf(conf);
>> mddev->private = NULL;
>> pr_warn("md/raid:%s: failed to run raid set.\n", mdname(mddev));
>> - ret = -EIO;
>> -exit_acct_set:
>> - acct_bioset_exit(mddev);
>> - return ret;
>> + return -EIO;
>> }
>>
>> static void raid5_free(struct mddev *mddev, void *priv)
>> @@ -8151,7 +8133,6 @@ static void raid5_free(struct mddev *mddev, void *priv)
>> struct r5conf *conf = priv;
>>
>> free_conf(conf);
>> - acct_bioset_exit(mddev);
>> mddev->to_remove = &raid5_attrs_group;
>> }
>>
>> --
>> 2.39.2
>>
>
> The patch is good for me.
>
> Reviewed-by: Xiao Ni <[email protected]>
>
> .
>

2023-06-21 04:02:17

by Yu Kuai

[permalink] [raw]

Subject: Re: [PATCH -next 3/8] raid5: fix missing io accounting in raid5_align_endio()

Hi,

在 2023/06/20 17:57, Paul Menzel 写道:
> Dear Yu,
>
>
> Thank you for your patch.
>
> Am 19.06.23 um 22:48 schrieb Yu Kuai:
>> From: Yu Kuai <[email protected]>
>>
>> Io will only be accounted as done from raid5_align_endio() if the io
>> succeed, and io inflight counter will be leaked if such io failed.
>
> succeed*s* or succeed*ed*?

I'll up date this.

>
>> Fix this problem by switching to use md_account_bio() for io accounting.
>
> How can this be tested?
>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> drivers/md/raid5.c | 29 ++++++++---------------------
>> 1 file changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index cef0b400b2ee..4cdb35e54251 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5468,26 +5468,17 @@ static struct bio
>> *remove_bio_from_retry(struct r5conf *conf,
>>    */
>> static void raid5_align_endio(struct bio *bi)
>> {
>> -    struct md_io_clone *md_io_clone = bi->bi_private;
>> -    struct bio *raid_bi = md_io_clone->orig_bio;
>> -    struct mddev *mddev;
>> -    struct r5conf *conf;
>> -    struct md_rdev *rdev;
>> +    struct bio *raid_bi = bi->bi_private;
>> +    struct md_rdev *rdev = (void *)raid_bi->bi_next;
>> +    struct mddev *mddev = rdev->mddev;
>> +    struct r5conf *conf = mddev->private;
>>       blk_status_t error = bi->bi_status;
>> -    unsigned long start_time = md_io_clone->start_time;
>>       bio_put(bi);
>> -
>> -    rdev = (void*)raid_bi->bi_next;
>>       raid_bi->bi_next = NULL;
>> -    mddev = rdev->mddev;
>> -    conf = mddev->private;
>> -
>
> This looks like unnecessary refactoring. No idea what the preferred
> style for the subsystem is though. If it is wanted, maybe make it a
> separate commit?

You mean that I initialize 'rdev' and 'mdev' while declaration?
I think code is cleaner this way, and this is too tiny to make a patch
for this... I will keep this for now. ????

Thanks,
Kuai

>
>>       rdev_dec_pending(rdev, conf->mddev);
>>       if (!error) {
>> -        if (blk_queue_io_stat(raid_bi->bi_bdev->bd_disk->queue))
>> -            bio_end_io_acct(raid_bi, start_time);
>>           bio_endio(raid_bi);
>>           if (atomic_dec_and_test(&conf->active_aligned_reads))
>>               wake_up(&conf->wait_for_quiescent);
>> @@ -5506,7 +5497,6 @@ static int raid5_read_one_chunk(struct mddev
>> *mddev, struct bio *raid_bio)
>>       struct md_rdev *rdev;
>>       sector_t sector, end_sector, first_bad;
>>       int bad_sectors, dd_idx;
>> -    struct md_io_clone *md_io_clone;
>>       bool did_inc;
>>       if (!in_chunk_boundary(mddev, raid_bio)) {
>> @@ -5543,16 +5533,13 @@ static int raid5_read_one_chunk(struct mddev
>> *mddev, struct bio *raid_bio)
>>           return 0;
>>       }
>> -    align_bio = bio_alloc_clone(rdev->bdev, raid_bio, GFP_NOIO,
>> -                    &mddev->io_clone_set);
>> -    md_io_clone = container_of(align_bio, struct md_io_clone,
>> bio_clone);
>> +    md_account_bio(mddev, &raid_bio);
>>       raid_bio->bi_next = (void *)rdev;
>> -    if (blk_queue_io_stat(raid_bio->bi_bdev->bd_disk->queue))
>> -        md_io_clone->start_time = bio_start_io_acct(raid_bio);
>> -    md_io_clone->orig_bio = raid_bio;
>> +    align_bio = bio_alloc_clone(rdev->bdev, raid_bio, GFP_NOIO,
>> +                    &mddev->bio_set);
>>       align_bio->bi_end_io = raid5_align_endio;
>> -    align_bio->bi_private = md_io_clone;
>> +    align_bio->bi_private = raid_bio;
>>       align_bio->bi_iter.bi_sector = sector;
>>       /* No reshape active, so we can trust rdev->data_offset */
>
>
> Kind regards,
>
> Paul
>
> .
>