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
