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

[Guoqing Jiang <guoqing.jiang@linux.dev>]

Hi Ming and Christoph,

On 8/14/21 4:57 PM, Ming Lei wrote:
> 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.

IIUC, the behind bio is allocated from bio_set (mddev->bio_set) which is 
allocated in md_run by
call bioset_init, so the mempool (bvec_pool) of  this bio_set is created 
by biovec_init_pool which
uses global biovec slabs. Do we really need another mempool? Or, there 
is no potential deadlock
  for this case.

>> 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.
>> +	 */

Thanks for the above, will copy it accordingly. I will check if 
WriteMostly is set before, then check both
the flag and bitmap.

>> +	if (bitmap)
>> +		max_sectors = min_t(int, max_sectors, BIO_MAX_VECS * PAGE_SIZE);
> s/PAGE_SIZE/PAGE_SECTORS

Agree.

>> +
>>   	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:ust splitted and not
>
> 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().

Thanks for deeper looking! I guess it is because how vcnt is calculated.

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

The path (blk_queue_split -> blk_bio_segment_split -> bvec_split_segs) 
which respects max_segments
of limit. Do you mean introduce max_single_page_bvec to limit? Then 
perform similar checking as for
  max_segment.

Thanks,
Guoqing