Re: [PATCH 2/2] md: raid1/raid10: initialize bvec table via bio_add_page()

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 4449 bytes --]

On Wed, Jul 12 2017, Ming Lei wrote:

> We will support multipage bvec soon, so initialize bvec
> table using the standardy way instead of writing the
> talbe directly. Otherwise it won't work any more once
> multipage bvec is enabled.
>
> Acked-by: Guoqing Jiang <gqjiang@suse.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/md/md.c     | 21 +++++++++++++++++++++
>  drivers/md/md.h     |  3 +++
>  drivers/md/raid1.c  | 16 ++--------------
>  drivers/md/raid10.c |  4 ++--
>  4 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 8cdca0296749..cc8dcd928dde 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr)
>  }
>  EXPORT_SYMBOL(md_reload_sb);
>  
> +/* generally called after bio_reset() for reseting bvec */
> +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> +			       int size)
> +{
> +	int idx = 0;
> +
> +	/* initialize bvec table again */
> +	do {
> +		struct page *page = resync_fetch_page(rp, idx);
> +		int len = min_t(int, size, PAGE_SIZE);
> +
> +		/*
> +		 * won't fail because the vec table is big
> +		 * enough to hold all these pages
> +		 */
> +		bio_add_page(bio, page, len, 0);
> +		size -= len;
> +	} while (idx++ < RESYNC_PAGES && size > 0);
> +}
> +EXPORT_SYMBOL(md_bio_reset_resync_pages);

I really don't think this is a good idea.
This code is specific to raid1/raid10.  It is not generic md code.  So
it doesn't belong here.

If you want to remove code duplication, then work on moving all raid1
functionality into raid10.c, then discard raid1.c

Or at the very least, have a separate "raid1-10.c" file for the common
code.

NeilBrown

> +
>  #ifndef MODULE
>  
>  /*
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 2c780aa8d07f..efb32ce7a2f1 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp,
>  		return NULL;
>  	return rp->pages[idx];
>  }
> +
> +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
> +			       int size);
>  #endif /* _MD_MD_H */
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7901ddc3362f..5dc3fda2fdf7 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio)
>  	/* Fix variable parts of all bios */
>  	vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
>  	for (i = 0; i < conf->raid_disks * 2; i++) {
> -		int j;
> -		int size;
>  		blk_status_t status;
> -		struct bio_vec *bi;
>  		struct bio *b = r1_bio->bios[i];
>  		struct resync_pages *rp = get_resync_pages(b);
>  		if (b->bi_end_io != end_sync_read)
> @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio)
>  		status = b->bi_status;
>  		bio_reset(b);
>  		b->bi_status = status;
> -		b->bi_vcnt = vcnt;
> -		b->bi_iter.bi_size = r1_bio->sectors << 9;
>  		b->bi_iter.bi_sector = r1_bio->sector +
>  			conf->mirrors[i].rdev->data_offset;
>  		b->bi_bdev = conf->mirrors[i].rdev->bdev;
> @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio)
>  		rp->raid_bio = r1_bio;
>  		b->bi_private = rp;
>  
> -		size = b->bi_iter.bi_size;
> -		bio_for_each_segment_all(bi, b, j) {
> -			bi->bv_offset = 0;
> -			if (size > PAGE_SIZE)
> -				bi->bv_len = PAGE_SIZE;
> -			else
> -				bi->bv_len = size;
> -			size -= PAGE_SIZE;
> -		}
> +		/* initialize bvec table again */
> +		md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9);
>  	}
>  	for (primary = 0; primary < conf->raid_disks * 2; primary++)
>  		if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index e594ca610f27..cb8e803cd1c2 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
>  		rp = get_resync_pages(tbio);
>  		bio_reset(tbio);
>  
> -		tbio->bi_vcnt = vcnt;
> -		tbio->bi_iter.bi_size = fbio->bi_iter.bi_size;
> +		md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size);
> +
>  		rp->raid_bio = r10_bio;
>  		tbio->bi_private = rp;
>  		tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
> -- 
> 2.9.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]