[PATCH] libceph: fix protocol feature mismatch failure path

[PATCH] libceph: fix protocol feature mismatch failure path

[Sage Weil]

We should not set con->state to CLOSED here; that happens in ceph_fault()
in the caller, where it first asserts that the state is not yet CLOSED.
Avoids a BUG when the features don't match.

Signed-off-by: Sage Weil <sage@inktank.com>
---
 net/ceph/messenger.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 4d111fd..24a5c86 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1508,9 +1508,9 @@ static int process_banner(struct ceph_connection *con)
 
 static void fail_protocol(struct ceph_connection *con)
 {
+	dout("fail_protocol %p\n", con);
 	reset_connection(con);
 	BUG_ON(con->state != CON_STATE_NEGOTIATING);
-	con->state = CON_STATE_CLOSED;
 }
 
 static int process_connect(struct ceph_connection *con)
-- 
1.7.9

Re: [PATCH] libceph: fix protocol feature mismatch failure path

[Alex Elder]

On 12/27/2012 06:07 PM, Sage Weil wrote:
> We should not set con->state to CLOSED here; that happens in ceph_fault()
> in the caller, where it first asserts that the state is not yet CLOSED.

I'm not seeing the code path you're talking about.
Do you mean in the LOSSYTX case?

(I don't doubt you're right, I'm just trying to follow
along at home.)

> Avoids a BUG when the features don't match.

Is this related to the crashes reported here?
    http://tracker.newdream.net/issues/3657

> Signed-off-by: Sage Weil <sage@inktank.com>
> ---
>  net/ceph/messenger.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 4d111fd..24a5c86 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1508,9 +1508,9 @@ static int process_banner(struct ceph_connection *con)
>  
>  static void fail_protocol(struct ceph_connection *con)
>  {
> +	dout("fail_protocol %p\n", con);
>  	reset_connection(con);
>  	BUG_ON(con->state != CON_STATE_NEGOTIATING);

Since fail_protocol becomes essentially a trivial wrapper
for reset_connection(), I think it should just go away
and call reset_connection() directly.  The assertion that
it's in NEGOTIATING state is not very useful at the moment;
fail_protocol() is only called from process_connect(),
and that's only called from try_read when the state
is NEGOTIATING.

					-Alex

> -	con->state = CON_STATE_CLOSED;
>  }
>  
>  static int process_connect(struct ceph_connection *con)
> 

Re: [PATCH] libceph: fix protocol feature mismatch failure path

[Sage Weil]

On Thu, 27 Dec 2012, Alex Elder wrote:
> On 12/27/2012 06:07 PM, Sage Weil wrote:
> > We should not set con->state to CLOSED here; that happens in ceph_fault()
> > in the caller, where it first asserts that the state is not yet CLOSED.
> 
> I'm not seeing the code path you're talking about.
> Do you mean in the LOSSYTX case?

It's when the feature bits don't match.  It calls this, sets CLOSED< and 
then returns -1 and con_work() calls ceph_fault().

> 
> (I don't doubt you're right, I'm just trying to follow
> along at home.)
> 
> > Avoids a BUG when the features don't match.
> 
> Is this related to the crashes reported here?
>     http://tracker.newdream.net/issues/3657
> 
> > Signed-off-by: Sage Weil <sage@inktank.com>
> > ---
> >  net/ceph/messenger.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> > index 4d111fd..24a5c86 100644
> > --- a/net/ceph/messenger.c
> > +++ b/net/ceph/messenger.c
> > @@ -1508,9 +1508,9 @@ static int process_banner(struct ceph_connection *con)
> >  
> >  static void fail_protocol(struct ceph_connection *con)
> >  {
> > +	dout("fail_protocol %p\n", con);
> >  	reset_connection(con);
> >  	BUG_ON(con->state != CON_STATE_NEGOTIATING);
> 
> Since fail_protocol becomes essentially a trivial wrapper
> for reset_connection(), I think it should just go away
> and call reset_connection() directly.  The assertion that
> it's in NEGOTIATING state is not very useful at the moment;
> fail_protocol() is only called from process_connect(),
> and that's only called from try_read when the state
> is NEGOTIATING.

Good point.  I'll clean that up!


> 
> 					-Alex
> 
> > -	con->state = CON_STATE_CLOSED;
> >  }
> >  
> >  static int process_connect(struct ceph_connection *con)
> > 
> 
>