| Message ID | 20230628015752.102267-3-linan666@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | raid10 bugfix | expand |
Hi, 在 2023/06/28 9:57, linan666@huaweicloud.com 写道: > From: Li Nan <linan122@huawei.com> > > Factor out a helper to get 'rdev' and 'replacement' from config->mirrors. > Just to make code cleaner and prepare to fix the bug of io loss while > 'replacement' replace 'rdev'. > > There is no functional change. > > Signed-off-by: Li Nan <linan122@huawei.com> > --- > drivers/md/raid10.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 3e6a09aaaba6..eaaf6307ddda 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -1346,6 +1346,26 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio, > } > } > > +static void get_rdev_repl_from_mirror(struct raid10_info *mirror, > + struct md_rdev **prdev, > + struct md_rdev **prrdev) I don't like this name, but I can live with this for now, related code will be removed eventually. Reviewed-by: Yu Kuai <yukuai3@huawei.com> > +{ > + struct md_rdev *rdev, *rrdev; > + > + rrdev = rcu_dereference(mirror->replacement); > + /* > + * Read replacement first to prevent reading both rdev and > + * replacement as NULL during replacement replace rdev. > + */ > + smp_mb(); > + rdev = rcu_dereference(mirror->rdev); > + if (rdev == rrdev) > + rrdev = NULL; > + > + *prrdev = rrdev; > + *prdev = rdev; > +} > + > static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio) > { > int i; > @@ -1489,15 +1509,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio, > int d = r10_bio->devs[i].devnum; > struct md_rdev *rdev, *rrdev; > > - rrdev = rcu_dereference(conf->mirrors[d].replacement); > - /* > - * Read replacement first to prevent reading both rdev and > - * replacement as NULL during replacement replace rdev. > - */ > - smp_mb(); > - rdev = rcu_dereference(conf->mirrors[d].rdev); > - if (rdev == rrdev) > - rrdev = NULL; > + get_rdev_repl_from_mirror(&conf->mirrors[d], &rdev, &rrdev); > if (rdev && (test_bit(Faulty, &rdev->flags))) > rdev = NULL; > if (rrdev && (test_bit(Faulty, &rrdev->flags))) >
On Tue, Jun 27, 2023 at 6:58 PM <linan666@huaweicloud.com> wrote: > [...] > > +static void get_rdev_repl_from_mirror(struct raid10_info *mirror, > + struct md_rdev **prdev, > + struct md_rdev **prrdev) > +{ > + struct md_rdev *rdev, *rrdev; > + > + rrdev = rcu_dereference(mirror->replacement); > + /* > + * Read replacement first to prevent reading both rdev and > + * replacement as NULL during replacement replace rdev. > + */ > + smp_mb(); > + rdev = rcu_dereference(mirror->rdev); > + if (rdev == rrdev) > + rrdev = NULL; > + > + *prrdev = rrdev; > + *prdev = rdev; I don't think the reduction in duplicated code justifies two output arguments. How about static struct md_rdev *dereference_rdev_and_rrdev(struct raid10_info *mirror, struct md_rdev **prrdev) { ... *prrdev = xxx; return rdev; } So we only have one argument for output. Also, "from_mirror" in the function name doesn't really add more value. Thanks, Song
在 2023/7/1 7:53, Song Liu 写道: > On Tue, Jun 27, 2023 at 6:58 PM <linan666@huaweicloud.com> wrote: >> > [...] > >> >> +static void get_rdev_repl_from_mirror(struct raid10_info *mirror, >> + struct md_rdev **prdev, >> + struct md_rdev **prrdev) >> +{ >> + struct md_rdev *rdev, *rrdev; >> + >> + rrdev = rcu_dereference(mirror->replacement); >> + /* >> + * Read replacement first to prevent reading both rdev and >> + * replacement as NULL during replacement replace rdev. >> + */ >> + smp_mb(); >> + rdev = rcu_dereference(mirror->rdev); >> + if (rdev == rrdev) >> + rrdev = NULL; >> + >> + *prrdev = rrdev; >> + *prdev = rdev; > > I don't think the reduction in duplicated code justifies two output arguments. > > How about > > static struct md_rdev *dereference_rdev_and_rrdev(struct raid10_info *mirror, > struct md_rdev **prrdev) > { > ... > *prrdev = xxx; > return rdev; > } > > So we only have one argument for output. > > Also, "from_mirror" in the function name doesn't really add more value. > > Thanks, > Song > . I agree. Let me improve this.
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 3e6a09aaaba6..eaaf6307ddda 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -1346,6 +1346,26 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio, } } +static void get_rdev_repl_from_mirror(struct raid10_info *mirror, + struct md_rdev **prdev, + struct md_rdev **prrdev) +{ + struct md_rdev *rdev, *rrdev; + + rrdev = rcu_dereference(mirror->replacement); + /* + * Read replacement first to prevent reading both rdev and + * replacement as NULL during replacement replace rdev. + */ + smp_mb(); + rdev = rcu_dereference(mirror->rdev); + if (rdev == rrdev) + rrdev = NULL; + + *prrdev = rrdev; + *prdev = rdev; +} + static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio) { int i; @@ -1489,15 +1509,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio, int d = r10_bio->devs[i].devnum; struct md_rdev *rdev, *rrdev; - rrdev = rcu_dereference(conf->mirrors[d].replacement); - /* - * Read replacement first to prevent reading both rdev and - * replacement as NULL during replacement replace rdev. - */ - smp_mb(); - rdev = rcu_dereference(conf->mirrors[d].rdev); - if (rdev == rrdev) - rrdev = NULL; + get_rdev_repl_from_mirror(&conf->mirrors[d], &rdev, &rrdev); if (rdev && (test_bit(Faulty, &rdev->flags))) rdev = NULL; if (rrdev && (test_bit(Faulty, &rrdev->flags)))