[PATCH] libfc: fix error handling in fc_exch_rrq_resp()

[PATCH] libfc: fix error handling in fc_exch_rrq_resp()

[Hillf Danton]

There seems no cleanup left when FC_EX_CLOSED encountered, which may
be from fc_exch_reset().

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/drivers/scsi/libfc/fc_exch.c	2010-09-13 07:07:38.000000000 +0800
+++ b/drivers/scsi/libfc/fc_exch.c	2010-10-22 23:17:50.000000000 +0800
@@ -1827,7 +1827,9 @@ static void fc_exch_rrq_resp(struct fc_s
 	if (IS_ERR(fp)) {
 		int err = PTR_ERR(fp);

-		if (err == -FC_EX_CLOSED || err == -FC_EX_TIMEOUT)
+		if (err == -FC_EX_CLOSED)
+			return;
+		if (err == -FC_EX_TIMEOUT)
 			goto cleanup;
 		FC_EXCH_DBG(aborted_ep, "Cannot process RRQ, "
 			    "frame error %d\n", err);

Re: [PATCH] libfc: fix error handling in fc_exch_rrq_resp()

[Joe Eykholt]

On 10/22/10 8:21 AM, Hillf Danton wrote:
> There seems no cleanup left when FC_EX_CLOSED encountered, which may
> be from fc_exch_reset().

Are you sure?  It seems that the RRQ exchange has a hold on another
exchange, the one that's the subject of the RRQ.  That exchange
still needs to be released.  I could be wrong, it's been a while.

> Signed-off-by: Hillf Danton <dhillf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> 
> --- a/drivers/scsi/libfc/fc_exch.c	2010-09-13 07:07:38.000000000 +0800
> +++ b/drivers/scsi/libfc/fc_exch.c	2010-10-22 23:17:50.000000000 +0800
> @@ -1827,7 +1827,9 @@ static void fc_exch_rrq_resp(struct fc_s
>  	if (IS_ERR(fp)) {
>  		int err = PTR_ERR(fp);
> 
> -		if (err == -FC_EX_CLOSED || err == -FC_EX_TIMEOUT)
> +		if (err == -FC_EX_CLOSED)
> +			return;
> +		if (err == -FC_EX_TIMEOUT)
>  			goto cleanup;
>  		FC_EXCH_DBG(aborted_ep, "Cannot process RRQ, "
>  			    "frame error %d\n", err);
> _______________________________________________
> devel mailing list
> devel-s9riP+hp16TNLxjTenLetw@public.gmane.org
> http://www.open-fcoe.org/mailman/listinfo/devel

Re: [Open-FCoE] [PATCH] libfc: fix error handling in fc_exch_rrq_resp()

[Hillf Danton]

On Sat, Oct 23, 2010 at 2:57 AM, Joe Eykholt <jeykholt@cisco.com> wrote:
>
>
> On 10/22/10 8:21 AM, Hillf Danton wrote:
>> There seems no cleanup left when FC_EX_CLOSED encountered, which may
>> be from fc_exch_reset().
>
> Are you sure?  It seems that the RRQ exchange has a hold on another
> exchange, the one that's the subject of the RRQ.  That exchange

In case of fc_exch_pool_reset(), each exch on ex_list is delivered to
fc_exch_reset(),  so "another" and "that" exch are treated in same way.

After cleanup, then the resp handler, if attached, is issued with FC_EX_CLOSED,
so no more work left for the resp handler.

If still needed, overwork occurs before calling resp handler.

> still needs to be released.  I could be wrong, it's been a while.
>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- a/drivers/scsi/libfc/fc_exch.c    2010-09-13 07:07:38.000000000 +0800
>> +++ b/drivers/scsi/libfc/fc_exch.c    2010-10-22 23:17:50.000000000 +0800
>> @@ -1827,7 +1827,9 @@ static void fc_exch_rrq_resp(struct fc_s
>>       if (IS_ERR(fp)) {
>>               int err = PTR_ERR(fp);
>>
>> -             if (err == -FC_EX_CLOSED || err == -FC_EX_TIMEOUT)
>> +             if (err == -FC_EX_CLOSED)
>> +                     return;
>> +             if (err == -FC_EX_TIMEOUT)
>>                       goto cleanup;
>>               FC_EXCH_DBG(aborted_ep, "Cannot process RRQ, "
>>                           "frame error %d\n", err);
>> _______________________________________________
>> devel mailing list
>> devel@open-fcoe.org
>> http://www.open-fcoe.org/mailman/listinfo/devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Open-FCoE] [PATCH] libfc: fix error handling in fc_exch_rrq_resp()

[Hillf Danton]

On Mon, Oct 25, 2010 at 2:20 PM, Joe Eykholt <jeykholt@cisco.com> wrote:
> <dropped scsi alias>
>
> On 10/23/10 12:56 AM, Hillf Danton wrote:
>>
>> On Sat, Oct 23, 2010 at 2:57 AM, Joe Eykholt<jeykholt@cisco.com>  wrote:
>>>
>>>
>>> On 10/22/10 8:21 AM, Hillf Danton wrote:
>>>>
>>>> There seems no cleanup left when FC_EX_CLOSED encountered, which may
>>>> be from fc_exch_reset().
>>>
>>> Are you sure?  It seems that the RRQ exchange has a hold on another
>>> exchange, the one that's the subject of the RRQ.  That exchange
>>
>> In case of fc_exch_pool_reset(), each exch on ex_list is delivered to
>> fc_exch_reset(),  so "another" and "that" exch are treated in same way.
>
> True, but nothing else is going to release the other exchange.

Here is snippet from fc_exch_reset(),

	if (ep->esb_stat & ESB_ST_REC_QUAL)
		atomic_dec(&ep->ex_refcnt);	/* drop hold for rec_qual */
	ep->esb_stat &= ~ESB_ST_REC_QUAL;

the hold does get dropped, based upon which it looks the cleanup in
fc_exch_reset() gets no more work left for response-handler.

>
>> After cleanup, then the resp handler, if attached, is issued with
>> FC_EX_CLOSED,
>> so no more work left for the resp handler.
>>
>> If still needed, overwork occurs before calling resp handler.
>
> This happens so infrequently that the amount of extra work, if any,
> is not significant.
>
>>> still needs to be released.  I could be wrong, it's been a while.
>>>
>>>> Signed-off-by: Hillf Danton<dhillf@gmail.com>
>>>> ---
>>>>
>>>> --- a/drivers/scsi/libfc/fc_exch.c    2010-09-13 07:07:38.000000000
>>>> +0800
>>>> +++ b/drivers/scsi/libfc/fc_exch.c    2010-10-22 23:17:50.000000000
>>>> +0800
>>>> @@ -1827,7 +1827,9 @@ static void fc_exch_rrq_resp(struct fc_s
>>>>       if (IS_ERR(fp)) {
>>>>               int err = PTR_ERR(fp);
>>>>
>>>> -             if (err == -FC_EX_CLOSED || err == -FC_EX_TIMEOUT)
>>>> +             if (err == -FC_EX_CLOSED)
>>>> +                     return;
>>>> +             if (err == -FC_EX_TIMEOUT)
>>>>                       goto cleanup;
>>>>               FC_EXCH_DBG(aborted_ep, "Cannot process RRQ, "
>>>>                           "frame error %d\n", err);
>>>> _______________________________________________
>>>> devel mailing list
>>>> devel@open-fcoe.org
>>>> http://www.open-fcoe.org/mailman/listinfo/devel
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html