On Mon, Jan 23, 2023 at 9:22 PM Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
>
> Dear Xiao,
>
>
> Am 21.01.23 um 02:39 schrieb Xiao Ni:
> > Now the type of active_io is atomic. It's used to count how many ios are
> > in the submitting process and it's added and decreased very time. But it
>
> s/very/every/
>
> > only needs to check if it's zero when suspending the raid. So we can
> > switch atomic to percpu to improve the performance.
> >
> > After switching active_io to percpu type, we use the state of active_io
> > to judge if the raid device is suspended. And we don't need to wake up
> > ->sb_wait in md_handle_request anymore. It's done in the callback function
> > which is registered when initing active_io. The argument mddev->suspended
> > is only used to count how many users are trying to set raid to suspend
> > state.
>
> How did you measure the performance impact?
Hi Paul
I used fio to do read/write/randread/randwrite tests on a raid0(nvme ssd hdd).
In my environment, there is no big change about the results.
Best Regards
Xiao
>
>
> Kind regards,
>
> Paul
>
>
> > Signed-off-by: Xiao Ni <xni@xxxxxxxxxx>
> > ---
> > drivers/md/md.c | 40 ++++++++++++++++++++++++----------------
> > drivers/md/md.h | 2 +-
> > 2 files changed, 25 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index d3627aad981a..04c067cf2f53 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -382,10 +382,7 @@ static DEFINE_SPINLOCK(all_mddevs_lock);
> >
> > static bool is_md_suspended(struct mddev *mddev)
> > {
> > - if (mddev->suspended)
> > - return true;
> > - else
> > - return false;
> > + return percpu_ref_is_dying(&mddev->active_io);
> > }
> > /* Rather than calling directly into the personality make_request function,
> > * IO requests come here first so that we can check if the device is
> > @@ -412,7 +409,6 @@ static bool is_suspended(struct mddev *mddev, struct bio *bio)
> > void md_handle_request(struct mddev *mddev, struct bio *bio)
> > {
> > check_suspended:
> > - rcu_read_lock();
> > if (is_suspended(mddev, bio)) {
> > DEFINE_WAIT(__wait);
> > /* Bail out if REQ_NOWAIT is set for the bio */
> > @@ -432,17 +428,15 @@ void md_handle_request(struct mddev *mddev, struct bio *bio)
> > }
> > finish_wait(&mddev->sb_wait, &__wait);
> > }
> > - atomic_inc(&mddev->active_io);
> > - rcu_read_unlock();
> > + if (!percpu_ref_tryget_live(&mddev->active_io))
> > + goto check_suspended;
> >
> > if (!mddev->pers->make_request(mddev, bio)) {
> > - atomic_dec(&mddev->active_io);
> > - wake_up(&mddev->sb_wait);
> > + percpu_ref_put(&mddev->active_io);
> > goto check_suspended;
> > }
> >
> > - if (atomic_dec_and_test(&mddev->active_io) && is_md_suspended(mddev))
> > - wake_up(&mddev->sb_wait);
> > + percpu_ref_put(&mddev->active_io);
> > }
> > EXPORT_SYMBOL(md_handle_request);
> >
> > @@ -488,11 +482,10 @@ void mddev_suspend(struct mddev *mddev)
> > lockdep_assert_held(&mddev->reconfig_mutex);
> > if (mddev->suspended++)
> > return;
> > - synchronize_rcu();
> > wake_up(&mddev->sb_wait);
> > set_bit(MD_ALLOW_SB_UPDATE, &mddev->flags);
> > - smp_mb__after_atomic();
> > - wait_event(mddev->sb_wait, atomic_read(&mddev->active_io) == 0);
> > + percpu_ref_kill(&mddev->active_io);
> > + wait_event(mddev->sb_wait, percpu_ref_is_zero(&mddev->active_io));
> > mddev->pers->quiesce(mddev, 1);
> > clear_bit_unlock(MD_ALLOW_SB_UPDATE, &mddev->flags);
> > wait_event(mddev->sb_wait, !test_bit(MD_UPDATING_SB, &mddev->flags));
> > @@ -510,6 +503,7 @@ void mddev_resume(struct mddev *mddev)
> > lockdep_assert_held(&mddev->reconfig_mutex);
> > if (--mddev->suspended)
> > return;
> > + percpu_ref_resurrect(&mddev->active_io);
> > wake_up(&mddev->sb_wait);
> > mddev->pers->quiesce(mddev, 0);
> >
> > @@ -688,7 +682,6 @@ void mddev_init(struct mddev *mddev)
> > timer_setup(&mddev->safemode_timer, md_safemode_timeout, 0);
> > atomic_set(&mddev->active, 1);
> > atomic_set(&mddev->openers, 0);
> > - atomic_set(&mddev->active_io, 0);
> > spin_lock_init(&mddev->lock);
> > atomic_set(&mddev->flush_pending, 0);
> > init_waitqueue_head(&mddev->sb_wait);
> > @@ -5765,6 +5758,12 @@ static void md_safemode_timeout(struct timer_list *t)
> > }
> >
> > static int start_dirty_degraded;
> > +static void active_io_release(struct percpu_ref *ref)
> > +{
> > + struct mddev *mddev = container_of(ref, struct mddev, active_io);
> > +
> > + wake_up(&mddev->sb_wait);
> > +}
> >
> > int md_run(struct mddev *mddev)
> > {
> > @@ -5845,10 +5844,15 @@ int md_run(struct mddev *mddev)
> > nowait = nowait && bdev_nowait(rdev->bdev);
> > }
> >
> > + err = percpu_ref_init(&mddev->active_io, active_io_release,
> > + PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
> > + if (err)
> > + return err;
> > +
> > if (!bioset_initialized(&mddev->bio_set)) {
> > err = bioset_init(&mddev->bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
> > if (err)
> > - return err;
> > + goto exit_active_io;
> > }
> > if (!bioset_initialized(&mddev->sync_set)) {
> > err = bioset_init(&mddev->sync_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
> > @@ -6036,6 +6040,8 @@ int md_run(struct mddev *mddev)
> > bioset_exit(&mddev->sync_set);
> > exit_bio_set:
> > bioset_exit(&mddev->bio_set);
> > +exit_active_io:
> > + percpu_ref_exit(&mddev->active_io);
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(md_run);
> > @@ -6260,6 +6266,7 @@ void md_stop(struct mddev *mddev)
> > */
> > __md_stop_writes(mddev);
> > __md_stop(mddev);
> > + percpu_ref_exit(&mddev->active_io);
> > bioset_exit(&mddev->bio_set);
> > bioset_exit(&mddev->sync_set);
> > }
> > @@ -7833,6 +7840,7 @@ static void md_free_disk(struct gendisk *disk)
> > struct mddev *mddev = disk->private_data;
> >
> > percpu_ref_exit(&mddev->writes_pending);
> > + percpu_ref_exit(&mddev->active_io);
> > bioset_exit(&mddev->bio_set);
> > bioset_exit(&mddev->sync_set);
> >
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index 554a9026669a..6335cb86e52e 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -315,7 +315,7 @@ struct mddev {
> > unsigned long sb_flags;
> >
> > int suspended;
> > - atomic_t active_io;
> > + struct percpu_ref active_io;
> > int ro;
> > int sysfs_active; /* set when sysfs deletes
> > * are happening, so run/
>
