[PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Xiao Ni]

In the first loop of function raid10_handle_discard. It already
determines which disk need to handle discard request and add the
rdev reference count. So the conf->mirrors will not change until
all bios come back from underlayer disks. It doesn't need to use
rcu_dereference to get rdev.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/raid10.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 16977e8..cef9869 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1743,9 +1743,8 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
 	for (disk = 0; disk < geo->raid_disks; disk++) {
 		sector_t dev_start, dev_end;
 		struct bio *mbio, *rbio = NULL;
-		struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
-		struct md_rdev *rrdev = rcu_dereference(
-			conf->mirrors[disk].replacement);
+		struct md_rdev *rdev = conf->mirrors[disk].rdev;
+		struct md_rdev *rrdev = conf->mirrors[disk].replacement;
 
 		/*
 		 * Now start to calculate the start and end address for each disk.
-- 
2.7.5

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Song Liu]

On Sun, Aug 8, 2021 at 9:02 PM Xiao Ni <xni@redhat.com> wrote:
>
> In the first loop of function raid10_handle_discard. It already
> determines which disk need to handle discard request and add the
> rdev reference count. So the conf->mirrors will not change until
> all bios come back from underlayer disks. It doesn't need to use
> rcu_dereference to get rdev.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>

Will we get performance benefits from this change? If not, I would
prefer to keep the code as-is.

Thanks,
Song

> ---
>  drivers/md/raid10.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 16977e8..cef9869 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1743,9 +1743,8 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>         for (disk = 0; disk < geo->raid_disks; disk++) {
>                 sector_t dev_start, dev_end;
>                 struct bio *mbio, *rbio = NULL;
> -               struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
> -               struct md_rdev *rrdev = rcu_dereference(
> -                       conf->mirrors[disk].replacement);
> +               struct md_rdev *rdev = conf->mirrors[disk].rdev;
> +               struct md_rdev *rrdev = conf->mirrors[disk].replacement;
>
>                 /*
>                  * Now start to calculate the start and end address for each disk.
> --
> 2.7.5
>

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Xiao Ni]

Hi Song

It can improve the performance. It needs to add rcu lock when calling
rcu_dereference.
Now it has a bug. It doesn't use rcu lock to protect. In the second
loop, it doesn't need
to use rcu_dereference when getting rdev. So to resolve this bug, we can remove
rcu_dereference directly.

Best Regards
Xiao

On Sat, Aug 14, 2021 at 12:50 AM Song Liu <song@kernel.org> wrote:
>
> On Sun, Aug 8, 2021 at 9:02 PM Xiao Ni <xni@redhat.com> wrote:
> >
> > In the first loop of function raid10_handle_discard. It already
> > determines which disk need to handle discard request and add the
> > rdev reference count. So the conf->mirrors will not change until
> > all bios come back from underlayer disks. It doesn't need to use
> > rcu_dereference to get rdev.
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
>
> Will we get performance benefits from this change? If not, I would
> prefer to keep the code as-is.
>
> Thanks,
> Song
>
> > ---
> >  drivers/md/raid10.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> > index 16977e8..cef9869 100644
> > --- a/drivers/md/raid10.c
> > +++ b/drivers/md/raid10.c
> > @@ -1743,9 +1743,8 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> >         for (disk = 0; disk < geo->raid_disks; disk++) {
> >                 sector_t dev_start, dev_end;
> >                 struct bio *mbio, *rbio = NULL;
> > -               struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
> > -               struct md_rdev *rrdev = rcu_dereference(
> > -                       conf->mirrors[disk].replacement);
> > +               struct md_rdev *rdev = conf->mirrors[disk].rdev;
> > +               struct md_rdev *rrdev = conf->mirrors[disk].replacement;
> >
> >                 /*
> >                  * Now start to calculate the start and end address for each disk.
> > --
> > 2.7.5
> >
>

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Song Liu]

On Fri, Aug 13, 2021 at 6:34 PM Xiao Ni <xni@redhat.com> wrote:
>
> Hi Song
>
> It can improve the performance. It needs to add rcu lock when calling
> rcu_dereference.
> Now it has a bug. It doesn't use rcu lock to protect. In the second
> loop, it doesn't need
> to use rcu_dereference when getting rdev. So to resolve this bug, we can remove
> rcu_dereference directly.

In the second loop, we only use rdev and rrdev when bio and repl_bio
exists. So we shouldn't trigger the "bug" in any cases, right?

Please:
1) If you do think this is a bug, add a fix tag, so we can back port to stable.
   (while I still think it is not a real bug).
2) move struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev); to
  under "if (r10_bio->devs[disk].bio)"; and the rrdev ... to "if
(repl_bio)". And add
  a comment there so it is more clear in the code.

Thanks,
Song

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Guoqing Jiang]

On 8/9/21 12:01 PM, Xiao Ni wrote:
> In the first loop of function raid10_handle_discard. It already
> determines which disk need to handle discard request and add the
> rdev reference count. So the conf->mirrors will not change until
> all bios come back from underlayer disks. It doesn't need to use
> rcu_dereference to get rdev.

Can rdev be removed between the first loop and the second loop?

Thanks,
Guoqing

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Xiao Ni]

On Mon, Aug 16, 2021 at 1:23 PM Song Liu <song@kernel.org> wrote:
>
> On Fri, Aug 13, 2021 at 6:34 PM Xiao Ni <xni@redhat.com> wrote:
> >
> > Hi Song
> >
> > It can improve the performance. It needs to add rcu lock when calling
> > rcu_dereference.
> > Now it has a bug. It doesn't use rcu lock to protect. In the second
> > loop, it doesn't need
> > to use rcu_dereference when getting rdev. So to resolve this bug, we can remove
> > rcu_dereference directly.
>
> In the second loop, we only use rdev and rrdev when bio and repl_bio
> exists. So we shouldn't trigger the "bug" in any cases, right?

Hi Song

Sorry for not describing this problem clearly. It triggers a warning like this:

[  695.110751] =============================
[  695.131439] WARNING: suspicious RCU usage
[  695.151389] 4.18.0-319.el8.x86_64+debug #1 Not tainted
[  695.174413] -----------------------------
[  695.192603] drivers/md/raid10.c:1776 suspicious
rcu_dereference_check() usage!
[  695.225107]
               other info that might help us debug this:

[  695.260940]
               rcu_scheduler_active = 2, debug_locks = 1
[  695.290157] no locks held by mkfs.xfs/10186.

>
> Please:
> 1) If you do think this is a bug, add a fix tag, so we can back port to stable.
>    (while I still think it is not a real bug).
> 2) move struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev); to
>   under "if (r10_bio->devs[disk].bio)"; and the rrdev ... to "if
> (repl_bio)". And add
>   a comment there so it is more clear in the code.

ok, I'll fix these two places.

Regards
Xiao

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Guoqing Jiang]

Hi Xiao,

On 8/16/21 5:01 PM, Xiao Ni wrote:
> On Mon, Aug 16, 2021 at 2:36 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> Can rdev be removed between the first loop and the second loop?
>>
> Hi Guoqing
>
> As the patch's commit message says, it adds rdev->nr_pending already.
> So we can guarantee the rdev
> can't be removed during the first loop and the second loop.

I missed that, thanks for clarification. Maybe mention it in description 
that  nr_pending is incremented
which means rdev can't disappear.

Thanks,
Guoqing

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Xiao Ni]

On Wed, Aug 18, 2021 at 9:44 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 8/18/21 9:36 AM, Xiao Ni wrote:
> > One warning message is triggered like this:
> > [  695.110751] =============================
> > [  695.131439] WARNING: suspicious RCU usage
> > [  695.151389] 4.18.0-319.el8.x86_64+debug #1 Not tainted
> > [  695.174413] -----------------------------
> > [  695.192603] drivers/md/raid10.c:1776 suspicious
> > rcu_dereference_check() usage!
> > [  695.225107] other info that might help us debug this:
> > [  695.260940] rcu_scheduler_active = 2, debug_locks = 1
> > [  695.290157] no locks held by mkfs.xfs/10186.
> >
> > In the first loop of function raid10_handle_discard. It already
> > determines which disk need to handle discard request and add the
> > rdev reference count rdev->nr_pending. So the conf->mirrors will
> > not change until all bios come back from underlayer disks. It
> > doesn't need to use rcu_dereference to get rdev.
> >
> > Fixes: d30588b2731f ('md/raid10: improve raid10 discard request')
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >   drivers/md/raid10.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> > index 16977e8..1d3ac76 100644
> > --- a/drivers/md/raid10.c
> > +++ b/drivers/md/raid10.c
> > @@ -1712,6 +1712,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> >       } else
> >               r10_bio->master_bio = (struct bio *)first_r10bio;
> >
> > +     /* first select target devices under rcu_lock and
> > +      * inc refcount on their rdev.  Record them by setting
> > +      * bios[x] to bio
> > +      */
>
> Nit: the comment style is not correct.
Hi Guoqing

        /*
         * first select target devices under rcu_lock and
         * inc refcount on their rdev.  Record them by setting
         * bios[x] to bio
         */

It should be like this, right? If so, I'll send v2 and add your ack in
the patch too.

Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Guoqing Jiang]

On 8/18/21 9:36 AM, Xiao Ni wrote:
> One warning message is triggered like this:
> [  695.110751] =============================
> [  695.131439] WARNING: suspicious RCU usage
> [  695.151389] 4.18.0-319.el8.x86_64+debug #1 Not tainted
> [  695.174413] -----------------------------
> [  695.192603] drivers/md/raid10.c:1776 suspicious
> rcu_dereference_check() usage!
> [  695.225107] other info that might help us debug this:
> [  695.260940] rcu_scheduler_active = 2, debug_locks = 1
> [  695.290157] no locks held by mkfs.xfs/10186.
>
> In the first loop of function raid10_handle_discard. It already
> determines which disk need to handle discard request and add the
> rdev reference count rdev->nr_pending. So the conf->mirrors will
> not change until all bios come back from underlayer disks. It
> doesn't need to use rcu_dereference to get rdev.
>
> Fixes: d30588b2731f ('md/raid10: improve raid10 discard request')
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>   drivers/md/raid10.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 16977e8..1d3ac76 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1712,6 +1712,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>   	} else
>   		r10_bio->master_bio = (struct bio *)first_r10bio;
>   
> +	/* first select target devices under rcu_lock and
> +	 * inc refcount on their rdev.  Record them by setting
> +	 * bios[x] to bio
> +	 */

Nit: the comment style is not correct.

>   	rcu_read_lock();
>   	for (disk = 0; disk < geo->raid_disks; disk++) {
>   		struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
> @@ -1743,9 +1747,6 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>   	for (disk = 0; disk < geo->raid_disks; disk++) {
>   		sector_t dev_start, dev_end;
>   		struct bio *mbio, *rbio = NULL;
> -		struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
> -		struct md_rdev *rrdev = rcu_dereference(
> -			conf->mirrors[disk].replacement);
>   
>   		/*
>   		 * Now start to calculate the start and end address for each disk.
> @@ -1775,9 +1776,12 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>   
>   		/*
>   		 * It only handles discard bio which size is >= stripe size, so
> -		 * dev_end > dev_start all the time
> +		 * dev_end > dev_start all the time.
> +		 * It doesn't need to use rcu lock to get rdev here. We already
> +		 * add rdev->nr_pending in the first loop.
>   		 */
>   		if (r10_bio->devs[disk].bio) {
> +			struct md_rdev *rdev = conf->mirrors[disk].rdev;
>   			mbio = bio_clone_fast(bio, GFP_NOIO, &mddev->bio_set);
>   			mbio->bi_end_io = raid10_end_discard_request;
>   			mbio->bi_private = r10_bio;
> @@ -1790,6 +1794,7 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>   			bio_endio(mbio);
>   		}
>   		if (r10_bio->devs[disk].repl_bio) {
> +			struct md_rdev *rrdev = conf->mirrors[disk].replacement;
>   			rbio = bio_clone_fast(bio, GFP_NOIO, &mddev->bio_set);
>   			rbio->bi_end_io = raid10_end_discard_request;
>   			rbio->bi_private = r10_bio;

Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

Thanks,
Guoqing

[PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect

[Xiao Ni]

One warning message is triggered like this:
[  695.110751] =============================
[  695.131439] WARNING: suspicious RCU usage
[  695.151389] 4.18.0-319.el8.x86_64+debug #1 Not tainted
[  695.174413] -----------------------------
[  695.192603] drivers/md/raid10.c:1776 suspicious
rcu_dereference_check() usage!
[  695.225107] other info that might help us debug this:
[  695.260940] rcu_scheduler_active = 2, debug_locks = 1
[  695.290157] no locks held by mkfs.xfs/10186.

In the first loop of function raid10_handle_discard. It already
determines which disk need to handle discard request and add the
rdev reference count rdev->nr_pending. So the conf->mirrors will
not change until all bios come back from underlayer disks. It
doesn't need to use rcu_dereference to get rdev.

Fixes: d30588b2731f ('md/raid10: improve raid10 discard request')
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/raid10.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 16977e8..1d3ac76 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1712,6 +1712,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
 	} else
 		r10_bio->master_bio = (struct bio *)first_r10bio;
 
+	/* first select target devices under rcu_lock and
+	 * inc refcount on their rdev.  Record them by setting
+	 * bios[x] to bio
+	 */
 	rcu_read_lock();
 	for (disk = 0; disk < geo->raid_disks; disk++) {
 		struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
@@ -1743,9 +1747,6 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
 	for (disk = 0; disk < geo->raid_disks; disk++) {
 		sector_t dev_start, dev_end;
 		struct bio *mbio, *rbio = NULL;
-		struct md_rdev *rdev = rcu_dereference(conf->mirrors[disk].rdev);
-		struct md_rdev *rrdev = rcu_dereference(
-			conf->mirrors[disk].replacement);
 
 		/*
 		 * Now start to calculate the start and end address for each disk.
@@ -1775,9 +1776,12 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
 
 		/*
 		 * It only handles discard bio which size is >= stripe size, so
-		 * dev_end > dev_start all the time
+		 * dev_end > dev_start all the time.
+		 * It doesn't need to use rcu lock to get rdev here. We already
+		 * add rdev->nr_pending in the first loop.
 		 */
 		if (r10_bio->devs[disk].bio) {
+			struct md_rdev *rdev = conf->mirrors[disk].rdev;
 			mbio = bio_clone_fast(bio, GFP_NOIO, &mddev->bio_set);
 			mbio->bi_end_io = raid10_end_discard_request;
 			mbio->bi_private = r10_bio;
@@ -1790,6 +1794,7 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
 			bio_endio(mbio);
 		}
 		if (r10_bio->devs[disk].repl_bio) {
+			struct md_rdev *rrdev = conf->mirrors[disk].replacement;
 			rbio = bio_clone_fast(bio, GFP_NOIO, &mddev->bio_set);
 			rbio->bi_end_io = raid10_end_discard_request;
 			rbio->bi_private = r10_bio;
-- 
2.7.5