Re: [PATCH 10/42] target: Rewrite transport_init_task_sg()

[Andy Grover <agrover@redhat.com>]

On 05/31/2011 12:08 AM, Nicholas A. Bellinger wrote:
> On Fri, 2011-05-27 at 12:06 -0700, Andy Grover wrote:
>> Don't set task->task_sg_num until task_sg is successfully alloced, to
>> prevent it from possibly being out of sync.
>>
>> Simplify loop counting sgs needed. Handle possible (?) case of offset
>> greater than se_mem->se_len. Remove some cluttering debugprints. Reduce
>> local variable usage.
>>
>> Do not set termination bit in padded-1 entry in the padded_sg case. If
>> something barfs on an extra zeroed sg, (will happen if chain-capable sgs
>> don't actually get chained) then it's broken.
>>
>> Remove unneeded parens.
>>
>> Signed-off-by: Andy Grover <agrover@redhat.com>
>> ---
> 
> So I am not exactly sure there is an issue here with task->task_sg_num
> assignment in the failure case, as nothing depends AFAICT upon
> task->task_sg_num in the release path before an possible
> transport_init_task_sg() failure..

Yeah I was just being paranoid..

> However, my main concern is the removal of sg_mark_end() calls  to
> signify the end of task->task_sg[] memory for individual task I/O
> dispatch.  As you have noticed this looks OK with existing code wrt to
> backend *_map_task_sg() and *_do_task() using for_each_sg() w/
> task->task_sg_num, but am wondering if it really makes sense to remove
> this for the actual scatterlist dispatch for all backends..?  Is there
> any possible code below TCM backend drivers where HW drivers can pose an
> issue..?
> 
> I am happy to merge this for tcm v4.1 along with patch #11 if this is
> really safe to do, but will defer for the moment until we can get a few
> more ACKs on these two..

Every sg[] that is initialized by sg_init_table (or sg_init_one) should
have sg_mark_end called on its last element[1] so I think we're ok, or
am I misunderstanding the issue you're getting at?

Regards -- Andy

[1] http://lxr.linux.no/linux+v2.6.38/lib/scatterlist.c#L85