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

[Xiao Ni <xni@redhat.com>]

Hi Song

I'll do a performance test today and give the test result.

Regards
Xiao

On Tue, Nov 15, 2022 at 2:14 AM Song Liu <song@kernel.org> wrote:
>
> Hi Xiao,
>
> On Sun, Oct 23, 2022 at 11:48 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.
>
> I am very sorry to bring this up late. Have you tested the performance
> impact of this change? I am afraid this may introduce some visible
> performance regression for very high speed arrays.
>
> Thanks,
> Song
>
>
> >
> > Reported-by: Fine Fan <ffan@redhat.com>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> > V2: Move struct mddev* to the start of struct mddev_io_acct
> >  drivers/md/md.c    | 13 ++++++++++++-
> >  drivers/md/md.h    | 11 ++++++++---
> >  drivers/md/raid0.c |  6 ++++++
> >  3 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 6f3b2c1cb6cd..208f69849054 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -685,6 +685,7 @@ void mddev_init(struct mddev *mddev)
> >         atomic_set(&mddev->flush_pending, 0);
> >         init_waitqueue_head(&mddev->sb_wait);
> >         init_waitqueue_head(&mddev->recovery_wait);
> > +       init_waitqueue_head(&mddev->wait_io_acct);
> >         mddev->reshape_position = MaxSector;
> >         mddev->reshape_backwards = 0;
> >         mddev->last_sync_action = "none";
> > @@ -8618,15 +8619,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);
> > @@ -8635,12 +8639,17 @@ 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);
> > +
> > +       if (atomic_dec_and_test(&mddev->io_acct_cnt))
> > +               if (unlikely(test_bit(MD_QUIESCE, &mddev->flags)))
> > +                       wake_up(&mddev->wait_io_acct);
> >  }
> >
> >  /*
> > @@ -8660,6 +8669,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..a7c89ed53be5 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -255,6 +255,7 @@ struct md_cluster_info;
> >   *                array is ready yet.
> >   * @MD_BROKEN: This is used to stop writes and mark array as failed.
> >   * @MD_DELETED: This device is being deleted
> > + * @MD_QUIESCE: This device is being quiesced. Now only raid0 use this flag
> >   *
> >   * change UNSUPPORTED_MDDEV_FLAGS for each array type if new flag is added
> >   */
> > @@ -272,6 +273,7 @@ enum mddev_flags {
> >         MD_NOT_READY,
> >         MD_BROKEN,
> >         MD_DELETED,
> > +       MD_QUIESCE,
> >  };
> >
> >  enum mddev_sb_flags {
> > @@ -513,6 +515,8 @@ struct mddev {
> >                                                    * metadata and bitmap writes
> >                                                    */
> >         struct bio_set                  io_acct_set; /* for raid0 and raid5 io accounting */
> > +       atomic_t                        io_acct_cnt;
> > +       wait_queue_head_t               wait_io_acct;
> >
> >         /* Generic flush handling.
> >          * The last to finish preflush schedules a worker to submit
> > @@ -710,9 +714,10 @@ struct md_thread {
> >  };
> >
> >  struct md_io_acct {
> > -       struct bio *orig_bio;
> > -       unsigned long start_time;
> > -       struct bio bio_clone;
> > +       struct mddev    *mddev;
> > +       struct bio      *orig_bio;
> > +       unsigned long   start_time;
> > +       struct bio      bio_clone;
> >  };
> >
> >  #define THREAD_WAKEUP  0
> > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> > index 857c49399c28..aced0ad8cdab 100644
> > --- a/drivers/md/raid0.c
> > +++ b/drivers/md/raid0.c
> > @@ -754,6 +754,12 @@ static void *raid0_takeover(struct mddev *mddev)
> >
> >  static void raid0_quiesce(struct mddev *mddev, int quiesce)
> >  {
> > +       /* It doesn't use a separate struct to count how many bios are submitted
> > +        * to member disks to avoid memory alloc and performance decrease
> > +        */
> > +       set_bit(MD_QUIESCE, &mddev->flags);
> > +       wait_event(mddev->wait_io_acct, !atomic_read(&mddev->io_acct_cnt));
> > +       clear_bit(MD_QUIESCE, &mddev->flags);
> >  }
> >
> >  static struct md_personality raid0_personality=
> > --
> > 2.32.0 (Apple Git-132)
> >
>