Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Boaz Harrosh <bharrosh@panasas.com>]

On 05/19/2009 12:41 PM, Jens Axboe wrote:
> On Sun, May 17 2009, Boaz Harrosh wrote:
>> New block API:
>> given a struct bio allocates a new request. This is the parallel of
>> generic_make_request for BLOCK_PC commands users.
>>
>> The passed bio may be a chained-bio. The bio is bounced if needed
>> inside the call to this member.
>>
>> This is in the effort of un-exporting blk_rq_append_bio().
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> CC: Jeff Garzik <jeff@garzik.org>
>> ---
>>  block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/blkdev.h |    2 ++
>>  2 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index a2d97de..89261d2 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -891,6 +891,51 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
>>  EXPORT_SYMBOL(blk_get_request);
>>  
>>  /**
>> + * blk_make_request - given a bio, allocate a corresponding struct request.
>> + *
>> + * @bio:  The bio describing the memory mappings that will be submitted for IO.
>> + *        It may be a chained-bio properly constructed by block/bio layer.
>> + *
>> + * blk_make_request is the parallel of generic_make_request for BLOCK_PC
>> + * type commands. Where the struct request needs to be farther initialized by
>> + * the caller. It is passed a &struct bio, which describes the memory info of
>> + * the I/O transfer.
>> + *
>> + * The caller of blk_make_request must make sure that bi_io_vec
>> + * are set to describe the memory buffers. That bio_data_dir() will return
>> + * the needed direction of the request. (And all bio's in the passed bio-chain
>> + * are properly set accordingly)
>> + *
>> + * If called under none-sleepable conditions, mapped bio buffers must not
>> + * need bouncing, by calling the appropriate masked or flagged allocator,
>> + * suitable for the target device. Otherwise the call to blk_queue_bounce will
>> + * BUG.
>> + */
>> +struct request *blk_make_request(struct request_queue *q, struct bio *bio,
>> +				 gfp_t gfp_mask)
>> +{
>> +	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
>> +
>> +	if (unlikely(!rq))
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for_each_bio(bio) {
>> +		struct bio *bounce_bio = bio;
>> +		int ret;
>> +
>> +		blk_queue_bounce(q, &bounce_bio);
>> +		ret = blk_rq_append_bio(q, rq, bounce_bio);
>> +		if (unlikely(ret)) {
>> +			blk_put_request(rq);
>> +			return ERR_PTR(ret);
>> +		}
>> +	}
>> +
>> +	return rq;
>> +}
>> +EXPORT_SYMBOL(blk_make_request);
> 
> Generally the patchset looks good, my only worry is that interfaces like
> the above may be asking for trouble. To generate a chained list of
> bio's, you need to be careful with how you allocate them. In particular,
> you cannot use __GFP_WAIT for anything but the first bio in the chain.
> Otherwise you risk deadlocking waiting for a bio to be returned to the
> pool, which it never will since you haven't submitted it yet.
> 
> 

Thanks Jens, for your comment.

I have three sources of bio allocations.
1. bio_map_kern which uses bio_kmalloc (recently fixed by Tejun)
2. by osdblk which does a clone and will not ever wait.
   (I've fixed the code to split up the IO on allocation failure into
    smaller requests (will repost soon))
3. Future code in exofs and pNFS-Client that will only ever use bio_kmalloc.

Should we add something to the Documentation, and/or above doc_book comment
to warn off users?

Boaz