Re: [PATCH 2/2] md: Add support for RAID-1 consistency check feature

[NeilBrown]

On Mon, 17 Mar 2014 16:00:05 +0100 Ralph Mueck <linux-kernel@xxxxxxxxx> wrote:

> This patch introduces a consistency check feature for level-1 RAID
> arrays that have been created with the md driver.
> When enabled, every read request is duplicated and initiated for each
> member of the RAID array. All read blocks are compared with their
> corresponding blocks on the other array members. If the check fails for
> a block, the block is not handed over, but an error code is returned
> instead.
> As mentioned in the cover letter, the implementation still has some 
> unresolved issues.
> 
> Signed-off-by: Ralph Mueck <linux-kernel@xxxxxxxxx>
> Signed-off-by: Matthias Oefelein <ma.oefelein@xxxxxxxx>
> 
> ---
>  drivers/md/raid1.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 250 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a6ca1c..8c64f9a 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -37,6 +37,7 @@
>  #include <linux/module.h>
>  #include <linux/seq_file.h>
>  #include <linux/ratelimit.h>
> +#include <linux/gfp.h>
>  #include "md.h"
>  #include "raid1.h"
>  #include "bitmap.h"
> @@ -257,6 +258,109 @@ static void call_bio_endio(struct r1bio *r1_bio)
>  	}
>  }
>  
> +/* The safe_read version of the raid_end_bio_io() function */
> +/* On a read request, we issue requests to all available disks.
> + * Data is returned only if all discs contain the same data
> + */
> +static void _endio(struct r1bio *r1_bio)
> +{
> +	struct bio *bio = r1_bio->master_bio;
> +	int done;
> +	struct r1conf *conf = r1_bio->mddev->private;
> +	sector_t start_next_window = r1_bio->start_next_window;
> +	sector_t bi_sector = bio->bi_iter.bi_sector;
> +	int disk;
> +	struct md_rdev *rdev;
> +	int i;
> +	struct page *dragptr = NULL;
> +	int already_copied = 0;	/* we want to copy the data only once */
> +
> +	for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
> +		struct bio *p = NULL;
> +		struct bio *s = NULL;
> +		
> +		rcu_read_lock();
> +		rdev = rcu_dereference(conf->mirrors[disk].rdev);
> +		rcu_read_unlock();

You cannot drop rcu_read_lock until you take a reference to rdev, or stop
using it.


> +
> +		if (r1_bio->bios[disk] == IO_BLOCKED
> +			|| rdev == NULL
> +			|| test_bit(Unmerged, &rdev->flags)
> +			|| test_bit(Faulty, &rdev->flags)) {
> +			continue;
> +		}
> +
> +		/* bio_for_each_segment is broken. at least here.. */
> +		/* iterate over linked bios */
> +		for (p = r1_bio->master_bio, s = r1_bio->bios[disk];
> +		     (p != NULL) && (s != NULL);
> +		     p = p->bi_next, s = s->bi_next) {
> +			/* compare the pages read */
> +			for (i = 0; i < r1_bio->bios[disk]->bi_vcnt; i++) {
> +				if (dragptr) { /* dragptr points to the previous page */
> +					if(memcmp(page_address(r1_bio->bios[disk]->bi_io_vec[0].bv_page),
> +						page_address(dragptr),
> +						(r1_bio->bios[disk]->bi_io_vec[0].bv_len))) {
> +						set_bit(R1BIO_ReadError, &r1_bio->state);
> +						clear_bit(R1BIO_Uptodate, &r1_bio->state);
> +					}
> +				}
> +				dragptr = r1_bio->bios[disk]->bi_io_vec[0].bv_page;
> +			}

This doesn't make any sense to me at all.  You use 'i' to loop bi_vnt times,
but never use 'i' or change any other variable in that loop (except dragptr
which is always set to the same value).

And you use "bi_next", but next set up any linkage through bi_next.

Confused.

NeilBrown

Attachment: signature.asc
Description: PGP signature