| Message ID | 20191204062718.56105-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix possible long time wait during umount | expand |
On Wed, 2019-12-04 at 01:27 -0500, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > During umount, if there has no any unsafe request in the mdsc and > some requests still in-flight and not got reply yet, and if the > rest requets are all safe ones, after that even all of them in mdsc > are unregistered, the umount must wait until after mount_timeout > seconds anyway. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/mds_client.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 163b470f3000..39f4d8501df5 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -2877,6 +2877,10 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg) > set_bit(CEPH_MDS_R_GOT_SAFE, &req->r_req_flags); > __unregister_request(mdsc, req); > > + /* last request during umount? */ > + if (mdsc->stopping && !__get_oldest_req(mdsc)) > + complete_all(&mdsc->safe_umount_waiters); > + > if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags)) { > /* > * We already handled the unsafe response, now do the > @@ -2887,9 +2891,6 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg) > */ > dout("got safe reply %llu, mds%d\n", tid, mds); > > - /* last unsafe request during umount? */ > - if (mdsc->stopping && !__get_oldest_req(mdsc)) > - complete_all(&mdsc->safe_umount_waiters); > mutex_unlock(&mdsc->mutex); > goto out; > } Looks reasonable. AIUI, the MDS is free to send a safe reply without ever sending an unsafe one, so I don't see why we want to make that conditional on receiving an earlier unsafe reply.
On 2019/12/4 22:26, Jeff Layton wrote: > On Wed, 2019-12-04 at 01:27 -0500, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> During umount, if there has no any unsafe request in the mdsc and >> some requests still in-flight and not got reply yet, and if the >> rest requets are all safe ones, after that even all of them in mdsc >> are unregistered, the umount must wait until after mount_timeout >> seconds anyway. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/mds_client.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >> index 163b470f3000..39f4d8501df5 100644 >> --- a/fs/ceph/mds_client.c >> +++ b/fs/ceph/mds_client.c >> @@ -2877,6 +2877,10 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg) >> set_bit(CEPH_MDS_R_GOT_SAFE, &req->r_req_flags); >> __unregister_request(mdsc, req); >> >> + /* last request during umount? */ >> + if (mdsc->stopping && !__get_oldest_req(mdsc)) >> + complete_all(&mdsc->safe_umount_waiters); >> + >> if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags)) { >> /* >> * We already handled the unsafe response, now do the >> @@ -2887,9 +2891,6 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg) >> */ >> dout("got safe reply %llu, mds%d\n", tid, mds); >> >> - /* last unsafe request during umount? */ >> - if (mdsc->stopping && !__get_oldest_req(mdsc)) >> - complete_all(&mdsc->safe_umount_waiters); >> mutex_unlock(&mdsc->mutex); >> goto out; >> } > Looks reasonable. AIUI, the MDS is free to send a safe reply without > ever sending an unsafe one, Yeah, that's the same from the test output. > so I don't see why we want to make that > conditional on receiving an earlier unsafe reply. From the commit history I couldn't get why. Thanks. BRs
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 163b470f3000..39f4d8501df5 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2877,6 +2877,10 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg) set_bit(CEPH_MDS_R_GOT_SAFE, &req->r_req_flags); __unregister_request(mdsc, req); + /* last request during umount? */ + if (mdsc->stopping && !__get_oldest_req(mdsc)) + complete_all(&mdsc->safe_umount_waiters); + if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags)) { /* * We already handled the unsafe response, now do the @@ -2887,9 +2891,6 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg) */ dout("got safe reply %llu, mds%d\n", tid, mds); - /* last unsafe request during umount? */ - if (mdsc->stopping && !__get_oldest_req(mdsc)) - complete_all(&mdsc->safe_umount_waiters); mutex_unlock(&mdsc->mutex); goto out; }