Re: [PATCH v2]: New implementation of scsi_execute_async()

[Boaz Harrosh <bharrosh@panasas.com>]

On 07/16/2009 09:15 PM, Vladislav Bolkhovitin wrote:
> Boaz Harrosh, on 07/15/2009 01:07 PM wrote:
>> On 07/14/2009 07:17 PM, Vladislav Bolkhovitin wrote:
<snip>
>>> +struct blk_kern_sg_hdr {
>>> +	struct scatterlist *orig_sgp;
>>> +	union {
>>> +		struct sg_table new_sg_table;
>>> +		struct scatterlist *saved_sg;
>>> +	};
>> There is a struct scatterlist * inside struct sg_table
>> just use that. No need for a union. (It's not like your saving
>> space). In the tail case nents == 1.
>> (orig_nents==0 and loose the tail_only)
> 
> This will uncover internal details of SG-chaining implementation, which 
> you wanted to hide. Or didn't?
> 

Are you commenting on the remove of tail_only, or the reuse of
->new_sg_table.sgl instead of ->saved_sg. The later makes tons of sense
to me.

if you want to keep the "tail_only" and not hack with nents && orig_nents
that's fine.

>>> +	while (hbio != NULL) {
>>> +		bio = hbio;
>>> +		hbio = hbio->bi_next;
>>> +		bio->bi_next = NULL;
>>> +
>>> +		blk_queue_bounce(q, &bio);
>> I do not understand. If you are bouncing on the bio level.
>> why do you need to do all the alignment checks and
>> sg-bouncing + tail handling all this is done again on the bio
>> level.
> 
> blk_queue_bounce() does another and completely independent level of 
> bouncing for pages allocated in the not accessible by the device area.
> 

No! you miss-understood. blk_queue_bounce() does all the bouncing,
high-mem, alignment, padding, draining, all of them.

The code you took from Tejun was meant to go at the bio level. Here
you are already taken care of.

All you need to do is:
	loop on all pages
		add_pc_page();
		when bio is full
			chain a new bio 
	and so on.

Then at the end call blk_make_request() which will do the bouncing and
the merging. No need for all the copy/tail_only etc.. this is done by
lower levels for you already.

If you do that then Tejun is right you should do an:
	bio_map_sg()
and not:
	blk_map_sg()

>> It looks to me that perhaps you did not understand Tejun's code
>> His code, as I remember, was on the bio level, but here you
>> are duplicating what is done in bio level.
>>
>> But maybe I don't understand. Tejun?
>>
>>> +	req->cmd_len = cmd_len;
>>> +	memset(req->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
>>> +	memcpy(req->cmd, cmd, req->cmd_len);
>> Does not support large commands.
> 
> Will be fixed.
> 
> (BTW, the SCSI layer assumes large CDBs as single big buffers, but all 
> SCSI transports I checked transfer large CDBs in 2 different parts: the 
> first 16 bytes and the rest. I.e. they deal with 2 different buffers. 
> So, the target side (SCST) deals with 2 buffers for large CDBs as well. 
> Having only one req->cmd will force SCST to merge those 2 buffers into a 
> single buffer. 
> So, scs[i,t]_execute_async() will have to make an 
> allocation for this as well.)
> 

Yep, allocation of command buffer if command_size > 16

>>> +/**
>>> + * sg_copy_elem - copy one SG element to another
>>> + * @dst_sg:	destination SG element
>>> + * @src_sg:	source SG element
>>> + * @copy_len:	maximum amount of data to copy. If 0, then copy all.
>>> + * @d_km_type:	kmap_atomic type for the destination SG
>>> + * @s_km_type:	kmap_atomic type for the source SG
>>> + *
>>> + * Description:
>>> + *    Data from the source SG element will be copied to the destination SG
>>> + *    element. Returns number of bytes copied. Can switch to the next dst_sg
>>> + *    element, so, to copy to strictly only one dst_sg element, it must be
>>> + *    either last in the chain, or copy_len == dst_sg->length.
>>> + */
>>> +int sg_copy_elem(struct scatterlist *dst_sg, struct scatterlist *src_sg,
>>> +		 size_t copy_len, enum km_type d_km_type,
>>> +		 enum km_type s_km_type)
>>> +{
>>> +	size_t dst_len = dst_sg->length, dst_offs = dst_sg->offset;
>>> +
>>> +	return __sg_copy_elem(&dst_sg, &dst_len, &dst_offs, src_sg,
>>> +		copy_len, d_km_type, s_km_type);
>>> +}
>>> +EXPORT_SYMBOL(sg_copy_elem);
>> Is not sg_copy_elem a copy of an sg with one element. Why do we need
>> two exports.
> 
> Perhaps, yes.
> 
>> I would pass a nents count to below and discard this one.
> 
> Perhaps, yes, but only for possible future use. I don't see any use of 
> it at the moment.
> 

Why? for example the use of not having another export.

>>> +
>>> +/**
>>> + * sg_copy - copy one SG vector to another
>>> + * @dst_sg:	destination SG
>>> + * @src_sg:	source SG
>>> + * @copy_len:	maximum amount of data to copy. If 0, then copy all.
>>> + * @d_km_type:	kmap_atomic type for the destination SG
>>> + * @s_km_type:	kmap_atomic type for the source SG
>>> + *
>>> + * Description:
>>> + *    Data from the source SG vector will be copied to the destination SG
>>> + *    vector. End of the vectors will be determined by sg_next() returning
>>> + *    NULL. Returns number of bytes copied.
>>> + */
>>> +int sg_copy(struct scatterlist *dst_sg,
>>> +	    struct scatterlist *src_sg, size_t copy_len,
>> Total length is nice, but also a nents count
>>
>>> +	    enum km_type d_km_type, enum km_type s_km_type)
>>> +{
>>> +	int res = 0;
>>> +	size_t dst_len, dst_offs;
>>> +
>>> +	if (copy_len == 0)
>>> +		copy_len = 0x7FFFFFFF; /* copy all */
>>> +
>>> +	dst_len = dst_sg->length;
>>> +	dst_offs = dst_sg->offset;
>>> +
>>> +	do {
>>> +		copy_len -= __sg_copy_elem(&dst_sg, &dst_len, &dst_offs,
>>> +				src_sg, copy_len, d_km_type, s_km_type);
>>> +		if ((copy_len == 0) || (dst_sg == NULL))
>>> +			goto out;
>>> +
>>> +		src_sg = sg_next(src_sg);
>>> +	} while (src_sg != NULL);
>>> +
>>> +out:
>>> +	return res;
>>> +}
>>> +EXPORT_SYMBOL(sg_copy);
> 
> Thanks,
> Vlad
>