Re: [PATCH 1/1] Add mddev->io_acct_cnt for raid0_quiesce

[Xiao Ni <xni@redhat.com>]

Please ignore this patch. Now there is no place to wake up raid0_quiesce.
I'll send a new one.

Regards
Xiao

On Wed, Oct 12, 2022 at 5:12 PM Xiao Ni <xni@redhat.com> wrote:
>
> It has added io_acct_set for raid0/raid5 io accounting and it needs to
> alloc md_io_acct in the i/o path. They are free when the bios come back
> from member disks. Now we don't have a method to monitor if those bios
> are all come back. In the takeover process, it needs to free the raid0
> memory resource including the memory pool for md_io_acct. But maybe some
> bios are still not returned. When those bios are returned, it can cause
> panic bcause of introducing NULL pointer or invalid address.
>
> This patch adds io_acct_cnt. So when stopping raid0, it can use this
> to wait until all bios come back.
>
> Reported-by: Fine Fan <ffan@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  drivers/md/md.c    | 10 +++++++++-
>  drivers/md/md.h    |  8 +++++---
>  drivers/md/raid0.c |  8 ++++++++
>  drivers/md/raid0.h |  1 +
>  4 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 9dc0175280b4..d6e9fa914087 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8600,15 +8600,18 @@ int acct_bioset_init(struct mddev *mddev)
>  {
>         int err = 0;
>
> -       if (!bioset_initialized(&mddev->io_acct_set))
> +       if (!bioset_initialized(&mddev->io_acct_set)) {
> +               atomic_set(&mddev->io_acct_cnt, 0);
>                 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)
>  {
> +       WARN_ON(atomic_read(&mddev->io_acct_cnt) != 0);
>         bioset_exit(&mddev->io_acct_set);
>  }
>  EXPORT_SYMBOL_GPL(acct_bioset_exit);
> @@ -8617,12 +8620,15 @@ static void md_end_io_acct(struct bio *bio)
>  {
>         struct md_io_acct *md_io_acct = bio->bi_private;
>         struct bio *orig_bio = md_io_acct->orig_bio;
> +       struct mddev *mddev = md_io_acct->mddev;
>
>         orig_bio->bi_status = bio->bi_status;
>
>         bio_end_io_acct(orig_bio, md_io_acct->start_time);
>         bio_put(bio);
>         bio_endio(orig_bio);
> +
> +       atomic_dec(&mddev->io_acct_cnt);
>  }
>
>  /*
> @@ -8642,6 +8648,8 @@ void md_account_bio(struct mddev *mddev, struct bio **bio)
>         md_io_acct = container_of(clone, struct md_io_acct, bio_clone);
>         md_io_acct->orig_bio = *bio;
>         md_io_acct->start_time = bio_start_io_acct(*bio);
> +       md_io_acct->mddev = mddev;
> +       atomic_inc(&mddev->io_acct_cnt);
>
>         clone->bi_end_io = md_end_io_acct;
>         clone->bi_private = md_io_acct;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index b4e2d8b87b61..29d30642e13f 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -513,6 +513,7 @@ struct mddev {
>                                                    * metadata and bitmap writes
>                                                    */
>         struct bio_set                  io_acct_set; /* for raid0 and raid5 io accounting */
> +       atomic_t                        io_acct_cnt;
>
>         /* Generic flush handling.
>          * The last to finish preflush schedules a worker to submit
> @@ -710,9 +711,10 @@ struct md_thread {
>  };
>
>  struct md_io_acct {
> -       struct bio *orig_bio;
> -       unsigned long start_time;
> -       struct bio bio_clone;
> +       struct bio      *orig_bio;
> +       unsigned long   start_time;
> +       struct bio      bio_clone;
> +       struct mddev    *mddev;
>  };
>
>  #define THREAD_WAKEUP  0
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index 857c49399c28..1d2e098e0d52 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -73,6 +73,8 @@ static int create_strip_zones(struct mddev *mddev, struct r0conf **private_conf)
>         *private_conf = ERR_PTR(-ENOMEM);
>         if (!conf)
>                 return -ENOMEM;
> +
> +       init_waitqueue_head(&conf->wait_quiesce);
>         rdev_for_each(rdev1, mddev) {
>                 pr_debug("md/raid0:%s: looking at %pg\n",
>                          mdname(mddev),
> @@ -754,6 +756,12 @@ static void *raid0_takeover(struct mddev *mddev)
>
>  static void raid0_quiesce(struct mddev *mddev, int quiesce)
>  {
> +       struct r0conf *conf = mddev->private;
> +
> +       /* It doesn't use a separate struct to count how many bios are submitted
> +        * to member disks to avoid memory alloc and performance decrease
> +        */
> +       wait_event(conf->wait_quiesce, atomic_read(&mddev->io_acct_cnt) == 0);
>  }
>
>  static struct md_personality raid0_personality=
> diff --git a/drivers/md/raid0.h b/drivers/md/raid0.h
> index 3816e5477db1..560dec93d459 100644
> --- a/drivers/md/raid0.h
> +++ b/drivers/md/raid0.h
> @@ -27,6 +27,7 @@ struct r0conf {
>                                             * by strip_zone->dev */
>         int                     nr_strip_zones;
>         enum r0layout           layout;
> +       wait_queue_head_t       wait_quiesce;
>  };
>
>  #endif
> --
> 2.32.0 (Apple Git-132)
>