On Tue, Nov 06 2018, Jack Wang wrote:
> From: Florian-Ewald Mueller <florian-ewald.mueller@xxxxxxxxxxxxxxx>
>
> - the bitmap only use in the first_clone clause for raid1_write_request.
> - possible we have stale data in bitmap pointer.
>
> Signed-off-by: Florian-Ewald Mueller <florian-ewald.mueller@xxxxxxxxxxxxxxx>
> Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxx>
> ---
> drivers/md/raid1.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 1d54109071cc..727cd5759c83 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1191,7 +1191,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> struct r1conf *conf = mddev->private;
> struct raid1_info *mirror;
> struct bio *read_bio;
> - struct bitmap *bitmap = mddev->bitmap;
> + struct bitmap *bitmap;
> const int op = bio_op(bio);
> const unsigned long do_sync = (bio->bi_opf & REQ_SYNC);
> int max_sectors;
> @@ -1254,7 +1254,9 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> mdname(mddev),
> (unsigned long long)r1_bio->sector,
> bdevname(mirror->rdev->bdev, b));
> -
> + /* get mddev->bitmap behind wait_barrier()
> + * as it could have been removed before */
> + bitmap = mddev->bitmap;
This is unnecessary. mddev_suspend() is called before mddev->bitmap is
changed, and mddev_suspend() cannot be called at this point as
md_handle_request() has incremented ->active_io for us.
> if (test_bit(WriteMostly, &mirror->rdev->flags) &&
> bitmap) {
> /*
> @@ -1305,7 +1307,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
> struct r1conf *conf = mddev->private;
> struct r1bio *r1_bio;
> int i, disks;
> - struct bitmap *bitmap = mddev->bitmap;
> unsigned long flags;
> struct md_rdev *blocked_rdev;
> struct blk_plug_cb *cb;
> @@ -1459,6 +1460,10 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>
>
> if (first_clone) {
> + /* get mddev->bitmap behind wait_barrier()
> + * as it could have been removed before */
> + struct bitmap *bitmap = mddev->bitmap;
> +
Again, not necessary.
The code change isn't harmful, but the comment is as it is wrong.
NeilBrown
> /* do behind I/O ?
> * Not if there are too many, or cannot
> * allocate memory, or a reader on WriteMostly
> --
> 2.7.4
Attachment:
signature.asc
Description: PGP signature
