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

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 <redacted>
---
 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

On Sun, Aug 8, 2021 at 9:02 PM Xiao Ni [off-list ref] 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 <redacted>

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

Thanks,
Song

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 [off-list ref] wrote:

On Sun, Aug 8, 2021 at 9:02 PM Xiao Ni [off-list ref] wrote:

quoted

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 <redacted>

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

Thanks,
Song

quoted

---
 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

On Fri, Aug 13, 2021 at 6:34 PM Xiao Ni [off-list ref] 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

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

On Mon, Aug 16, 2021 at 1:23 PM Song Liu [off-list ref] wrote:

On Fri, Aug 13, 2021 at 6:34 PM Xiao Ni [off-list ref] wrote:

quoted

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