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

Subsystems: software raid (multiple disks) support, the rest

STALE1820d

3 messages, 2 authors, 2021-08-18 · open the first message on its own page

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

From: Xiao Ni <hidden>
Date: 2021-08-18 01:36:59

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

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

From: Guoqing Jiang <hidden>
Date: 2021-08-18 01:44:03


On 8/18/21 9:36 AM, Xiao Ni wrote:
quoted hunk
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 <redacted>
---
  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.
quoted hunk
  	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 <redacted>

Thanks,
Guoqing

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

From: Xiao Ni <hidden>
Date: 2021-08-18 01:55:36

On Wed, Aug 18, 2021 at 9:44 AM Guoqing Jiang [off-list ref] wrote:


On 8/18/21 9:36 AM, Xiao Ni wrote:
quoted
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 <redacted>
---
  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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help