| Message ID | 1342831308-18815-5-git-send-email-sage@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Yehuda Sadeh <yehuda@inktank.com> On Fri, Jul 20, 2012 at 5:41 PM, Sage Weil <sage@inktank.com> wrote: > Hold the mutex while twiddling all of the state bits to avoid possible > races. While we're here, make not of why we cannot close the socket > directly. > > Signed-off-by: Sage Weil <sage@inktank.com> > --- > net/ceph/messenger.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c > index 7105908..e24310e 100644 > --- a/net/ceph/messenger.c > +++ b/net/ceph/messenger.c > @@ -503,6 +503,7 @@ static void reset_connection(struct ceph_connection *con) > */ > void ceph_con_close(struct ceph_connection *con) > { > + mutex_lock(&con->mutex); > dout("con_close %p peer %s\n", con, > ceph_pr_addr(&con->peer_addr.in_addr)); > clear_bit(NEGOTIATING, &con->state); > @@ -515,11 +516,16 @@ void ceph_con_close(struct ceph_connection *con) > clear_bit(KEEPALIVE_PENDING, &con->flags); > clear_bit(WRITE_PENDING, &con->flags); > > - mutex_lock(&con->mutex); > reset_connection(con); > con->peer_global_seq = 0; > cancel_delayed_work(&con->work); > mutex_unlock(&con->mutex); > + > + /* > + * We cannot close the socket directly from here because the > + * work threads use it without holding the mutex. Instead, let > + * con_work() do it. > + */ > queue_con(con); > } > EXPORT_SYMBOL(ceph_con_close); > -- > 1.7.9 > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/20/2012 07:41 PM, Sage Weil wrote: > Hold the mutex while twiddling all of the state bits to avoid possible > races. While we're here, make not of why we cannot close the socket > directly. > > Signed-off-by: Sage Weil <sage@inktank.com> Looks OK to me. A quick scan seems to show that the state and flag bits are *almost* always set while the mutex is held. The one counterexample I found was for the STANDBY state bit in clear_standby() (but I really didn't look very closely). Anyway, I think this looks fine--it makes things safer even if there could be another imperfection somewhere. Reviewed-by: Alex Elder <elder@inktank.com> > --- > net/ceph/messenger.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c > index 7105908..e24310e 100644 > --- a/net/ceph/messenger.c > +++ b/net/ceph/messenger.c > @@ -503,6 +503,7 @@ static void reset_connection(struct ceph_connection *con) > */ > void ceph_con_close(struct ceph_connection *con) > { > + mutex_lock(&con->mutex); > dout("con_close %p peer %s\n", con, > ceph_pr_addr(&con->peer_addr.in_addr)); > clear_bit(NEGOTIATING, &con->state); > @@ -515,11 +516,16 @@ void ceph_con_close(struct ceph_connection *con) > clear_bit(KEEPALIVE_PENDING, &con->flags); > clear_bit(WRITE_PENDING, &con->flags); > > - mutex_lock(&con->mutex); > reset_connection(con); > con->peer_global_seq = 0; > cancel_delayed_work(&con->work); > mutex_unlock(&con->mutex); > + > + /* > + * We cannot close the socket directly from here because the > + * work threads use it without holding the mutex. Instead, let > + * con_work() do it. > + */ > queue_con(con); > } > EXPORT_SYMBOL(ceph_con_close); > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index 7105908..e24310e 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -503,6 +503,7 @@ static void reset_connection(struct ceph_connection *con) */ void ceph_con_close(struct ceph_connection *con) { + mutex_lock(&con->mutex); dout("con_close %p peer %s\n", con, ceph_pr_addr(&con->peer_addr.in_addr)); clear_bit(NEGOTIATING, &con->state); @@ -515,11 +516,16 @@ void ceph_con_close(struct ceph_connection *con) clear_bit(KEEPALIVE_PENDING, &con->flags); clear_bit(WRITE_PENDING, &con->flags); - mutex_lock(&con->mutex); reset_connection(con); con->peer_global_seq = 0; cancel_delayed_work(&con->work); mutex_unlock(&con->mutex); + + /* + * We cannot close the socket directly from here because the + * work threads use it without holding the mutex. Instead, let + * con_work() do it. + */ queue_con(con); } EXPORT_SYMBOL(ceph_con_close);
Hold the mutex while twiddling all of the state bits to avoid possible races. While we're here, make not of why we cannot close the socket directly. Signed-off-by: Sage Weil <sage@inktank.com> --- net/ceph/messenger.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)