Re: [PATCH 13/17] blk-map: implement blk_rq_map_kern_sgl()

[Tejun Heo <tj@kernel.org>]

Hello,

Boaz Harrosh wrote:
>> -	do_copy = !blk_rq_aligned(q, kbuf, len) || object_is_on_stack(kbuf);
>> -	if (do_copy)
>> -		bio = bio_copy_kern(q, kbuf, len, gfp, rw);
>> -	else
>> -		bio = bio_map_kern(q, kbuf, len, gfp);
>> -
>> +	bio = bio_map_kern_sg(q, sgl, nents, rw, gfp);
>> +	if (IS_ERR(bio))
>> +		bio = bio_copy_kern_sg(q, sgl, nents, rw, gfp);
>>  	if (IS_ERR(bio))
>>  		return PTR_ERR(bio);
> 
> You might want to call bio_copy_kern_sg from within bio_map_kern_sg
> and remove yet another export from bio layer
> 
> Same thing with bio_map_user_iov/bio_copy_user_iov

Right, that makes sense.  Will incorporate it.

>>  
>> -	if (rq_data_dir(rq) == WRITE)
>> -		bio->bi_rw |= (1 << BIO_RW);
>> -
>> -	if (do_copy)
>> +	if (!bio_flagged(bio, BIO_USER_MAPPED))
>>  		rq->cmd_flags |= REQ_COPY_USER;
>>  
>> +	blk_queue_bounce(q, &bio);
>>  	blk_rq_bio_prep(q, rq, bio);
> 
> It could be nice to call blk_rq_append_bio() here
> and support multiple calls to this member.
> This will solve half of my problem with osd_initiator
> 
> Hmm .. but you wanted to drop multiple bio chaining
> perhaps you would reconsider.

I don't want to drop multiple bio chaining at all in itself.  I just
didn't see the current uses as, well, useful.  If building a sgl for a
request at once is difficult for your purpose, making blk_rq_map_*()
functions accumulate bios sounds like a good idea.  The primary goal
was to remove direct bio visibility/manipulation from low level
driver's POV.

>> +int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
>> +		    unsigned int len, gfp_t gfp)
>> +{
>> +	struct scatterlist sg;
>> +
>> +	sg_init_one(&sg, kbuf, len);
>> +
>> +	return blk_rq_map_kern_sg(q, rq, &sg, 1, gfp);
>> +}
>>  EXPORT_SYMBOL(blk_rq_map_kern);
> 
> could be inline like with the end_request functions

Yeap, will make it inline.

Thanks.

-- 
tejun