[PATCH 1/1] md: Increase active_io to count acct_io

[PATCH 1/1] md: Increase active_io to count acct_io

[Xiao Ni]

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.

[ 6973.767999] RIP: 0010:mempool_free+0x52/0x80
[ 6973.786098] Call Trace:
[ 6973.786549]  md_end_io_acct+0x31/0x40
[ 6973.787227]  blk_update_request+0x224/0x380
[ 6973.787994]  blk_mq_end_request+0x1a/0x130
[ 6973.788739]  blk_complete_reqs+0x35/0x50
[ 6973.789456]  __do_softirq+0xd7/0x2c8
[ 6973.790114]  ? sort_range+0x20/0x20
[ 6973.790763]  run_ksoftirqd+0x2a/0x40
[ 6973.791400]  smpboot_thread_fn+0xb5/0x150
[ 6973.792114]  kthread+0x10b/0x130
[ 6973.792724]  ? set_kthread_struct+0x50/0x50
[ 6973.793491]  ret_from_fork+0x1f/0x40

This patch trys to use ->active_io to count acct_io. The regression
tests have passed. And I did takeover from raid0 to raid5 and from
raid5 to raid0 while I/O is happening. This patch works well.

Reported-by: Fine Fan <ffan@redhat.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/md.c | 6 ++++++
 drivers/md/md.h | 7 ++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index da6370835c47..7f06eb953488 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8627,12 +8627,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);
+
+	percpu_ref_put(&mddev->active_io);
 }
 
 /*
@@ -8648,10 +8651,13 @@ void md_account_bio(struct mddev *mddev, struct bio **bio)
 	if (!blk_queue_io_stat(bdev->bd_disk->queue))
 		return;
 
+	percpu_ref_get(&mddev->active_io);
+
 	clone = bio_alloc_clone(bdev, *bio, GFP_NOIO, &mddev->io_acct_set);
 	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;
 
 	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 6335cb86e52e..e148e3c83b0d 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -710,9 +710,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
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 1/1] md: Increase active_io to count acct_io

[Xiao Ni]

Hi Song

Is the patch ok? If so, are there chances to merge this into md-next this week?

Regards
Xiao

On Fri, Feb 3, 2023 at 1:16 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.
>
> [ 6973.767999] RIP: 0010:mempool_free+0x52/0x80
> [ 6973.786098] Call Trace:
> [ 6973.786549]  md_end_io_acct+0x31/0x40
> [ 6973.787227]  blk_update_request+0x224/0x380
> [ 6973.787994]  blk_mq_end_request+0x1a/0x130
> [ 6973.788739]  blk_complete_reqs+0x35/0x50
> [ 6973.789456]  __do_softirq+0xd7/0x2c8
> [ 6973.790114]  ? sort_range+0x20/0x20
> [ 6973.790763]  run_ksoftirqd+0x2a/0x40
> [ 6973.791400]  smpboot_thread_fn+0xb5/0x150
> [ 6973.792114]  kthread+0x10b/0x130
> [ 6973.792724]  ? set_kthread_struct+0x50/0x50
> [ 6973.793491]  ret_from_fork+0x1f/0x40
>
> This patch trys to use ->active_io to count acct_io. The regression
> tests have passed. And I did takeover from raid0 to raid5 and from
> raid5 to raid0 while I/O is happening. This patch works well.
>
> Reported-by: Fine Fan <ffan@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  drivers/md/md.c | 6 ++++++
>  drivers/md/md.h | 7 ++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index da6370835c47..7f06eb953488 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8627,12 +8627,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);
> +
> +       percpu_ref_put(&mddev->active_io);
>  }
>
>  /*
> @@ -8648,10 +8651,13 @@ void md_account_bio(struct mddev *mddev, struct bio **bio)
>         if (!blk_queue_io_stat(bdev->bd_disk->queue))
>                 return;
>
> +       percpu_ref_get(&mddev->active_io);
> +
>         clone = bio_alloc_clone(bdev, *bio, GFP_NOIO, &mddev->io_acct_set);
>         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;
>
>         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 6335cb86e52e..e148e3c83b0d 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -710,9 +710,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
> --
> 2.32.0 (Apple Git-132)
>

Re: [PATCH 1/1] md: Increase active_io to count acct_io

[Song Liu]

On Tue, Feb 7, 2023 at 5:49 AM Xiao Ni <xni@redhat.com> wrote:
>
> Hi Song
>
> Is the patch ok? If so, are there chances to merge this into md-next this week?
>

I rewrote the commit log and applied it to md-next. Please double check
the new commit log is accurate.

Thanks,
Song

Re: [PATCH 1/1] md: Increase active_io to count acct_io

[Xiao Ni]

On Wed, Feb 8, 2023 at 2:12 AM Song Liu <song@kernel.org> wrote:
>
> On Tue, Feb 7, 2023 at 5:49 AM Xiao Ni <xni@redhat.com> wrote:
> >
> > Hi Song
> >
> > Is the patch ok? If so, are there chances to merge this into md-next this week?
> >
>
> I rewrote the commit log and applied it to md-next. Please double check
> the new commit log is accurate.
>
> Thanks,
> Song
>

Thanks, the commit log is good.
Xiao