Re: [PATCH] raid1: ensure bio doesn't have more than BIO_MAX_VECS sectors

[Ming Lei <ming.lei@redhat.com>]

On Sat, Aug 14, 2021 at 08:55:21AM +0100, Christoph Hellwig wrote:
> On Fri, Aug 13, 2021 at 04:38:59PM +0800, Guoqing Jiang wrote:
> > 
> > Ok, thanks.
> > 
> > > In general the size of a bio only depends on the number of vectors, not
> > > the total I/O size.  But alloc_behind_master_bio allocates new backing
> > > pages using order 0 allocations, so in this exceptional case the total
> > > size oes actually matter.
> > > 
> > > While we're at it: this huge memory allocation looks really deadlock
> > > prone.
> > 
> > Hmm, let me think more about it, or could you share your thought? ????
> 
> Well, you'd need a mempool which can fit the max payload of a bio,
> that is BIO_MAX_VECS pages.
> 
> FYI, this is what I'd do instead of this patch for now.  We don't really
> need a vetor per sector, just per page.  So this limits the I/O
> size a little less.
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 3c44c4bb40fc..5b27d995302e 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1454,6 +1454,15 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  		goto retry_write;
>  	}
>  
> +	/*
> +	 * When using a bitmap, we may call alloc_behind_master_bio below.
> +	 * alloc_behind_master_bio allocates a copy of the data payload a page
> +	 * at a time and thus needs a new bio that can fit the whole payload
> +	 * this bio in page sized chunks.
> +	 */
> +	if (bitmap)
> +		max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SIZE);

s/PAGE_SIZE/PAGE_SECTORS

> +
>  	if (max_sectors < bio_sectors(bio)) {
>  		struct bio *split = bio_split(bio, max_sectors,
>  					      GFP_NOIO, &conf->bio_split);
> 

Here the limit is max single-page vectors, and the above way may not work,
such as:

0 ~ 254: each bvec's length is 512
255: bvec's length is 8192

the total length is just 512*255 + 8192 = 138752 bytes = 271 sectors, but it
still may need 257 bvecs, which can't be allocated via bio_alloc_bioset().

One solution is to add queue limit of max_single_page_bvec, and let
blk_queue_split() handle it.



Thanks,
Ming