| Message ID | 1372056089-11603-1-git-send-email-zheng.z.yan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
ping I think this patch should goes into 3.11 or fix the issue by other means On 06/24/2013 02:41 PM, Yan, Zheng wrote: > From: "Yan, Zheng" <zheng.z.yan@intel.com> > > We can't use !req->r_sent to check if OSD request is sent for the > first time, this is because __cancel_request() zeros req->r_sent > when OSD map changes. Rather than adding a new variable to struct > ceph_osd_request to indicate if it's sent for the first time, We > can call the unsafe callback only when unsafe OSD reply is received. > If OSD's first reply is safe, just skip calling the unsafe callback. > > The purpose of unsafe callback is adding unsafe request to a list, > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need > to wait for a write(2) that hasn't returned yet. So it's OK to add > request to the unsafe list when the first OSD reply is received. > (ceph_sync_write() returns after receiving the first OSD reply) > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > --- > net/ceph/osd_client.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index 540dd29..dd47889 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, > > ceph_msg_get(req->r_request); /* send consumes a ref */ > > - /* Mark the request unsafe if this is the first timet's being sent. */ > - > - if (!req->r_sent && req->r_unsafe_callback) > - req->r_unsafe_callback(req, true); > req->r_sent = req->r_osd->o_incarnation; > > ceph_con_send(&req->r_osd->o_con, req->r_request); > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) > > static void complete_request(struct ceph_osd_request *req) > { > - if (req->r_unsafe_callback) > - req->r_unsafe_callback(req, false); > complete_all(&req->r_safe_completion); /* fsync waiter */ > } > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > mutex_unlock(&osdc->request_mutex); > > if (!already_completed) { > + if (req->r_unsafe_callback && > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) > + req->r_unsafe_callback(req, true); > if (req->r_callback) > req->r_callback(req, msg); > else > complete_all(&req->r_completion); > } > > - if (flags & CEPH_OSD_FLAG_ONDISK) > + if (flags & CEPH_OSD_FLAG_ONDISK) { > + if (req->r_unsafe_callback && already_completed) > + req->r_unsafe_callback(req, false); > complete_request(req); > + } > > done: > dout("req=%p req->r_linger=%d\n", req, req->r_linger); > -- 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 Mon, 1 Jul 2013, Yan, Zheng wrote: > ping > > I think this patch should goes into 3.11 or fix the issue by other means Applied this to the testing branch, thanks. Let me know if there are any others I missed! sage > > > On 06/24/2013 02:41 PM, Yan, Zheng wrote: > > From: "Yan, Zheng" <zheng.z.yan@intel.com> > > > > We can't use !req->r_sent to check if OSD request is sent for the > > first time, this is because __cancel_request() zeros req->r_sent > > when OSD map changes. Rather than adding a new variable to struct > > ceph_osd_request to indicate if it's sent for the first time, We > > can call the unsafe callback only when unsafe OSD reply is received. > > If OSD's first reply is safe, just skip calling the unsafe callback. > > > > The purpose of unsafe callback is adding unsafe request to a list, > > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need > > to wait for a write(2) that hasn't returned yet. So it's OK to add > > request to the unsafe list when the first OSD reply is received. > > (ceph_sync_write() returns after receiving the first OSD reply) > > > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > > --- > > net/ceph/osd_client.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > index 540dd29..dd47889 100644 > > --- a/net/ceph/osd_client.c > > +++ b/net/ceph/osd_client.c > > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, > > > > ceph_msg_get(req->r_request); /* send consumes a ref */ > > > > - /* Mark the request unsafe if this is the first timet's being sent. */ > > - > > - if (!req->r_sent && req->r_unsafe_callback) > > - req->r_unsafe_callback(req, true); > > req->r_sent = req->r_osd->o_incarnation; > > > > ceph_con_send(&req->r_osd->o_con, req->r_request); > > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) > > > > static void complete_request(struct ceph_osd_request *req) > > { > > - if (req->r_unsafe_callback) > > - req->r_unsafe_callback(req, false); > > complete_all(&req->r_safe_completion); /* fsync waiter */ > > } > > > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > > mutex_unlock(&osdc->request_mutex); > > > > if (!already_completed) { > > + if (req->r_unsafe_callback && > > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) > > + req->r_unsafe_callback(req, true); > > if (req->r_callback) > > req->r_callback(req, msg); > > else > > complete_all(&req->r_completion); > > } > > > > - if (flags & CEPH_OSD_FLAG_ONDISK) > > + if (flags & CEPH_OSD_FLAG_ONDISK) { > > + if (req->r_unsafe_callback && already_completed) > > + req->r_unsafe_callback(req, false); > > complete_request(req); > > + } > > > > done: > > dout("req=%p req->r_linger=%d\n", req, req->r_linger); > > > > -- 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 06/24/2013 01:41 AM, Yan, Zheng wrote: > From: "Yan, Zheng" <zheng.z.yan@intel.com> Sorry it took so long, I intended to take a look at this for you sooner. I would also like to thank you for this nice clear description. It made it very easy to understand why you were proposing the change, and to focus in on exactly which parts of the design it's affecting. > We can't use !req->r_sent to check if OSD request is sent for the > first time, this is because __cancel_request() zeros req->r_sent > when OSD map changes. Rather than adding a new variable to struct You're right. > ceph_osd_request to indicate if it's sent for the first time, We > can call the unsafe callback only when unsafe OSD reply is received. > If OSD's first reply is safe, just skip calling the unsafe callback. This seems reasonable, but it's different from the way I thought about what constituted "unsafe." But I may be wrong, and the way this is used by the file system might do something that addresses my concern. The way I interpreted "unsafe" was simply that it was possible a write *could* have been made persistent, even if the client doesn't know about it. A request could have made it to its target osd, been written, and the response could be in flight at the point something (maybe a router?) crashes and the response gets lost. During that time window, the stored data may not be in a state that's consistent with the client's view of it. So I thought of "unsafe" as meaning that a write is in flight, and until we get a successful response, the storage might contain the old data or it might contain the new data; the client has no way of knowing which. With that interpretation, a request becomes unsafe the instant it leaves the client, and becomes safe again the instant a response arrives. If my interpretation is correct, this change is wrong. But I may be wrong, and there may really be no need to worry about a possible modification of data until after an acknowledgement response is received. In that case, I've looked at your patch and it looks good. Can you explain why I'm wrong about what is "unsafe?" -Alex > The purpose of unsafe callback is adding unsafe request to a list, > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need > to wait for a write(2) that hasn't returned yet. So it's OK to add > request to the unsafe list when the first OSD reply is received. > (ceph_sync_write() returns after receiving the first OSD reply) > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > --- > net/ceph/osd_client.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index 540dd29..dd47889 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, > > ceph_msg_get(req->r_request); /* send consumes a ref */ > > - /* Mark the request unsafe if this is the first timet's being sent. */ > - > - if (!req->r_sent && req->r_unsafe_callback) > - req->r_unsafe_callback(req, true); > req->r_sent = req->r_osd->o_incarnation; > > ceph_con_send(&req->r_osd->o_con, req->r_request); > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) > > static void complete_request(struct ceph_osd_request *req) > { > - if (req->r_unsafe_callback) > - req->r_unsafe_callback(req, false); > complete_all(&req->r_safe_completion); /* fsync waiter */ > } > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > mutex_unlock(&osdc->request_mutex); > > if (!already_completed) { > + if (req->r_unsafe_callback && > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) > + req->r_unsafe_callback(req, true); > if (req->r_callback) > req->r_callback(req, msg); > else > complete_all(&req->r_completion); > } > > - if (flags & CEPH_OSD_FLAG_ONDISK) > + if (flags & CEPH_OSD_FLAG_ONDISK) { > + if (req->r_unsafe_callback && already_completed) > + req->r_unsafe_callback(req, false); > complete_request(req); > + } > > done: > dout("req=%p req->r_linger=%d\n", req, req->r_linger); > -- 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 Tue, Jul 2, 2013 at 9:07 PM, Alex Elder <alex.elder@linaro.org> wrote: > On 06/24/2013 01:41 AM, Yan, Zheng wrote: >> From: "Yan, Zheng" <zheng.z.yan@intel.com> > > Sorry it took so long, I intended to take a look at this > for you sooner. > > I would also like to thank you for this nice clear > description. It made it very easy to understand > why you were proposing the change, and to focus in > on exactly which parts of the design it's affecting. > >> We can't use !req->r_sent to check if OSD request is sent for the >> first time, this is because __cancel_request() zeros req->r_sent >> when OSD map changes. Rather than adding a new variable to struct > > You're right. > >> ceph_osd_request to indicate if it's sent for the first time, We >> can call the unsafe callback only when unsafe OSD reply is received. >> If OSD's first reply is safe, just skip calling the unsafe callback. > > This seems reasonable, but it's different from the way I > thought about what constituted "unsafe." But I may be > wrong, and the way this is used by the file system might > do something that addresses my concern. > > The way I interpreted "unsafe" was simply that it was possible > a write *could* have been made persistent, even if the client > doesn't know about it. A request could have made it to its > target osd, been written, and the response could be in flight > at the point something (maybe a router?) crashes and the response > gets lost. During that time window, the stored data may not be > in a state that's consistent with the client's view of it. > > So I thought of "unsafe" as meaning that a write is in flight, > and until we get a successful response, the storage might > contain the old data or it might contain the new data; the > client has no way of knowing which. > > With that interpretation, a request becomes unsafe the > instant it leaves the client, and becomes safe again > the instant a response arrives. > > If my interpretation is correct, this change is wrong. > > But I may be wrong, and there may really be no need to > worry about a possible modification of data until after > an acknowledgement response is received. In that case, > I've looked at your patch and it looks good. > > Can you explain why I'm wrong about what is "unsafe?" I didn't say you are wrong. the reason I changed the meaning of the unsafe callback is that the "unsafe' callback is only used for fsync(2). I think it's OK to change it as long as the change does not break fsync(2). regards yan, zheng > > -Alex > >> The purpose of unsafe callback is adding unsafe request to a list, >> so that fsync(2) can wait for the safe reply. fsync(2) doesn't need >> to wait for a write(2) that hasn't returned yet. So it's OK to add >> request to the unsafe list when the first OSD reply is received. >> (ceph_sync_write() returns after receiving the first OSD reply) >> >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >> --- >> net/ceph/osd_client.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> index 540dd29..dd47889 100644 >> --- a/net/ceph/osd_client.c >> +++ b/net/ceph/osd_client.c >> @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, >> >> ceph_msg_get(req->r_request); /* send consumes a ref */ >> >> - /* Mark the request unsafe if this is the first timet's being sent. */ >> - >> - if (!req->r_sent && req->r_unsafe_callback) >> - req->r_unsafe_callback(req, true); >> req->r_sent = req->r_osd->o_incarnation; >> >> ceph_con_send(&req->r_osd->o_con, req->r_request); >> @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) >> >> static void complete_request(struct ceph_osd_request *req) >> { >> - if (req->r_unsafe_callback) >> - req->r_unsafe_callback(req, false); >> complete_all(&req->r_safe_completion); /* fsync waiter */ >> } >> >> @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, >> mutex_unlock(&osdc->request_mutex); >> >> if (!already_completed) { >> + if (req->r_unsafe_callback && >> + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) >> + req->r_unsafe_callback(req, true); >> if (req->r_callback) >> req->r_callback(req, msg); >> else >> complete_all(&req->r_completion); >> } >> >> - if (flags & CEPH_OSD_FLAG_ONDISK) >> + if (flags & CEPH_OSD_FLAG_ONDISK) { >> + if (req->r_unsafe_callback && already_completed) >> + req->r_unsafe_callback(req, false); >> complete_request(req); >> + } >> >> done: >> dout("req=%p req->r_linger=%d\n", req, req->r_linger); >> > > -- > 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 Tue, 2 Jul 2013, Alex Elder wrote: > On 06/24/2013 01:41 AM, Yan, Zheng wrote: > > From: "Yan, Zheng" <zheng.z.yan@intel.com> > > Sorry it took so long, I intended to take a look at this > for you sooner. > > I would also like to thank you for this nice clear > description. It made it very easy to understand > why you were proposing the change, and to focus in > on exactly which parts of the design it's affecting. > > > We can't use !req->r_sent to check if OSD request is sent for the > > first time, this is because __cancel_request() zeros req->r_sent > > when OSD map changes. Rather than adding a new variable to struct > > You're right. > > > ceph_osd_request to indicate if it's sent for the first time, We > > can call the unsafe callback only when unsafe OSD reply is received. > > If OSD's first reply is safe, just skip calling the unsafe callback. > > This seems reasonable, but it's different from the way I > thought about what constituted "unsafe." But I may be > wrong, and the way this is used by the file system might > do something that addresses my concern. > > The way I interpreted "unsafe" was simply that it was possible > a write *could* have been made persistent, even if the client > doesn't know about it. A request could have made it to its > target osd, been written, and the response could be in flight > at the point something (maybe a router?) crashes and the response > gets lost. During that time window, the stored data may not be > in a state that's consistent with the client's view of it. > > So I thought of "unsafe" as meaning that a write is in flight, > and until we get a successful response, the storage might > contain the old data or it might contain the new data; the > client has no way of knowing which. > > With that interpretation, a request becomes unsafe the > instant it leaves the client, and becomes safe again > the instant a response arrives. > > If my interpretation is correct, this change is wrong. The interpretation is correct, but in this case it doesn't matter. There are two intervals: - write(2) starts - request is sent <interval 1> - got ack reply, write(2) returns <interval 2> - got commit reply The important end result is that we need to wait for requests in interval 2 if we fsync(). With your 'unsafe' definition, we *also* wait for syscalls that haven't returned yet, but this isn't necessary... fsync() need only wait for completed but uncommitted writes, not racing ones. We could quibble about better naming, but the end result is correct. sage > > But I may be wrong, and there may really be no need to > worry about a possible modification of data until after > an acknowledgement response is received. In that case, > I've looked at your patch and it looks good. > > Can you explain why I'm wrong about what is "unsafe?" > > -Alex > > > The purpose of unsafe callback is adding unsafe request to a list, > > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need > > to wait for a write(2) that hasn't returned yet. So it's OK to add > > request to the unsafe list when the first OSD reply is received. > > (ceph_sync_write() returns after receiving the first OSD reply) > > > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > > --- > > net/ceph/osd_client.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > index 540dd29..dd47889 100644 > > --- a/net/ceph/osd_client.c > > +++ b/net/ceph/osd_client.c > > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, > > > > ceph_msg_get(req->r_request); /* send consumes a ref */ > > > > - /* Mark the request unsafe if this is the first timet's being sent. */ > > - > > - if (!req->r_sent && req->r_unsafe_callback) > > - req->r_unsafe_callback(req, true); > > req->r_sent = req->r_osd->o_incarnation; > > > > ceph_con_send(&req->r_osd->o_con, req->r_request); > > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) > > > > static void complete_request(struct ceph_osd_request *req) > > { > > - if (req->r_unsafe_callback) > > - req->r_unsafe_callback(req, false); > > complete_all(&req->r_safe_completion); /* fsync waiter */ > > } > > > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > > mutex_unlock(&osdc->request_mutex); > > > > if (!already_completed) { > > + if (req->r_unsafe_callback && > > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) > > + req->r_unsafe_callback(req, true); > > if (req->r_callback) > > req->r_callback(req, msg); > > else > > complete_all(&req->r_completion); > > } > > > > - if (flags & CEPH_OSD_FLAG_ONDISK) > > + if (flags & CEPH_OSD_FLAG_ONDISK) { > > + if (req->r_unsafe_callback && already_completed) > > + req->r_unsafe_callback(req, false); > > complete_request(req); > > + } > > > > done: > > dout("req=%p req->r_linger=%d\n", req, req->r_linger); > > > > -- > 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/02/2013 01:10 PM, Sage Weil wrote: > On Tue, 2 Jul 2013, Alex Elder wrote: >> On 06/24/2013 01:41 AM, Yan, Zheng wrote: >>> From: "Yan, Zheng" <zheng.z.yan@intel.com> >> >> Sorry it took so long, I intended to take a look at this >> for you sooner. >> >> I would also like to thank you for this nice clear >> description. It made it very easy to understand >> why you were proposing the change, and to focus in >> on exactly which parts of the design it's affecting. >> >>> We can't use !req->r_sent to check if OSD request is sent for the >>> first time, this is because __cancel_request() zeros req->r_sent >>> when OSD map changes. Rather than adding a new variable to struct >> >> You're right. >> >>> ceph_osd_request to indicate if it's sent for the first time, We >>> can call the unsafe callback only when unsafe OSD reply is received. >>> If OSD's first reply is safe, just skip calling the unsafe callback. >> >> This seems reasonable, but it's different from the way I >> thought about what constituted "unsafe." But I may be >> wrong, and the way this is used by the file system might >> do something that addresses my concern. >> >> The way I interpreted "unsafe" was simply that it was possible >> a write *could* have been made persistent, even if the client >> doesn't know about it. A request could have made it to its >> target osd, been written, and the response could be in flight >> at the point something (maybe a router?) crashes and the response >> gets lost. During that time window, the stored data may not be >> in a state that's consistent with the client's view of it. >> >> So I thought of "unsafe" as meaning that a write is in flight, >> and until we get a successful response, the storage might >> contain the old data or it might contain the new data; the >> client has no way of knowing which. >> >> With that interpretation, a request becomes unsafe the >> instant it leaves the client, and becomes safe again >> the instant a response arrives. >> >> If my interpretation is correct, this change is wrong. > > The interpretation is correct, but in this case it doesn't matter. There > are two intervals: > > - write(2) starts > - request is sent > <interval 1> > - got ack reply, write(2) returns > <interval 2> > - got commit reply > > The important end result is that we need to wait for requests in interval > 2 if we fsync(). With your 'unsafe' definition, we *also* wait for > syscalls that haven't returned yet, but this isn't necessary... fsync() > need only wait for completed but uncommitted writes, not racing ones. We > could quibble about better naming, but the end result is correct. OK, sounds good to me. In that case you can include this if you like: Reviewed-by: Alex Elder <elder@linaro.org> > sage > > >> >> But I may be wrong, and there may really be no need to >> worry about a possible modification of data until after >> an acknowledgement response is received. In that case, >> I've looked at your patch and it looks good. >> >> Can you explain why I'm wrong about what is "unsafe?" >> >> -Alex >> >>> The purpose of unsafe callback is adding unsafe request to a list, >>> so that fsync(2) can wait for the safe reply. fsync(2) doesn't need >>> to wait for a write(2) that hasn't returned yet. So it's OK to add >>> request to the unsafe list when the first OSD reply is received. >>> (ceph_sync_write() returns after receiving the first OSD reply) >>> >>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >>> --- >>> net/ceph/osd_client.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >>> index 540dd29..dd47889 100644 >>> --- a/net/ceph/osd_client.c >>> +++ b/net/ceph/osd_client.c >>> @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, >>> >>> ceph_msg_get(req->r_request); /* send consumes a ref */ >>> >>> - /* Mark the request unsafe if this is the first timet's being sent. */ >>> - >>> - if (!req->r_sent && req->r_unsafe_callback) >>> - req->r_unsafe_callback(req, true); >>> req->r_sent = req->r_osd->o_incarnation; >>> >>> ceph_con_send(&req->r_osd->o_con, req->r_request); >>> @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) >>> >>> static void complete_request(struct ceph_osd_request *req) >>> { >>> - if (req->r_unsafe_callback) >>> - req->r_unsafe_callback(req, false); >>> complete_all(&req->r_safe_completion); /* fsync waiter */ >>> } >>> >>> @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, >>> mutex_unlock(&osdc->request_mutex); >>> >>> if (!already_completed) { >>> + if (req->r_unsafe_callback && >>> + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) >>> + req->r_unsafe_callback(req, true); >>> if (req->r_callback) >>> req->r_callback(req, msg); >>> else >>> complete_all(&req->r_completion); >>> } >>> >>> - if (flags & CEPH_OSD_FLAG_ONDISK) >>> + if (flags & CEPH_OSD_FLAG_ONDISK) { >>> + if (req->r_unsafe_callback && already_completed) >>> + req->r_unsafe_callback(req, false); >>> complete_request(req); >>> + } >>> >>> done: >>> dout("req=%p req->r_linger=%d\n", req, req->r_linger); >>> >> >> -- >> 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
Hi Yan- On Mon, 1 Jul 2013, Sage Weil wrote: > On Mon, 1 Jul 2013, Yan, Zheng wrote: > > ping > > > > I think this patch should goes into 3.11 or fix the issue by other means > > Applied this to the testing branch, thanks. Let me know if there are any > others I missed! This broke rbd, which was using the unsafe callback. I pushed a patch to simplify that (testing-next^); care to take a look? Thanks! sage > > sage > > > > > > > On 06/24/2013 02:41 PM, Yan, Zheng wrote: > > > From: "Yan, Zheng" <zheng.z.yan@intel.com> > > > > > > We can't use !req->r_sent to check if OSD request is sent for the > > > first time, this is because __cancel_request() zeros req->r_sent > > > when OSD map changes. Rather than adding a new variable to struct > > > ceph_osd_request to indicate if it's sent for the first time, We > > > can call the unsafe callback only when unsafe OSD reply is received. > > > If OSD's first reply is safe, just skip calling the unsafe callback. > > > > > > The purpose of unsafe callback is adding unsafe request to a list, > > > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need > > > to wait for a write(2) that hasn't returned yet. So it's OK to add > > > request to the unsafe list when the first OSD reply is received. > > > (ceph_sync_write() returns after receiving the first OSD reply) > > > > > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > > > --- > > > net/ceph/osd_client.c | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > > index 540dd29..dd47889 100644 > > > --- a/net/ceph/osd_client.c > > > +++ b/net/ceph/osd_client.c > > > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, > > > > > > ceph_msg_get(req->r_request); /* send consumes a ref */ > > > > > > - /* Mark the request unsafe if this is the first timet's being sent. */ > > > - > > > - if (!req->r_sent && req->r_unsafe_callback) > > > - req->r_unsafe_callback(req, true); > > > req->r_sent = req->r_osd->o_incarnation; > > > > > > ceph_con_send(&req->r_osd->o_con, req->r_request); > > > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) > > > > > > static void complete_request(struct ceph_osd_request *req) > > > { > > > - if (req->r_unsafe_callback) > > > - req->r_unsafe_callback(req, false); > > > complete_all(&req->r_safe_completion); /* fsync waiter */ > > > } > > > > > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > > > mutex_unlock(&osdc->request_mutex); > > > > > > if (!already_completed) { > > > + if (req->r_unsafe_callback && > > > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) > > > + req->r_unsafe_callback(req, true); > > > if (req->r_callback) > > > req->r_callback(req, msg); > > > else > > > complete_all(&req->r_completion); > > > } > > > > > > - if (flags & CEPH_OSD_FLAG_ONDISK) > > > + if (flags & CEPH_OSD_FLAG_ONDISK) { > > > + if (req->r_unsafe_callback && already_completed) > > > + req->r_unsafe_callback(req, false); > > > complete_request(req); > > > + } > > > > > > done: > > > dout("req=%p req->r_linger=%d\n", req, req->r_linger); > > > > > > > > -- > 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
Yan, Can you help me understand how this change fixes: http://tracker.ceph.com/issues/2019 ? The symptom on the client is that the processes get stuck waiting in ceph_mdsc_do_request according to /proc/PID/stack. Thanks in advance, - Milosz On Wed, Jul 3, 2013 at 5:57 PM, Sage Weil <sage@inktank.com> wrote: > Hi Yan- > > On Mon, 1 Jul 2013, Sage Weil wrote: >> On Mon, 1 Jul 2013, Yan, Zheng wrote: >> > ping >> > >> > I think this patch should goes into 3.11 or fix the issue by other means >> >> Applied this to the testing branch, thanks. Let me know if there are any >> others I missed! > > This broke rbd, which was using the unsafe callback. I pushed a patch to > simplify that (testing-next^); care to take a look? > > Thanks! > sage > > >> >> sage >> >> > >> > >> > On 06/24/2013 02:41 PM, Yan, Zheng wrote: >> > > From: "Yan, Zheng" <zheng.z.yan@intel.com> >> > > >> > > We can't use !req->r_sent to check if OSD request is sent for the >> > > first time, this is because __cancel_request() zeros req->r_sent >> > > when OSD map changes. Rather than adding a new variable to struct >> > > ceph_osd_request to indicate if it's sent for the first time, We >> > > can call the unsafe callback only when unsafe OSD reply is received. >> > > If OSD's first reply is safe, just skip calling the unsafe callback. >> > > >> > > The purpose of unsafe callback is adding unsafe request to a list, >> > > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need >> > > to wait for a write(2) that hasn't returned yet. So it's OK to add >> > > request to the unsafe list when the first OSD reply is received. >> > > (ceph_sync_write() returns after receiving the first OSD reply) >> > > >> > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >> > > --- >> > > net/ceph/osd_client.c | 14 +++++++------- >> > > 1 file changed, 7 insertions(+), 7 deletions(-) >> > > >> > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> > > index 540dd29..dd47889 100644 >> > > --- a/net/ceph/osd_client.c >> > > +++ b/net/ceph/osd_client.c >> > > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, >> > > >> > > ceph_msg_get(req->r_request); /* send consumes a ref */ >> > > >> > > - /* Mark the request unsafe if this is the first timet's being sent. */ >> > > - >> > > - if (!req->r_sent && req->r_unsafe_callback) >> > > - req->r_unsafe_callback(req, true); >> > > req->r_sent = req->r_osd->o_incarnation; >> > > >> > > ceph_con_send(&req->r_osd->o_con, req->r_request); >> > > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) >> > > >> > > static void complete_request(struct ceph_osd_request *req) >> > > { >> > > - if (req->r_unsafe_callback) >> > > - req->r_unsafe_callback(req, false); >> > > complete_all(&req->r_safe_completion); /* fsync waiter */ >> > > } >> > > >> > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, >> > > mutex_unlock(&osdc->request_mutex); >> > > >> > > if (!already_completed) { >> > > + if (req->r_unsafe_callback && >> > > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) >> > > + req->r_unsafe_callback(req, true); >> > > if (req->r_callback) >> > > req->r_callback(req, msg); >> > > else >> > > complete_all(&req->r_completion); >> > > } >> > > >> > > - if (flags & CEPH_OSD_FLAG_ONDISK) >> > > + if (flags & CEPH_OSD_FLAG_ONDISK) { >> > > + if (req->r_unsafe_callback && already_completed) >> > > + req->r_unsafe_callback(req, false); >> > > complete_request(req); >> > > + } >> > > >> > > done: >> > > dout("req=%p req->r_linger=%d\n", req, req->r_linger); >> > > >> > >> > >> -- >> 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 -- 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 Wed, 3 Jul 2013, Milosz Tanski wrote: > Yan, > > Can you help me understand how this change fixes: > http://tracker.ceph.com/issues/2019 ? The symptom on the client is > that the processes get stuck waiting in ceph_mdsc_do_request according > to /proc/PID/stack. Note that the blocked request is a secondary effect; the MDS is trying to revoke caps (Fcb i think?) on that inode. It's not clear to me how that is related to this patch either, though. :) sage > > Thanks in advance, > - Milosz > > On Wed, Jul 3, 2013 at 5:57 PM, Sage Weil <sage@inktank.com> wrote: > > Hi Yan- > > > > On Mon, 1 Jul 2013, Sage Weil wrote: > >> On Mon, 1 Jul 2013, Yan, Zheng wrote: > >> > ping > >> > > >> > I think this patch should goes into 3.11 or fix the issue by other means > >> > >> Applied this to the testing branch, thanks. Let me know if there are any > >> others I missed! > > > > This broke rbd, which was using the unsafe callback. I pushed a patch to > > simplify that (testing-next^); care to take a look? > > > > Thanks! > > sage > > > > > >> > >> sage > >> > >> > > >> > > >> > On 06/24/2013 02:41 PM, Yan, Zheng wrote: > >> > > From: "Yan, Zheng" <zheng.z.yan@intel.com> > >> > > > >> > > We can't use !req->r_sent to check if OSD request is sent for the > >> > > first time, this is because __cancel_request() zeros req->r_sent > >> > > when OSD map changes. Rather than adding a new variable to struct > >> > > ceph_osd_request to indicate if it's sent for the first time, We > >> > > can call the unsafe callback only when unsafe OSD reply is received. > >> > > If OSD's first reply is safe, just skip calling the unsafe callback. > >> > > > >> > > The purpose of unsafe callback is adding unsafe request to a list, > >> > > so that fsync(2) can wait for the safe reply. fsync(2) doesn't need > >> > > to wait for a write(2) that hasn't returned yet. So it's OK to add > >> > > request to the unsafe list when the first OSD reply is received. > >> > > (ceph_sync_write() returns after receiving the first OSD reply) > >> > > > >> > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > >> > > --- > >> > > net/ceph/osd_client.c | 14 +++++++------- > >> > > 1 file changed, 7 insertions(+), 7 deletions(-) > >> > > > >> > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > >> > > index 540dd29..dd47889 100644 > >> > > --- a/net/ceph/osd_client.c > >> > > +++ b/net/ceph/osd_client.c > >> > > @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, > >> > > > >> > > ceph_msg_get(req->r_request); /* send consumes a ref */ > >> > > > >> > > - /* Mark the request unsafe if this is the first timet's being sent. */ > >> > > - > >> > > - if (!req->r_sent && req->r_unsafe_callback) > >> > > - req->r_unsafe_callback(req, true); > >> > > req->r_sent = req->r_osd->o_incarnation; > >> > > > >> > > ceph_con_send(&req->r_osd->o_con, req->r_request); > >> > > @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) > >> > > > >> > > static void complete_request(struct ceph_osd_request *req) > >> > > { > >> > > - if (req->r_unsafe_callback) > >> > > - req->r_unsafe_callback(req, false); > >> > > complete_all(&req->r_safe_completion); /* fsync waiter */ > >> > > } > >> > > > >> > > @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > >> > > mutex_unlock(&osdc->request_mutex); > >> > > > >> > > if (!already_completed) { > >> > > + if (req->r_unsafe_callback && > >> > > + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) > >> > > + req->r_unsafe_callback(req, true); > >> > > if (req->r_callback) > >> > > req->r_callback(req, msg); > >> > > else > >> > > complete_all(&req->r_completion); > >> > > } > >> > > > >> > > - if (flags & CEPH_OSD_FLAG_ONDISK) > >> > > + if (flags & CEPH_OSD_FLAG_ONDISK) { > >> > > + if (req->r_unsafe_callback && already_completed) > >> > > + req->r_unsafe_callback(req, false); > >> > > complete_request(req); > >> > > + } > >> > > > >> > > done: > >> > > dout("req=%p req->r_linger=%d\n", req, req->r_linger); > >> > > > >> > > >> > > >> -- > >> 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 > -- > 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/03/2013 04:57 PM, Sage Weil wrote: > Hi Yan- > > On Mon, 1 Jul 2013, Sage Weil wrote: >> On Mon, 1 Jul 2013, Yan, Zheng wrote: >>> ping >>> >>> I think this patch should goes into 3.11 or fix the issue by other means >> >> Applied this to the testing branch, thanks. Let me know if there are any >> others I missed! > > This broke rbd, which was using the unsafe callback. I pushed a patch to > simplify that (testing-next^); care to take a look? Sorry, I should have checked that when I reviewed it but I was paying attention to the explanation of how it fixed a problem in the file system code. I guess I assumed you'd verified the change didn't break anything else that used the code (I know, don't assume). The rbd code does use the callback for write requests, but only to know when they're safely on disk (it ignores the initial "request is unsafe" callback). -Alex > Thanks! > sage > > >> >> sage >> >>> >>> >>> On 06/24/2013 02:41 PM, Yan, Zheng wrote: >>>> From: "Yan, Zheng" <zheng.z.yan@intel.com> >>>> >>>> We can't use !req->r_sent to check if OSD request is sent for the >>>> first time, this is because __cancel_request() zeros req->r_sent >>>> when OSD map changes. Rather than adding a new variable to struct >>>> ceph_osd_request to indicate if it's sent for the first time, We >>>> can call the unsafe callback only when unsafe OSD reply is received. >>>> If OSD's first reply is safe, just skip calling the unsafe callback. >>>> >>>> The purpose of unsafe callback is adding unsafe request to a list, >>>> so that fsync(2) can wait for the safe reply. fsync(2) doesn't need >>>> to wait for a write(2) that hasn't returned yet. So it's OK to add >>>> request to the unsafe list when the first OSD reply is received. >>>> (ceph_sync_write() returns after receiving the first OSD reply) >>>> >>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >>>> --- >>>> net/ceph/osd_client.c | 14 +++++++------- >>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >>>> index 540dd29..dd47889 100644 >>>> --- a/net/ceph/osd_client.c >>>> +++ b/net/ceph/osd_client.c >>>> @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, >>>> >>>> ceph_msg_get(req->r_request); /* send consumes a ref */ >>>> >>>> - /* Mark the request unsafe if this is the first timet's being sent. */ >>>> - >>>> - if (!req->r_sent && req->r_unsafe_callback) >>>> - req->r_unsafe_callback(req, true); >>>> req->r_sent = req->r_osd->o_incarnation; >>>> >>>> ceph_con_send(&req->r_osd->o_con, req->r_request); >>>> @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) >>>> >>>> static void complete_request(struct ceph_osd_request *req) >>>> { >>>> - if (req->r_unsafe_callback) >>>> - req->r_unsafe_callback(req, false); >>>> complete_all(&req->r_safe_completion); /* fsync waiter */ >>>> } >>>> >>>> @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, >>>> mutex_unlock(&osdc->request_mutex); >>>> >>>> if (!already_completed) { >>>> + if (req->r_unsafe_callback && >>>> + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) >>>> + req->r_unsafe_callback(req, true); >>>> if (req->r_callback) >>>> req->r_callback(req, msg); >>>> else >>>> complete_all(&req->r_completion); >>>> } >>>> >>>> - if (flags & CEPH_OSD_FLAG_ONDISK) >>>> + if (flags & CEPH_OSD_FLAG_ONDISK) { >>>> + if (req->r_unsafe_callback && already_completed) >>>> + req->r_unsafe_callback(req, false); >>>> complete_request(req); >>>> + } >>>> >>>> done: >>>> dout("req=%p req->r_linger=%d\n", req, req->r_linger); >>>> >>> >>> >> -- >> 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 Thu, Jul 4, 2013 at 5:57 AM, Sage Weil <sage@inktank.com> wrote: > Hi Yan- > > On Mon, 1 Jul 2013, Sage Weil wrote: >> On Mon, 1 Jul 2013, Yan, Zheng wrote: >> > ping >> > >> > I think this patch should goes into 3.11 or fix the issue by other means >> >> Applied this to the testing branch, thanks. Let me know if there are any >> others I missed! > > This broke rbd, which was using the unsafe callback. I pushed a patch to > simplify that (testing-next^); care to take a look? > the patch looks good. looks like issue #5146 actually does not exist. Alex, could you take a look. Thanks Yan, Zheng -- 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 Thu, 4 Jul 2013, Yan, Zheng wrote: > On Thu, Jul 4, 2013 at 5:57 AM, Sage Weil <sage@inktank.com> wrote: > > Hi Yan- > > > > On Mon, 1 Jul 2013, Sage Weil wrote: > >> On Mon, 1 Jul 2013, Yan, Zheng wrote: > >> > ping > >> > > >> > I think this patch should goes into 3.11 or fix the issue by other means > >> > >> Applied this to the testing branch, thanks. Let me know if there are any > >> others I missed! > > > > This broke rbd, which was using the unsafe callback. I pushed a patch to > > simplify that (testing-next^); care to take a look? > > > > the patch looks good. looks like issue #5146 actually does not exist. > Alex, could you take a look. Yeah, i'll close. sage -- 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 Wed, 3 Jul 2013, Sage Weil wrote: > On Thu, 4 Jul 2013, Yan, Zheng wrote: > > On Thu, Jul 4, 2013 at 5:57 AM, Sage Weil <sage@inktank.com> wrote: > > > Hi Yan- > > > > > > On Mon, 1 Jul 2013, Sage Weil wrote: > > >> On Mon, 1 Jul 2013, Yan, Zheng wrote: > > >> > ping > > >> > > > >> > I think this patch should goes into 3.11 or fix the issue by other means > > >> > > >> Applied this to the testing branch, thanks. Let me know if there are any > > >> others I missed! > > > > > > This broke rbd, which was using the unsafe callback. I pushed a patch to > > > simplify that (testing-next^); care to take a look? > > > > > > > the patch looks good. looks like issue #5146 actually does not exist. > > Alex, could you take a look. > > Yeah, i'll close. Ok, I just realized that my patch is essentially reverting 12166906, which AFAICS was based on the assumption that ONDISK would get an ACK but not ONDISK reply from the OSD, but in reality we get only the ONDISK. We should just drop them both... sage -- 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 Thu, Jul 4, 2013 at 6:07 AM, Milosz Tanski <milosz@adfin.com> wrote: > Yan, > > Can you help me understand how this change fixes: > http://tracker.ceph.com/issues/2019 ? The symptom on the client is > that the processes get stuck waiting in ceph_mdsc_do_request according > to /proc/PID/stack. The bug this patch fixes is that ceph_sync_write_unsafe can be called multiple times with parameter unsafe=true. The bug prevents the kclient from releasing Fw cap, further lead to filelock stuck in unstable state forever and request hang. Yan, Zheng -- 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
Yan, So it looks like it fixes the issue. I had to update all my clients and restart MDS and things got back to normal. - Milosz On Wed, Jul 3, 2013 at 6:43 PM, Yan, Zheng <ukernel@gmail.com> wrote: > On Thu, Jul 4, 2013 at 6:07 AM, Milosz Tanski <milosz@adfin.com> wrote: >> Yan, >> >> Can you help me understand how this change fixes: >> http://tracker.ceph.com/issues/2019 ? The symptom on the client is >> that the processes get stuck waiting in ceph_mdsc_do_request according >> to /proc/PID/stack. > > The bug this patch fixes is that ceph_sync_write_unsafe can be called > multiple times > with parameter unsafe=true. The bug prevents the kclient from > releasing Fw cap, further > lead to filelock stuck in unstable state forever and request hang. > > Yan, Zheng -- 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
Yan, Actually after playing some more today I have another one of my clients stuck in this spot. When I look at the kernel stacks this is what I see for all the threads: [<ffffffffa02d2bab>] ceph_mdsc_do_request+0xcb/0x1a0 [ceph] [<ffffffffa02c018f>] ceph_do_getattr+0xdf/0x120 [ceph] [<ffffffffa02c01f4>] ceph_getattr+0x24/0x100 [ceph] [<ffffffff811775fd>] vfs_getattr+0x4d/0x80 [<ffffffff8117784d>] vfs_fstat+0x3d/0x70 [<ffffffff81177895>] SYSC_newfstat+0x15/0x30 [<ffffffff8117794e>] SyS_newfstat+0xe/0x10 [<ffffffff8155dd59>] system_call_fastpath+0x16/0x1b [<ffffffffffffffff>] 0xffffffffffffffff Anything I can do on my end to debug this issue? - Milosz P.S: Sorry for the second email if you got it gmail keep switching me to non-plain text mode. Sigh. On Mon, Jul 8, 2013 at 10:42 AM, Milosz Tanski <milosz@adfin.com> wrote: > Yan, > > So it looks like it fixes the issue. I had to update all my clients > and restart MDS and things got back to normal. > > - Milosz > > On Wed, Jul 3, 2013 at 6:43 PM, Yan, Zheng <ukernel@gmail.com> wrote: >> On Thu, Jul 4, 2013 at 6:07 AM, Milosz Tanski <milosz@adfin.com> wrote: >>> Yan, >>> >>> Can you help me understand how this change fixes: >>> http://tracker.ceph.com/issues/2019 ? The symptom on the client is >>> that the processes get stuck waiting in ceph_mdsc_do_request according >>> to /proc/PID/stack. >> >> The bug this patch fixes is that ceph_sync_write_unsafe can be called >> multiple times >> with parameter unsafe=true. The bug prevents the kclient from >> releasing Fw cap, further >> lead to filelock stuck in unstable state forever and request hang. >> >> Yan, Zheng -- 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 Tue, Jul 9, 2013 at 3:58 AM, Milosz Tanski <milosz@adfin.com> wrote: > Yan, > > Actually after playing some more today I have another one of my > clients stuck in this spot. When I look at the kernel stacks this is > what I see for all the threads: > > [<ffffffffa02d2bab>] ceph_mdsc_do_request+0xcb/0x1a0 [ceph] > [<ffffffffa02c018f>] ceph_do_getattr+0xdf/0x120 [ceph] > [<ffffffffa02c01f4>] ceph_getattr+0x24/0x100 [ceph] > [<ffffffff811775fd>] vfs_getattr+0x4d/0x80 > [<ffffffff8117784d>] vfs_fstat+0x3d/0x70 > [<ffffffff81177895>] SYSC_newfstat+0x15/0x30 > [<ffffffff8117794e>] SyS_newfstat+0xe/0x10 > [<ffffffff8155dd59>] system_call_fastpath+0x16/0x1b > [<ffffffffffffffff>] 0xffffffffffffffff > > > Anything I can do on my end to debug this issue? > find the hang request (and inode) through /sys/kernel/debug/ceph/xxx/mdsc, use 'ceph mds tell \* dumpcache' to dump mds cache. open /cachedump.xxx and check the inode's state. does your kernel include all fixes in testing branch of ceph-client ? does restarting the mds resolve the hang ? yan, zheng -- 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
In this case (unlike last week) the restart did unlock my clients. - M On Mon, Jul 8, 2013 at 4:30 PM, Yan, Zheng <ukernel@gmail.com> wrote: > On Tue, Jul 9, 2013 at 3:58 AM, Milosz Tanski <milosz@adfin.com> wrote: >> Yan, >> >> Actually after playing some more today I have another one of my >> clients stuck in this spot. When I look at the kernel stacks this is >> what I see for all the threads: >> >> [<ffffffffa02d2bab>] ceph_mdsc_do_request+0xcb/0x1a0 [ceph] >> [<ffffffffa02c018f>] ceph_do_getattr+0xdf/0x120 [ceph] >> [<ffffffffa02c01f4>] ceph_getattr+0x24/0x100 [ceph] >> [<ffffffff811775fd>] vfs_getattr+0x4d/0x80 >> [<ffffffff8117784d>] vfs_fstat+0x3d/0x70 >> [<ffffffff81177895>] SYSC_newfstat+0x15/0x30 >> [<ffffffff8117794e>] SyS_newfstat+0xe/0x10 >> [<ffffffff8155dd59>] system_call_fastpath+0x16/0x1b >> [<ffffffffffffffff>] 0xffffffffffffffff >> >> >> Anything I can do on my end to debug this issue? >> > > find the hang request (and inode) through /sys/kernel/debug/ceph/xxx/mdsc, > use 'ceph mds tell \* dumpcache' to dump mds cache. > open /cachedump.xxx and check the inode's state. > > does your kernel include all fixes in testing branch of ceph-client ? > does restarting the mds resolve the hang ? > > yan, zheng -- 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
I just wanted to follow up to say that after applying these patches and running it for a few weeks we're I haven't seen another lock up under load. - Milosz On Mon, Jul 8, 2013 at 5:16 PM, Milosz Tanski <milosz@adfin.com> wrote: > In this case (unlike last week) the restart did unlock my clients. > > - M > > On Mon, Jul 8, 2013 at 4:30 PM, Yan, Zheng <ukernel@gmail.com> wrote: >> On Tue, Jul 9, 2013 at 3:58 AM, Milosz Tanski <milosz@adfin.com> wrote: >>> Yan, >>> >>> Actually after playing some more today I have another one of my >>> clients stuck in this spot. When I look at the kernel stacks this is >>> what I see for all the threads: >>> >>> [<ffffffffa02d2bab>] ceph_mdsc_do_request+0xcb/0x1a0 [ceph] >>> [<ffffffffa02c018f>] ceph_do_getattr+0xdf/0x120 [ceph] >>> [<ffffffffa02c01f4>] ceph_getattr+0x24/0x100 [ceph] >>> [<ffffffff811775fd>] vfs_getattr+0x4d/0x80 >>> [<ffffffff8117784d>] vfs_fstat+0x3d/0x70 >>> [<ffffffff81177895>] SYSC_newfstat+0x15/0x30 >>> [<ffffffff8117794e>] SyS_newfstat+0xe/0x10 >>> [<ffffffff8155dd59>] system_call_fastpath+0x16/0x1b >>> [<ffffffffffffffff>] 0xffffffffffffffff >>> >>> >>> Anything I can do on my end to debug this issue? >>> >> >> find the hang request (and inode) through /sys/kernel/debug/ceph/xxx/mdsc, >> use 'ceph mds tell \* dumpcache' to dump mds cache. >> open /cachedump.xxx and check the inode's state. >> >> does your kernel include all fixes in testing branch of ceph-client ? >> does restarting the mds resolve the hang ? >> >> yan, zheng -- 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/osd_client.c b/net/ceph/osd_client.c index 540dd29..dd47889 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -1337,10 +1337,6 @@ static void __send_request(struct ceph_osd_client *osdc, ceph_msg_get(req->r_request); /* send consumes a ref */ - /* Mark the request unsafe if this is the first timet's being sent. */ - - if (!req->r_sent && req->r_unsafe_callback) - req->r_unsafe_callback(req, true); req->r_sent = req->r_osd->o_incarnation; ceph_con_send(&req->r_osd->o_con, req->r_request); @@ -1431,8 +1427,6 @@ static void handle_osds_timeout(struct work_struct *work) static void complete_request(struct ceph_osd_request *req) { - if (req->r_unsafe_callback) - req->r_unsafe_callback(req, false); complete_all(&req->r_safe_completion); /* fsync waiter */ } @@ -1559,14 +1553,20 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, mutex_unlock(&osdc->request_mutex); if (!already_completed) { + if (req->r_unsafe_callback && + result >= 0 && !(flags & CEPH_OSD_FLAG_ONDISK)) + req->r_unsafe_callback(req, true); if (req->r_callback) req->r_callback(req, msg); else complete_all(&req->r_completion); } - if (flags & CEPH_OSD_FLAG_ONDISK) + if (flags & CEPH_OSD_FLAG_ONDISK) { + if (req->r_unsafe_callback && already_completed) + req->r_unsafe_callback(req, false); complete_request(req); + } done: dout("req=%p req->r_linger=%d\n", req, req->r_linger);