Hi Mike,

Mike Christie wrote:
> On 07/27/2009 06:22 AM, Hannes Reinecke wrote:
>> The network core will call state_change() callback
>> prior to the data_ready() callback, which might cause
>> us to lose a connection state change.
>> So we have to evaluate the socket state at the end
>> of the data_ready() callback, too.
>>
>> Signed-off-by: Hannes Reinecke<h...@suse.de>
>> ---
>>   drivers/scsi/iscsi_tcp.c |   13 +++++++++++++
>>   1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
>> index f02ddf8..7ca52b3 100644
>> --- a/drivers/scsi/iscsi_tcp.c
>> +++ b/drivers/scsi/iscsi_tcp.c
>> @@ -117,6 +117,19 @@ static void iscsi_sw_tcp_data_ready(struct sock
>> *sk, int flag)
>>       rd_desc.count = 1;
>>       tcp_read_sock(sk,&rd_desc, iscsi_sw_tcp_recv);
>>
>> +    /*
>> +     * We might have missed a dropped connection
>> +     * from state_change() callback, evaluate now.
>> +     */
>> +    if ((sk->sk_state == TCP_CLOSE_WAIT ||
>> +         sk->sk_state == TCP_CLOSE)&&
> 
> Need a space before the &&.
> 
>> +        !atomic_read(&sk->sk_rmem_alloc)) {
> 
> 
> Did you copy this from the iscsi_sw_tcp_state_change check or from
> somewhere else? Do you know what the sk_rmem_alloc check is for?
> 
Yes, and yes.
I conferenced with our TCP-God-in-Residence aka Olaf Kirch :-)

> I ask because I was looking at this the other day when I was trying to
> figure out why when the target closes the connection we sometimes do not
> figure it out right away and could not figure out why that sk_rmem_alloc
> check was there. I have noticed that the state changes and sk_rmem_alloc
> is greater than zero, so in iscsi_sw_tcp_state_change we end up not
> dropping the session. This then delays handling of the problem until a
> nop or scsi cmd times out and we figure out the connection is bad
> through that route.
> 
Yep, that's the problem I've noticed, too.
And that's fixed by this patch.

> Can the state hit WAIT or CLOSE and sk_rmem_alloc be greater than zero
> and we can end up going on successfully? Or do we check that value
> because it would leak mem if non-zero or something like that?
> 
Right, some more detail here.

sk_rmem_alloc tells us the bytes of the incoming data which have been
allocated for this socket. So, by inference, if it's not zero there
is some data to be read.

And the check in state_change() is there to avoid us closing the
connection / socket if there is still some data to be read from it.

The real problem is that the callbacks are called in reverse order
(check net/ipv4/tcp_input.c:tcp_data_queue()); first the 'FIN' flag
is evaluated (and the state_change() callback invoked), and _then_
the data_ready() callback is triggered.
Which essentially means that by the time data_ready() is called
the socket might already be in CLOSE or WAIT, even though we haven't
read all data.
Hence we need the check at the end of the callback to make sure
we're pulling down the connection, too.
Otherwise we're seeing the SCSI timeouts.

> If we need it or not, I would just put the check in some new function or
> macro.
> 
Yes, good point. Should be doing it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                   zSeries & Storage
h...@suse.de                          +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to