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

[Boaz Harrosh <bharrosh@panasas.com>]

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,
>>>>>
>>>>> On Wed, 22 Oct 2008 19:27:35 -0700
>>>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>>>
>>>>>> And it seems like that the panic is happening due to the fact that
>>>>>> blk_delete_timer() is not called upon having completion of the  
>>>>>> service.
>>>>>> In other words, the block layer calls blk_add_timer() prior to
>>>>>> dispatch the service but, it doesn't call blk_delete_timer()  
>>>>>> when it
>>>>>> returned.
>>>>> Yeah, we need to call blk_delete_timer somewhere.
>>>>>
>>>>>
>>>>>> Just for heck of it, I've tried out by adding blk_delete_timer()  
>>>>>> in
>>>>>> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the  
>>>>>> problem.
>>>>> I think blk_end_sync_rq() is not the good place. From the  
>>>>> perspective
>>>>> of bsg, we need to handle both blk_execute_rq_nowait and
>>>>> blk_execute_rq.
>>>>>
>>>>>
>>>>>> Seems like that there are APIs in the block layer that are call  
>>>>>> the
>>>>>> blk_delete_timer(), including,
>>>>>> - blk_end_io()
>>>>>> - __blk_end_request()
>>>>>>
>>>>>> Could you guide me what is right way to fix the problem?
>>>>> Exporting blk_delete_timer is one option, but it doesn't look very
>>>>> nice (since the block layer doesn't export any details about its  
>>>>> timer
>>>>> infrastructure), I think. Modifying blk_end_io() to make it  
>>>>> usable for
>>>>> requests via something like bsg might be better.
>>>>>
>>>>> Anyway, we need to ask Jens.
>>>>>
>>>>> Jens, fc people have working on fc pass through support via bsg,  
>>>>> which
>>>>> hooks bsg's request queue on fc transport objects (We did the  
>>>>> similar
>>>>> thing for sas transport).
>>>>>
>>>>> We want the timeout feature for fc pass through and I think that  
>>>>> it's
>>>>> nice to use the block layer timeout feature for it. But the users  
>>>>> of
>>>>> bsg request queue don't need (or call) APIs such as
>>>>> end_that_request_last to call blk_delete_timer internally. How  
>>>>> should
>>>>> these users call blk_delete_timer?
>>>> TOMO Hi
>>>> If a command is queued by bsg to a scsi device, which is posible.  
>>>> Then
>>>> blk_end_request() is called by scsi-ml. So it does work.
>>> It doesn't work for bsg's scsi transport pass through stuff such as
>>> SMP (sas management protocol, we already support) and FC. Virtually,
>>> they don't use scsi-ml.
>>>
>> Right, I know that, that's why I say.
>>
>>>> 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.

> Seokmann
> ---
> diff -Naurp a/blk-exec.c b/blk-exec.c
> --- a/blk-exec.c	2008-10-27 09:33:14.000000000 -0700
> +++ b/blk-exec.c	2008-10-27 09:34:08.000000000 -0700
> @@ -22,6 +22,13 @@ static void blk_end_sync_rq(struct reque
>   {
>   	struct completion *waiting = rq->end_io_data;
> 
> +	if (rq->next_rq) {
> +		blk_end_bidi_request(rq, error, rq->data_len,
> +		    rq->next_rq->data_len);
> +		// blk_end_request(rq);
> +		// blk_delete_timer(rq);
> +	}
> +
>   	rq->end_io_data = NULL;
>   	__blk_put_request(rq->q, rq);
> ---

Please don't change blk_end_sync_rq at blk-exec.c it is too
delicate here. At this stage rq->data_len is already holding
the residual count in most cases, and touching it would
be a bug

>>
>>>> there are lots to choose from. We don't need
>>>> a new API. It will work with or without data, and it does what
>>>> you want.
>> Boaz
> 

Do you have this code on a public git-web somewhere? I need
to look at the complete code, if you need that I advise you
where in the FC code to call blk_end_bidi_request()

Thanks
Boaz