[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-raid
Subject: Re: [PATCH 3/3] md/raid10: factor out a get_error_dev helper
From: Guoqing Jiang <guoqing.jiang () linux ! dev>
Date: 2021-10-19 8:00:30
Message-ID: 0c31ce7f-1ce9-60aa-ad61-a5320f61d49f () linux ! dev
[Download RAW message or body]
On 10/19/21 2:59 PM, Song Liu wrote:
> On Sun, Oct 17, 2021 at 6:50 AM Guoqing Jiang<guoqing.jiang@linux.dev> wrote:
>> Add a helper to find error_dev in case handle_read_err is true.
>>
>> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
> For 2/3 and 3/3, I was thinking about something like below (only
> compile tested).
> Would this work?
Thanks for clarification, I guess it works.
> Thanks,
> Song
>
> diff --git i/drivers/md/raid10.c w/drivers/md/raid10.c
> index dde98f65bd04f..c2387f55343dd 100644
> --- i/drivers/md/raid10.c
> +++ w/drivers/md/raid10.c
> @@ -1116,7 +1116,7 @@ static void regular_request_wait(struct mddev
> *mddev, struct r10conf *conf,
> }
>
> static void raid10_read_request(struct mddev *mddev, struct bio *bio,
> - struct r10bio *r10_bio)
> + struct r10bio *r10_bio, struct md_rdev
> *err_rdev)
> {
> struct r10conf *conf = mddev->private;
> struct bio *read_bio;
> @@ -1126,36 +1126,17 @@ static void raid10_read_request(struct mddev
> *mddev, struct bio *bio,
> struct md_rdev *rdev;
> char b[BDEVNAME_SIZE];
> int slot = r10_bio->read_slot;
> - struct md_rdev *err_rdev = NULL;
> gfp_t gfp = GFP_NOIO;
>
> - if (slot >= 0 && r10_bio->devs[slot].rdev) {
> - /*
> - * This is an error retry, but we cannot
> - * safely dereference the rdev in the r10_bio,
> - * we must use the one in conf.
> - * If it has already been disconnected (unlikely)
> - * we lose the device name in error messages.
> - */
> - int disk;
> - /*
> - * As we are blocking raid10, it is a little safer to
> - * use __GFP_HIGH.
> - */
> + /*
> + * As we are blocking raid10, it is a little safer to
> + * use __GFP_HIGH.
> + */
> + if (err_rdev) {
> gfp = GFP_NOIO | __GFP_HIGH;
> -
> - rcu_read_lock();
> - disk = r10_bio->devs[slot].devnum;
> - err_rdev = rcu_dereference(conf->mirrors[disk].rdev);
> - if (err_rdev)
> - bdevname(err_rdev->bdev, b);
> - else {
> - strcpy(b, "???");
> - /* This never gets dereferenced */
> - err_rdev = r10_bio->devs[slot].rdev;
> - }
> - rcu_read_unlock();
> - }
> + bdevname(err_rdev->bdev, b);
> + } else
> + strcpy(b, "???");
>
> regular_request_wait(mddev, conf, bio, r10_bio->sectors);
> rdev = read_balance(conf, r10_bio, &max_sectors);
> @@ -1519,7 +1500,7 @@ static void __make_request(struct mddev *mddev,
> struct bio *bio, int sectors)
> conf->geo.raid_disks);
>
> if (bio_data_dir(bio) == READ)
> - raid10_read_request(mddev, bio, r10_bio);
> + raid10_read_request(mddev, bio, r10_bio, NULL);
> else
> raid10_write_request(mddev, bio, r10_bio);
> }
> @@ -2887,6 +2868,31 @@ static int narrow_write_error(struct r10bio
> *r10_bio, int i)
> return ok;
> }
>
> +static struct md_rdev *get_error_dev(struct mddev *mddev, struct
> r10conf *conf, int slot,
> + struct r10bio *r10_bio)
> +{
> + struct md_rdev *err_rdev = NULL;
> +
We need the original check ("slot >= 0 && r10_bio->devs[slot].rdev ")
in the function I think. Will take a closer look and send a new version.
Thanks,
Guoqing
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic