Skip to contentSkip to conversation

ocfs2: o2net: fix connect expired

PATCH New Junxiao Bi 2 participants 4 messages Oct 30, 2014 – Oct 31, 2014 OCFS2

lore
Share & tools
lore

Junxiao Bi

author
#0 lore raw

PATCH 1/1 ocfs2: o2net: fix connect expired

Quoted 5 lines · ----- srinivas.eeda at oracle.com

Hi Junxiao,

thanks for explaining. For this case allowing a reconnect (setting "atomic_set(&nn->nn_timeout, 1);" ) in o2net_connect_expired should work ?

Hi Srini,

Yes, that should also work for this case. But it breaks the usage of nn_timeout, it is only used for idle timeout.

Thanks,
Junxiao.

Quoted 65 lines

Thanks,
--Srini

Quoted 6 lines · Junxiao Bi

Hi Srini,

Quoted 3 lines · Srinivas Eeda

Junxiao,

can you please describe under what circumstances you saw this

problem?

Quoted 1 line
Quoted 1 line

My understanding is o2net_connect_expired is only queued when

connection

Quoted 2 lines
Quoted 1 line

actually broke and ENOTCONN is the right error in that case.

This happened when o2net was issuing the first connect request to

some

Quoted 2 lines

node, but the request packet is lost due to some error like network broken, then the connect would be expired, in

o2net_connect_expired()

Quoted 1 line

that set nn->nn_persistent_error to -ENOTCONN but timeout was zero,

so

Quoted 11 lines

o2net_start_connect() would return without sending another connect request, connection to the node will never be built.

Thanks,
Junxiao.

Quoted 5 lines

Thanks,
--Srini

Quoted 1 line · Junxiao Bi

Set nn_persistent_error to -ENOTCONN will stop reconnect since

the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

"stop" condition in o2net_start_connect() will be true.

stop = (nn->nn_sc ||

         (nn->nn_persistent_error &&
         (nn->nn_persistent_error != -ENOTCONN || timeout ==

0)));

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

This will make connection never be established if the first

connection

Quoted 21 lines
Quoted 21 lines
Quoted 21 lines

request
is lost.

Signed-off-by Junxiao Bi <[email protected]>
---
   fs/ocfs2/cluster/tcp.c |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/cluster/tcp.c +1 -0
index 97de0fb..4d6b645 100644
@@ -1736,7 +1736,7 @@ static void o2net_connect_expired(struct
 work_struct *work)
                o2net_idle_timeout() / 1000,
                o2net_idle_timeout() % 1000);
   -        o2net_set_nn_state(nn, NULL, 0, -ENOTCONN);
+        o2net_set_nn_state(nn, NULL, 0, 0);
       }
       spin_unlock(&nn->nn_lock);
   }
#1 lore raw

PATCH 1/1 ocfs2: o2net: fix connect expired

Hi Junxiao,

I am hesitant to make changes to o2net_set_nn_state calls as it is a sensitive code. Also, currently, nn_timeout is only used to determine if a reconnect needs to be allowed or not. So, lets rename that to nn_reconnect and use it for that purpose.

With your change, o2net_connect_expired now doesn't pass -ENOTCONN when a network hb timeout happens. So o2net_set_nn_state now fails to wake_up any threads stuck on nn_sc_wq(check o2net_send_message_vec function) ?

Thanks,
--Srini

Quoted 76 lines · Junxiao Bi
Quoted 5 lines · ----- srinivas.eeda at oracle.com

Hi Junxiao,

thanks for explaining. For this case allowing a reconnect (setting "atomic_set(&nn->nn_timeout, 1);" ) in o2net_connect_expired should work ?

Hi Srini,

Yes, that should also work for this case. But it breaks the usage of nn_timeout, it is only used for idle timeout.

Thanks,
Junxiao.

Quoted 63 lines

Thanks,
--Srini

Quoted 6 lines · Junxiao Bi

Hi Srini,

Quoted 3 lines · Srinivas Eeda

Junxiao,

can you please describe under what circumstances you saw this

problem?

Quoted 1 line
Quoted 1 line

My understanding is o2net_connect_expired is only queued when

connection

Quoted 2 lines
Quoted 1 line

actually broke and ENOTCONN is the right error in that case.

This happened when o2net was issuing the first connect request to

some

Quoted 2 lines

node, but the request packet is lost due to some error like network broken, then the connect would be expired, in

o2net_connect_expired()

Quoted 1 line

that set nn->nn_persistent_error to -ENOTCONN but timeout was zero,

so

Quoted 11 lines

o2net_start_connect() would return without sending another connect request, connection to the node will never be built.

Thanks,
Junxiao.

Quoted 5 lines

Thanks,
--Srini

Quoted 1 line · Junxiao Bi

Set nn_persistent_error to -ENOTCONN will stop reconnect since

the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

"stop" condition in o2net_start_connect() will be true.

stop = (nn->nn_sc ||

          (nn->nn_persistent_error &&
          (nn->nn_persistent_error != -ENOTCONN || timeout ==

0)));

Quoted 1 line
Quoted 1 line
Quoted 1 line

This will make connection never be established if the first

connection

Quoted 21 lines
Quoted 21 lines
Quoted 21 lines

request
is lost.

Signed-off-by Junxiao Bi <[email protected]>
---
    fs/ocfs2/cluster/tcp.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/cluster/tcp.c +1 -0
index 97de0fb..4d6b645 100644
@@ -1736,7 +1736,7 @@ static void o2net_connect_expired(struct
 work_struct *work)
                 o2net_idle_timeout() / 1000,
                 o2net_idle_timeout() % 1000);
    -        o2net_set_nn_state(nn, NULL, 0, -ENOTCONN);
+        o2net_set_nn_state(nn, NULL, 0, 0);
        }
        spin_unlock(&nn->nn_lock);
    }
author
#2 lore raw

PATCH 1/1 ocfs2: o2net: fix connect expired

Hi Srini,

Quoted 10 lines · Srinivas Eeda

Hi Junxiao,

I am hesitant to make changes to o2net_set_nn_state calls as it is a sensitive code. Also, currently, nn_timeout is only used to determine if a reconnect needs to be allowed or not. So, lets rename that to nn_reconnect and use it for that purpose.

With your change, o2net_connect_expired now doesn't pass -ENOTCONN when a network hb timeout happens. So o2net_set_nn_state now fails to wake_up any threads stuck on nn_sc_wq(check o2net_send_message_vec function) ?

As we talked, dlm depends on network, for connection expires, dlm don't need be waken up, just wait network to recover is ok. Also dlm didn't do good work on error wakeup case. Could you add you review-by?

Thanks,
Junxiao.

Quoted 85 lines

Thanks,
--Srini

Quoted 77 lines · Junxiao Bi
Quoted 5 lines · ----- srinivas.eeda at oracle.com

Hi Junxiao,

thanks for explaining. For this case allowing a reconnect (setting "atomic_set(&nn->nn_timeout, 1);" ) in o2net_connect_expired should work ?

Hi Srini,

Yes, that should also work for this case. But it breaks the usage of nn_timeout, it is only used for idle timeout.

Thanks,
Junxiao.

Quoted 63 lines

Thanks,
--Srini

Quoted 6 lines · Junxiao Bi

Hi Srini,

Quoted 3 lines · Srinivas Eeda

Junxiao,

can you please describe under what circumstances you saw this

problem?

Quoted 1 line
Quoted 1 line

My understanding is o2net_connect_expired is only queued when

connection

Quoted 2 lines
Quoted 1 line

actually broke and ENOTCONN is the right error in that case.

This happened when o2net was issuing the first connect request to

some

Quoted 2 lines

node, but the request packet is lost due to some error like network broken, then the connect would be expired, in

o2net_connect_expired()

Quoted 1 line

that set nn->nn_persistent_error to -ENOTCONN but timeout was zero,

so

Quoted 11 lines

o2net_start_connect() would return without sending another connect request, connection to the node will never be built.

Thanks,
Junxiao.

Quoted 5 lines

Thanks,
--Srini

Quoted 1 line · Junxiao Bi

Set nn_persistent_error to -ENOTCONN will stop reconnect since

the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

"stop" condition in o2net_start_connect() will be true.

stop = (nn->nn_sc ||

          (nn->nn_persistent_error &&
          (nn->nn_persistent_error != -ENOTCONN || timeout ==

0)));

Quoted 1 line
Quoted 1 line
Quoted 1 line

This will make connection never be established if the first

connection

Quoted 21 lines
Quoted 21 lines
Quoted 21 lines

request
is lost.

Signed-off-by Junxiao Bi <[email protected]>
---
    fs/ocfs2/cluster/tcp.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/cluster/tcp.c +1 -0
index 97de0fb..4d6b645 100644
@@ -1736,7 +1736,7 @@ static void o2net_connect_expired(struct
 work_struct *work)
                 o2net_idle_timeout() / 1000,
                 o2net_idle_timeout() % 1000);
    -        o2net_set_nn_state(nn, NULL, 0, -ENOTCONN);
+        o2net_set_nn_state(nn, NULL, 0, 0);
        }
        spin_unlock(&nn->nn_lock);
    }
author
#3 lore raw

PATCH 1/1 ocfs2: o2net: fix connect expired

Quoted 10 lines · Junxiao Bi

Hi Srini,

Quoted 7 lines · Srinivas Eeda

Hi Junxiao,

I am hesitant to make changes to o2net_set_nn_state calls as it is a sensitive code. Also, currently, nn_timeout is only used to determine if a reconnect needs to be allowed or not. So, lets rename that to nn_reconnect and use it for that purpose.

Quoted 6 lines
Quoted 3 lines

With your change, o2net_connect_expired now doesn't pass -ENOTCONN when a network hb timeout happens. So o2net_set_nn_state now fails to wake_up any threads stuck on nn_sc_wq(check o2net_send_message_vec function) ?

As we talked, dlm depends on network, for connection expires, dlm don't need be waken up, just wait network to recover is ok. Also dlm didn't do good work on error wakeup case. Could you add you review-by?

Srini, send a v2 version to add more explanation. Please help review.

Thanks,
Junxiao.

Quoted 96 lines

Thanks,
Junxiao.

Quoted 71 lines

Thanks,
--Srini

Quoted 64 lines · Junxiao Bi
Quoted 5 lines · ----- srinivas.eeda at oracle.com

Hi Junxiao,

thanks for explaining. For this case allowing a reconnect (setting "atomic_set(&nn->nn_timeout, 1);" ) in o2net_connect_expired should work ?

Hi Srini,

Yes, that should also work for this case. But it breaks the usage of nn_timeout, it is only used for idle timeout.

Thanks,
Junxiao.

Quoted 50 lines

Thanks,
--Srini

Quoted 6 lines · Junxiao Bi

Hi Srini,

Quoted 3 lines · Srinivas Eeda

Junxiao,

can you please describe under what circumstances you saw this

problem?

Quoted 1 line
Quoted 1 line

My understanding is o2net_connect_expired is only queued when

connection

Quoted 2 lines
Quoted 1 line

actually broke and ENOTCONN is the right error in that case.

This happened when o2net was issuing the first connect request to

some

Quoted 2 lines

node, but the request packet is lost due to some error like network broken, then the connect would be expired, in

o2net_connect_expired()

Quoted 1 line

that set nn->nn_persistent_error to -ENOTCONN but timeout was zero,

so

Quoted 11 lines

o2net_start_connect() would return without sending another connect request, connection to the node will never be built.

Thanks,
Junxiao.

Quoted 5 lines

Thanks,
--Srini

Quoted 1 line · Junxiao Bi

Set nn_persistent_error to -ENOTCONN will stop reconnect since

the

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

"stop" condition in o2net_start_connect() will be true.

stop = (nn->nn_sc ||

          (nn->nn_persistent_error &&
          (nn->nn_persistent_error != -ENOTCONN || timeout ==

0)));

Quoted 1 line
Quoted 1 line
Quoted 1 line

This will make connection never be established if the first

connection

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

request
is lost.

Signed-off-by Junxiao Bi <[email protected]>
---
    fs/ocfs2/cluster/tcp.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/cluster/tcp.c +0 -0
index 97de0fb..4d6b645 100644
@@ -1736,7 +1736,7 @@ static void o2net_connect_expired(struct
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/ocfs2/cluster/tcp.c static void o2net_connect_expired(struct

Quoted 8 lines
fs/ocfs2/cluster/tcp.c +1 -0
 work_struct *work)
                 o2net_idle_timeout() / 1000,
                 o2net_idle_timeout() % 1000);
    -        o2net_set_nn_state(nn, NULL, 0, -ENOTCONN);
+        o2net_set_nn_state(nn, NULL, 0, 0);
        }
        spin_unlock(&nn->nn_lock);
    }

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel