Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh <bharrosh@panasas.com>]

Boaz Harrosh wrote:
> Boaz Harrosh wrote:
>> Seokmann Ju wrote:
>>> On Oct 28, 2008, at 12:57 AM, Boaz Harrosh wrote:
>>>
>>>> Seokmann Ju wrote:
>>>>> On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:
>>>>>
>>>>>> FUJITA Tomonori wrote:
>>>>>>> On Sun, 26 Oct 2008 11:38:04 +0200
>>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>>>>
>>>>>>>> FUJITA Tomonori wrote:
>>>>>>>>> CC'ed Jens,
>>>>>>>> I think that all block-queue consumers should call one of
>>>>>>>> blk_end_request(),
>>>>>>> This is kinda what I suggested in the previous mail but as I wrote,
>>>>>>> some of them don't now.
>>>>>>>
>>>>>> I think they should, specially if they're going to use the timer.
>>>>>> The way I see it they must. It's kind of a block layer API thing.
>>>>>> Someone calls blk_execute_xx then eventually someone needs to call
>>>>>> blk_end_request. You could call it from bsg but only temporary until
>>>>>> all are fixed. (because you will need an ugly check to see if  
>>>>>> request
>>>>>> was not already ended)
>>>>> I made following changes but, it seems not helpful for the issue.
>>>>> It, eventually, got failed to call blk_delete_timer() as ~/block/blk-
>>>>> core.c:__end_that_request_first() returns non-zero.
>>>>> Inside of the __end_that_reqeust_first(), it detected 'nbytes' is
>>>>> bigger than 'nr_bytes' in case of bidi (where req->next_rq is not  
>>>>> NULL).
>>>>> I'm not sure whether we need to have chains of function calls
>>>>> initiated by the blk_end_request() or blk_end_bidi_request().
>>>>> Would it create any problems if we directly call  
>>>>> 'blk_delete_timer()'?
>>>>>
>>>> Dear Seokmann. You miss understud me. What I'm saying is that you must
>>>> call blk_end_bidi_request at the FC end, just after you have finished
>>>> to consume the request, and before you return it upstream. it can be
>>>> some thing like:
>>>>
>>>> +	blk_end_bidi_request(rq, 0, blk_rq_bytes(rq),
>>>> +	                     rq->next_rq ?  blk_rq_bytes(rq->next_rq) : 0);
>>>>
>>>> In this case __end_that_reqeust_first should never return non-zero.
>>> Hello Boaz,
>>> Thank you for the clarification.
>>> I made the changes accordingly and tested it, but the problem is still
>>> there - same result of getting non-zero returns from
>>> __end_that_request_first().
>>> I guess that, either, I still get confused about the location or, there
>>> is something else going on...
>>>
>>> Sorry, I don't have public git-web.
>>> Here is snaptshot of the FC transport layer changes.
>>> The fc_service_done() is the callback that the FC transport layer
>>> provides. And that is the callback called by LLD before returning.
>>>
>>> Please let me know for any comments.
>>>
>>> Thank you,
>>> Seokmann
>> if the attached file is the code you tested then it is wrong look here:
>>
>>> +
>>> +	if (service->srv_reply.residual) {
>>> +		service->req->data_len = 0;
>>> +		service->req->next_rq->data_len = service->srv_reply.residual;
>>> +	} else {
>>> +		service->req->data_len = 0;
>>> +		service->req->next_rq->data_len = 0;
>>> +	}
>>> +
>> Move above to after the blk_end_bidi_request call
>>
>>> +	blk_end_bidi_request(service->req, 0, blk_rq_bytes(service->req),
>>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
>> You must call blk_end_bidi_request before you change service->req->data_len
>> to hold the residual (or 0). Otherwise you damage the request.
>>
>>> +	service->req->end_io(service->req, 0);
> 
> Hmm, on re inspection req->end_io(...) called here has the same problem.
> Are you sure it's needed?
> 

No you do not call req->end_io(..) directly. It eventual gets called
by blk_end_bidi_request() inside end_that_request_last(). (Once
all byte are completed)

<snip>

Boaz