From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tariq Saeed Date: Mon, 07 Apr 2014 20:07:41 -0700 Subject: [Ocfs2-devel] [patch 03/11] ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one In-Reply-To: <20140319140143.b3a0fa14b49284fb79c1bd61@linux-foundation.org> References: <20140124204702.9871031C2C7@corp2gmr1-1.hot.corp.google.com> <20140124215554.GC24361@wotan.suse.de> <52E2E7C1.7070801@oracle.com> <20140319140143.b3a0fa14b49284fb79c1bd61@linux-foundation.org> Message-ID: <534367FD.8060403@oracle.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ocfs2-devel@oss.oracle.com On 03/19/2014 02:01 PM, Andrew Morton wrote: > On Fri, 24 Jan 2014 14:22:57 -0800 tariq saeed wrote: > >> On 1/24/2014 1:55 PM, Mark Fasheh wrote: >>> On Fri, Jan 24, 2014 at 12:47:02PM -0800, akpm at linux-foundation.org wrote: >>>> From: Tariq Saeed >>>> Subject: ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one >>>> >>>> When o2net-accept-one() rejects an illegal connection, it terminates the >>>> loop picking up the remaining queued connections. This fix will continue >>>> accepting connections till the queue is emtpy. >>>> >>>> Addresses Orabug 17489469. >>> Thanks for sending this, review comments below. >>> >>> >>>> diff -puN fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one fs/ocfs2/cluster/tcp.c >>>> --- a/fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one >>>> +++ a/fs/ocfs2/cluster/tcp.c >>>> @@ -1826,7 +1826,7 @@ int o2net_register_hb_callbacks(void) >>>> >>>> /* ------------------------------------------------------------ */ >>>> >>>> -static int o2net_accept_one(struct socket *sock) >>>> +static int o2net_accept_one(struct socket *sock, int *more) >>>> { >>>> int ret, slen; >>>> struct sockaddr_in sin; >>>> @@ -1837,6 +1837,7 @@ static int o2net_accept_one(struct socke >>>> struct o2net_node *nn; >>>> >>>> BUG_ON(sock == NULL); >>>> + *more = 0; >>>> ret = sock_create_lite(sock->sk->sk_family, sock->sk->sk_type, >>>> sock->sk->sk_protocol, &new_sock); >>>> if (ret) >>>> @@ -1848,6 +1849,7 @@ static int o2net_accept_one(struct socke >>>> if (ret < 0) >>>> goto out; >>>> >>>> + *more = 1; >>>> new_sock->sk->sk_allocation = GFP_ATOMIC; >>>> >>>> ret = o2net_set_nodelay(new_sock); >>>> @@ -1949,8 +1951,15 @@ out: >>>> static void o2net_accept_many(struct work_struct *work) >>>> { >>>> struct socket *sock = o2net_listen_sock; >>>> - while (o2net_accept_one(sock) == 0) >>>> + int more; >>>> + int err; >>>> + >>>> + for (;;) { >>>> + err = o2net_accept_one(sock, &more); >>>> + if (!more) >>>> + break; >>> We're throwing out 'err' here and trusting the variable 'more'. However, err >>> could be set and more would be 0 regardless of whether there actually are >>> more connections to be had. This makes more sense given when 'more' is set: >> >> Thanks for the comments. >> To understand the consequences of ignoring the err, we need to look at >> what is going on. >> We get a softIRQ when a connection packet (tcp SYN). It is critical to >> note that we may not >> get a softIRQ_for every connection s_ince connection packets can arrive >> back-to-back (as happened in this bug). So, one softIRQ could be >> delivered for > 1 pending accept. >> _This is the KEY point. _ >> >> If we terminate the loop calling o2net_accept_one() upon seeing an >> error, what happens >> to the rest of the connections in the queue. If no new connection >> arrives for hours, no new softIRQ >> will be delivered, and the connections will just sit in the queue. > > Please note that I had to edit your email to undo the top-posting so I > could reply to it. Please don't top-post. > > Mark, are you now OK with the patch as-is? Mark, do you have further questios? > > > From: Tariq Saeed > Subject: ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one > > When o2net-accept-one() rejects an illegal connection, it terminates the > loop picking up the remaining queued connections. This fix will continue > accepting connections till the queue is emtpy. > > Addresses Orabug 17489469. > > Signed-off-by: Tariq Saseed > Cc: Mark Fasheh > Cc: Joel Becker > Signed-off-by: Andrew Morton > --- > > fs/ocfs2/cluster/tcp.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff -puN fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one fs/ocfs2/cluster/tcp.c > --- a/fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one > +++ a/fs/ocfs2/cluster/tcp.c > @@ -1826,7 +1826,7 @@ int o2net_register_hb_callbacks(void) > > /* ------------------------------------------------------------ */ > > -static int o2net_accept_one(struct socket *sock) > +static int o2net_accept_one(struct socket *sock, int *more) > { > int ret, slen; > struct sockaddr_in sin; > @@ -1837,6 +1837,7 @@ static int o2net_accept_one(struct socke > struct o2net_node *nn; > > BUG_ON(sock == NULL); > + *more = 0; > ret = sock_create_lite(sock->sk->sk_family, sock->sk->sk_type, > sock->sk->sk_protocol, &new_sock); > if (ret) > @@ -1848,6 +1849,7 @@ static int o2net_accept_one(struct socke > if (ret < 0) > goto out; > > + *more = 1; > new_sock->sk->sk_allocation = GFP_ATOMIC; > > ret = o2net_set_nodelay(new_sock); > @@ -1949,8 +1951,15 @@ out: > static void o2net_accept_many(struct work_struct *work) > { > struct socket *sock = o2net_listen_sock; > - while (o2net_accept_one(sock) == 0) > + int more; > + int err; > + > + for (;;) { > + err = o2net_accept_one(sock, &more); > + if (!more) > + break; > cond_resched(); > + } > } > > static void o2net_listen_data_ready(struct sock *sk, int bytes) > _ >