* [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
@ 2021-08-09 4:01 Xiao Ni
2021-08-13 16:49 ` Song Liu
2021-08-16 6:35 ` Guoqing Jiang
0 siblings, 2 replies; 10+ messages in thread
From: Xiao Ni @ 2021-08-09 4:01 UTC (permalink / raw)
To: songliubraving; +Cc: ncroxon, linux-raid
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-09 4:01 [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect Xiao Ni
@ 2021-08-13 16:49 ` Song Liu
2021-08-14 1:34 ` Xiao Ni
2021-08-16 6:35 ` Guoqing Jiang
1 sibling, 1 reply; 10+ messages in thread
From: Song Liu @ 2021-08-13 16:49 UTC (permalink / raw)
To: Xiao Ni; +Cc: Song Liu, Nigel Croxon, linux-raid
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-13 16:49 ` Song Liu
@ 2021-08-14 1:34 ` Xiao Ni
2021-08-16 5:22 ` Song Liu
0 siblings, 1 reply; 10+ messages in thread
From: Xiao Ni @ 2021-08-14 1:34 UTC (permalink / raw)
To: Song Liu; +Cc: Song Liu, Nigel Croxon, linux-raid
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
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-14 1:34 ` Xiao Ni
@ 2021-08-16 5:22 ` Song Liu
2021-08-16 8:58 ` Xiao Ni
0 siblings, 1 reply; 10+ messages in thread
From: Song Liu @ 2021-08-16 5:22 UTC (permalink / raw)
To: Xiao Ni; +Cc: Song Liu, Nigel Croxon, linux-raid
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-16 5:22 ` Song Liu
@ 2021-08-16 8:58 ` Xiao Ni
0 siblings, 0 replies; 10+ messages in thread
From: Xiao Ni @ 2021-08-16 8:58 UTC (permalink / raw)
To: Song Liu; +Cc: Song Liu, Nigel Croxon, linux-raid
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-09 4:01 [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect Xiao Ni
2021-08-13 16:49 ` Song Liu
@ 2021-08-16 6:35 ` Guoqing Jiang
[not found] ` <CALTww2-YhNyKCyMjjviWJ4XmELNUZJonryrJfXtrwP4DU-C1zg@mail.gmail.com>
1 sibling, 1 reply; 10+ messages in thread
From: Guoqing Jiang @ 2021-08-16 6:35 UTC (permalink / raw)
To: Xiao Ni, songliubraving; +Cc: ncroxon, linux-raid
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
@ 2021-08-18 1:36 Xiao Ni
2021-08-18 1:43 ` Guoqing Jiang
0 siblings, 1 reply; 10+ messages in thread
From: Xiao Ni @ 2021-08-18 1:36 UTC (permalink / raw)
To: song; +Cc: ncroxon, linux-raid
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-18 1:36 Xiao Ni
@ 2021-08-18 1:43 ` Guoqing Jiang
2021-08-18 1:55 ` Xiao Ni
0 siblings, 1 reply; 10+ messages in thread
From: Guoqing Jiang @ 2021-08-18 1:43 UTC (permalink / raw)
To: Xiao Ni, song; +Cc: ncroxon, linux-raid
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect
2021-08-18 1:43 ` Guoqing Jiang
@ 2021-08-18 1:55 ` Xiao Ni
0 siblings, 0 replies; 10+ messages in thread
From: Xiao Ni @ 2021-08-18 1:55 UTC (permalink / raw)
To: Guoqing Jiang; +Cc: Song Liu, Nigel Croxon, linux-raid
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-08-18 1:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 4:01 [PATCH 1/1] md/raid10: Remove rcu_dereference when it doesn't need rcu lock to protect Xiao Ni
2021-08-13 16:49 ` Song Liu
2021-08-14 1:34 ` Xiao Ni
2021-08-16 5:22 ` Song Liu
2021-08-16 8:58 ` Xiao Ni
2021-08-16 6:35 ` Guoqing Jiang
[not found] ` <CALTww2-YhNyKCyMjjviWJ4XmELNUZJonryrJfXtrwP4DU-C1zg@mail.gmail.com>
2021-08-16 9:33 ` Guoqing Jiang
2021-08-18 1:36 Xiao Ni
2021-08-18 1:43 ` Guoqing Jiang
2021-08-18 1:55 ` Xiao Ni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.