Dev, Vasu wrote:
>> -----Original Message-----
>> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On
>> Behalf Of [EMAIL PROTECTED]
>> Sent: Wednesday, July 30, 2008 1:44 AM
>> To: [email protected]
>> Subject: [Open-FCoE] [PATCH 1/1] libfc: remove shared tag map and fix timer
>> usage in fc_fcp.c (take 2)
>>
>> From: Mike Christie <[EMAIL PROTECTED]>
>>
>> This patch removes the shared tag map. It is only being used
>> to loop over running commands which is better suited with
>> a list of commands. This patch does not fix the issue with
>> the original outstanding array use and with the shared tag patch,
>> where the resp function can be running while the fc_io_compl
>> is freeing pkt.
>
>
> Nice catch, it has been like this for quite some time of calling
> exch resp handler while exch can be done/freed by FCP error handling.
> So there is race, however window of race is very small since FCP error
> handling works with fcp pkt lock held.
>
Yeah, it is small, but for some reason I can hit it very easily. About
half the time I run dd if=/dev/sdb of=/dev/null I will hit the problem.
If I run mutliple dds and have lots of IO in flight I will almost always
hit it.
My targets is very slow so it stresses the eh paths :)
> So one possible solution can be adding exch lock/unlock functions
> similar to recently added fc pkt lock/unlock functions by you and
> then call exch resp handler with exch lock held. This would also
> require ensuring other EM calls (e.g. fc_exch_done()) are not in
> call path to any circular exch locking.
Yeah, I was thinking that might get difficult if it is a spin lock with
the code how it is now. Another problem besides the circular locking is
that he exch resp handler may sleep. For example when we fc_io_compl
calls del_timer_sync. The sleeping problem could be fixed though.
I was tinkering with something like del_timer_sync where the fc_fcp.c
resp function would hold the pkt lock, set some state bit indicating it
is cleaning up the exchange, drop the pkt lock then call a fc_exch.c
function to clean up the exachnge. The fc_exch.c function would then not
return until the resp function was done running. For the fc_fcp resp
functions it would get run, grab the pkt lock then see the new state bit
indicating that it should just exit and not process the fsp. All this
seems maybe more complicated than what you propose though :(
>
>> This will be fixed in a different patch.
>>
>
> I can work on above suggested solution unless you are in the middle
> of this different patch.
Go ahead. I have just been timkering with different approaches and
learning the fc_exch.c code.
>
> As you mentioned that there were issues like this prior to shared
> tag map fix in libfc, therefore not sure on reverting shared tag map
> fix from libfc with added over head of maintaining link list of
I think we agree we need a way to loop over running commands for error
handling operations. Are you saying you want to keep the shared tag map
way of looking up commands? My issue with the shared tag map is if we
absolutely needed to go from some unique tag to some struct then a look
up table is great.
However, if there is any way we can not have to use that code I would
prefer it. Using it to just loop over the running commands is really
ugly and more difficult than it needs to be. We have to handle the cases
where the tag is mapped to a fc_fcp_pkt (normal case), the tag is
allocated but not yet mapped to a fc_fcp_pkt (in this case the scsi mid
layer is either cleaning up the command because we called scsi_done on
it, or scsi-ml has allocated a tag and associated it with a scsi_cmnd,
but has not yet called fc_queuecommand). We also have to worry about the
cases where the tag is reused and the scsi-ml eh is running, but I think
that should be handled by the first two cases (at least that is how I
worked it out) - I do not like having to worry about it though.
> outstanding commands in libfc. The BUG_ON in shared tag map is
> likely side effect of other pre-existing bugs like this.
>
>> This patch does fix several issues with the timer usage
>> in fc_fcp.c in the same code path I changed including:
>> - Use setup_timer and mod_timer.
>> - Don't call del_timer_sync with a spin_lock held.
>> - del_timer_sync does not handle timer functions that
>> rearm themselves. The user must add some synchrinization for this.
>> - The lun reset code could set a timer if the frame allocation fails, but
>> it was not deleting it.
>>
>
> The lun reset code timer on frame allocation fails is not fully fixed,
> See my additional comments below.
>
> <snip>
>
>> -static void fc_lun_reset_send(struct fc_fcp_pkt *fsp)
>> +static void fc_lun_reset_send(unsigned long data)
>> {
>> + struct fc_fcp_pkt *fsp = (struct fc_fcp_pkt *)data;
>> const size_t len = sizeof(fsp->cdb_cmd);
>> struct fc_lport *lp = fsp->lp;
>> struct fc_frame *fp;
>> @@ -1061,6 +1072,10 @@ static void fc_lun_reset_send(struct fc_fcp_pkt
>> *fsp)
>> struct fc_rport *rport;
>> struct fc_rport_libfc_priv *rp;
>>
>> + spin_lock(&fsp->scsi_pkt_lock);
>> + if (fsp->state & FC_SRB_COMPL)
>> + goto unlock;
>> +
>
> The fsp packet might be freed by this time though FC_SCSI_REC_TOV for
How will it get freed? I thought this fsp is only freed when we do this
in fc_device_reset:
sp->state = FC_SRB_FREE;
fc_fcp_pkt_free(sp);
If fc_lun_reset calls fc_lun_reset_send and the frame allocation always
fails so that timer is getting rearmed over and over, then the call to
wait_for_completion_timeout(&fsp->tm_done, FC_SCSI_TM_TOV)
would timeout and return but we do this:
spin_lock(&fsp->scsi_pkt_lock);
fsp->state |= FC_SRB_COMPL;
spin_unlock(&fsp->scsi_pkt_lock);
del_timer_sync(&fsp->timer);
the del_timer_sync call will wait for fc_lun_reset_send to complete
before it returns or kill the pending timer then return.
> fc_lun_reset_send is smaller than FC_SCSI_TM_TOV used by caller
> fc_lun_reset(), but fc_lun_reset_send() may get called multiple times on
> each frame alloc failure. So may be fc_lun_reset_send() should keep retry
> count not exceeding FC_SCSI_TM_TOV.
>
>> fp = fc_frame_alloc(lp, len);
>> if (!fp)
>> goto retry;
>> @@ -1078,15 +1093,17 @@ static void fc_lun_reset_send(struct fc_fcp_pkt
>> *fsp)
>>
>> if (sp) {
>> fsp->seq_ptr = sp;
>> - return;
>> + goto unlock;
>> }
>> /*
>> * Exchange or frame allocation failed. Set timer and retry.
>> */
>> fc_frame_free(fp);
>> retry:
>> - fsp->timer.function = (void (*)(unsigned long))fc_lun_reset_send;
>> + setup_timer(&fsp->timer, fc_lun_reset_send, (unsigned long)fsp);
>> fc_fcp_timer_set(fsp, FC_SCSI_REC_TOV);
>> +unlock:
>> + spin_unlock(&fsp->scsi_pkt_lock);
>> }
>
>
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
