Re: [PATCH 2/2] md: Change active_io to percpu

[Paul Menzel <pmenzel@xxxxxxxxxxxxx>]

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?


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/