>-----Original Message-----
>From: Mike Christie [mailto:[EMAIL PROTECTED]
>Sent: Sunday, August 10, 2008 6:51 PM
>To: Dev, Vasu
>Cc: [email protected]
>Subject: Re: [Open-FCoE] [RFC PATCH 1/2] libfc: removed checks for
>timer_pending and then updating exch ref count.
>
>Mike Christie wrote:
>> Mike Christie wrote:
>>> Dev, Vasu wrote:
>>>>> The locking and refcount use is really weird now though. Would it be
>>>>> cleaner or more standard to just say that you need to always grab the
>>>>> lock when accessing the ex_refcount instead of sometimes doing it and
>>>>> sometimes not (too confusing).
>>>> Currently exch ref count can be changed(inc/dec) without having need to
>hold
>>>> exch lock, but this can be revised as suggested above to always have
>exch
>>>> lock for changing exch ref. However as you mentioned in other email
>that
>>>> exch ref and locking is still nicer than fsp pkt locking and doesn't
>need
>>>> expensive del_timer_sync() as used in FCP.
>>> I am not sure what you mean by combining the two issues? For the FCP
>>> path we do not need the pkt lock for the actual release or hold of the
>>> refcount. We actually nornally never have the pkt lock when we do the
>>> final release, because fc_fcp_unlock_pkt drops the lock then drops the
>>> refcount.
>>>
>>> My comment about the del timer and refcounts is just about not needing
>>> to use the del_timer_sync with the exahnge code because the timer
>>> operation should always have a refcount.
>>>
>>> My comment about taking the lock around the exhange hold/release is just
>>> about what the locks and refcounts are protecting? I mean it seems like
>>> we are trying to do the two operations:
>>>
>>> exchange mp->exches lookup + hold
>>> and
>>> mp->exches clear + release
>>>
>>> atomically. We currently do both the lookup and hold under the
>>> mp->em_lock, but we do the release without the lock and take it only if
>>> the refcount is zero so we can clear the mp->exches array among other
>>> things. And this is racey because it seems like we want do drop the
>>> refcount and clear the mp->exches atomically.
>>>
>>> So with your patch we now drop the count without the lock, but if zero
>>> grab the lock to check the refcont again which seems weird because
>>> normally when a refcount goes to zero it means that we are going to free
>>> it. I mean if we were strict and we used krefs this would not be
>>> possible, and if you tried to send a patch for kref_put to do an extra
>>> check it is easier to imagine Christoph telling us whatever we are
>>> trying to do is crap over Greg happily taking it.
>>>
>>
>> After I thought this over the night, I think both solutions are equally
>> ugly :) so maybe there is something more fundamentally wrong with what
>> we are doing.
>>
>> I am trying to fix the abort handling and I think that interacts with
>> this. Right now we can go from fc_fcp_timeout to fc_timeout_error which
>> calls seq_exch_abort then calls fc_io_compl and that calls scsi_done to
>> return the command to the scsi layer. The problem with this is that
>> after we have sent the abort we do not know the status of it. If it is a
>> tape command scsi ml is not going to retry, but we really would like to
>> tell st.c what is up with the command (if it got aborted or not). And
>> for disk commands, if scsi-ml decides to kill it then we want to make
>> sure the device is still not writing out data, when it is returned to
>> the FS.
>>
>> Also for the fc_fcp_timeout->fc_timeout_error path, if the seq_ptr is
>> set we never call exch_done to clean up the response handler.
Abort handling in fc_exch.c will complete the exchange in this case but
I do see other issues along this as you also have described below.
>> fc_io_compl then does the release on the fsp from the initial allocation
>> so if we ever do get a response the ep->resp is going to be accessing a
>> freed (or possible reallocated to some other command) fsp.
>>
>> Or if we have sent the abort, and then fc_fcp_recv gets a response for
>> the scsi command that indicates it is done, then we call exch_done and
>> if fc_exch_release frees the ep before the abort response comes in
>> fc_exch.c does not get the abort response (which is not needed today
>> because fc_eh_abort's response code does not exist and always timesout)
>> but will be needed below), so any waiters are left timing out.
>> fc_fcp_complete could actually check for this too scenario.
>>
I see, double exch done is possible when fc_fcp.c is doing exch abort,
it is due to not having exclusive exch ref for abort and not always doing
fc_exch_done() since fc_exch_done() is called only if abort is not issued.
See my more comments on this along you suggested solution for this issue below.
>> So I think for the abort code we want to do a hold on the exchange if it
>> is sent, and fc_fcp.c wants to get a response. In fc_exch.c we want to
>> do a hold on the ep in exch_abort.
Extra exch hold/ref or always have upper call fc_exch_done, more
details below on this.
>> Then in the fc_fcp.c abort response
>> handler, we can do a exch_done for the abort to release the
>> seq_send_abort's hold. If the abort completely cleaned up the exchange
>> fc_fcp.c could do another exch_done to release the hold that was done
>> for the scsi command.
>>
Yeap, double exch done issue when abort is issued from fc_fcp.c.
>> exch_done would then need to be modified so that instead of
>> fc_exch_release doing
>>
>> spin_lock_bh(&mp->em_lock);
>> WARN_ON(mp->total_exches <= 0);
>> mp->total_exches--;
>> mp->exches[ep->xid - mp->min_xid] = NULL;
>> list_del(&ep->ex_list);
>> spin_unlock_bh(&mp->em_lock);
>>
>> exch_done would do that, then release its hold. And actually exch_done
>> would need to check that if there were multiple ep->resp functions (one
>> from fc_fcp_send_cmd and one from the abort) then we do not do the mp
>> manipulation above until the exch_done has been called for the last
>> ep->resp.
>>
I don't understand calling exch_done() twice from upper layer of fc_exch.c
and checking for multiple ep->resp functions to fix above described issue
along with extra exch ref for abort in fc_exch.c. How are you planning to
set these multiple exch resp functions and how that is going to change
fc_exch.c interface? How are you going to check for last fc_exch_done()
to do mp cleanup ?
I think, to fix this issue we should following things:-
1. Always call fc_exch_done() from upper layer, it can be either when
scsi command response is received or fc_exch.c abort/error handling is done.
2. Currently fc_exch.c doesn't call upper layer on completion of abort
handling and it completes exch implicitly by it self at end of abort, this
implicit exch completion at end of abort handling is not clean instead
fc_exch.c should be changed to call upper layer at end of abort, so that
upper layer can call fc_exch_done() to free the exchange. However fc_exch.c
abort handling can complete the exch if resp is null and that would be
for non fc_fcp.c upper layers and that is okay since in that case fc_exch.c
will issue the abort.
3. In think if we implement above two then additional exch ref for abort is
not required and fcp would ensure by it fsp state to call fc_exch_done()
only once to free the exch. However if you think for some corner case this
is still required then fc_exch.c should only increment and decrement that
exch ref for abort, increment on abort issue and decrement when abort
handling is done.
4. The fc_fcp_recv() can figure out ep->resp calling context (error/abort
v/s scsi resp) by looking into fc_frame pointer to make use of ex_resp_sync
of fc_exch_done() to ensure no dangling ep->resp since ex_resp_sync can be
set only if calling context is error handling path as I described in patch 2
description. I also suggested alternate approach of using callback to free
fsp pkt instead of ex_resp_sync with calling context restrictions.
What do you think between ex_resp_sync and call back approach of patch-2.
I was hoping for more comments on patch 2 since I was looking for
direction between ex_resp_sync and callback but some how patch-1
created more interest despite mainly only timer cleanup in patch-1 :-).
>> There are more corner cases like with all fun error handler, but I guess
>> the main idea is that we would be moving the mp manipulation to
>> exch_done instead of doing it in the release function. And exch_done
>> would do the release after the mp manipulation, so we would not hit that
>> race. >>The abort has a hold on the ep which is released by the abort
>> caller when it calls exch_done. fc_fcp.c gets its abort responses and
>> the fc_timeout_error path does not leave dangling ep->resps.
>>
Are you referring to using ex_resp_sync of patch 2 of this series
to ensure no more dangling ep->resp ?
>> What do you think?
>
>Oh yeah, my comments on this patch can be ignored for the merge of the
>patch since it is almost al timer clean ups in here. I will send a patch
>over your two patches.
Thanks for you're all comments and finding issues in error handling paths.
I'll get these patches in today, if callback is preferred over
ex_resp_sync then it can be implemented on top of patch-2.
Thanks
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
