[1/9] libceph: fix safe completion

Commit Message

Yan, Zheng June 4, 2013, 3:19 a.m. UTC

From: "Yan, Zheng" <[e-mail]>

handle_reply() calls complete_request() only if the first OSD reply
has ONDISK flag.

Signed-off-by: Yan, Zheng <[e-mail]>
---
 include/linux/ceph/osd_client.h |  1 -
 net/ceph/osd_client.c           | 16 ++++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

Comments

Alex Elder June 11, 2013, 4:04 a.m. UTC | #1

On 06/03/2013 10:19 PM, Yan, Zheng wrote:
> From: "Yan, Zheng" <[e-mail]>
> 
> handle_reply() calls complete_request() only if the first OSD reply
> has ONDISK flag.

I believe that you're trying to fix a simple problem here, but
you are changing the logic around in several ways at the same
time and it makes it very difficult to see.

Let me see if I can explain what you've done:
- There's no reason to defer setting already_completed; it can
  set earlier.
- req->r_completed will be 0 until the first time a reply for
  req is received, at which point it will be set to 1.  That
  is exactly the same as what happens for req->r_got_reply,
  so already_completed can be equivalently set from that.
- That makes req->r_completed unnecessary, so it can be
  removed.
- The test near the end can be inverted, and a block can
  be executed rather than jumping over it with "goto done;"

Now, given those changes...
- This leaves the call to complete_request() happening *only*
  when the request had not already been completed *and* the
  current completion supplied the ONDISK flag.

And therein lies the problem you're trying to solve--it's
possible that a completion for the request arrived before,
but did not have the ONDISK flag set, and because of that a
later request with ONDISK set will not call complete_request()
as required.

The fix for that is to move the complete_request() call out
so it's called only when ONDISK is set, but regardless of
the value of already_completed.

Is that correct?

If my understanding is correct, I guess I'll say I've reviewed
this change, and in that case:

    Reviewed-by: Alex Elder <[e-mail]>

It would have been a lot easier to review this with a
better explanation, and with fewer logic changes rolled
into the patch.

					-Alex


> Signed-off-by: Yan, Zheng <[e-mail]>
> ---
>  include/linux/ceph/osd_client.h |  1 -
>  net/ceph/osd_client.c           | 16 ++++++++--------
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h
> index 186db0b..ce6df39 100644
> --- a/include/linux/ceph/osd_client.h
> +++ b/include/linux/ceph/osd_client.h
> @@ -145,7 +145,6 @@ struct ceph_osd_request {
>  	s32               r_reply_op_result[CEPH_OSD_MAX_OP];
>  	int               r_got_reply;
>  	int		  r_linger;
> -	int		  r_completed;
>  
>  	struct ceph_osd_client *r_osdc;
>  	struct kref       r_kref;
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index a3395fd..536c0e5 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -1525,6 +1525,8 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg,
>  	for (i = 0; i < numops; i++)
>  		req->r_reply_op_result[i] = ceph_decode_32(&p);
>  
> +	already_completed = req->r_got_reply;
> +
>  	if (!req->r_got_reply) {
>  
>  		req->r_result = result;
> @@ -1555,16 +1557,14 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg,
>  	    ((flags & CEPH_OSD_FLAG_WRITE) == 0))
>  		__unregister_request(osdc, req);
>  
> -	already_completed = req->r_completed;
> -	req->r_completed = 1;
>  	mutex_unlock(&osdc->request_mutex);
> -	if (already_completed)
> -		goto done;
>  
> -	if (req->r_callback)
> -		req->r_callback(req, msg);
> -	else
> -		complete_all(&req->r_completion);
> +	if (!already_completed) {
> +		if (req->r_callback)
> +			req->r_callback(req, msg);
> +		else
> +			complete_all(&req->r_completion);
> +	}
>  
>  	if (flags & CEPH_OSD_FLAG_ONDISK)
>  		complete_request(req);
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h
index 186db0b..ce6df39 100644
--- a/include/linux/ceph/osd_client.h
+++ b/include/linux/ceph/osd_client.h
@@ -145,7 +145,6 @@  struct ceph_osd_request {
 	s32               r_reply_op_result[CEPH_OSD_MAX_OP];
 	int               r_got_reply;
 	int		  r_linger;
-	int		  r_completed;
 
 	struct ceph_osd_client *r_osdc;
 	struct kref       r_kref;
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index a3395fd..536c0e5 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1525,6 +1525,8 @@  static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg,
 	for (i = 0; i < numops; i++)
 		req->r_reply_op_result[i] = ceph_decode_32(&p);
 
+	already_completed = req->r_got_reply;
+
 	if (!req->r_got_reply) {
 
 		req->r_result = result;
@@ -1555,16 +1557,14 @@  static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg,
 	    ((flags & CEPH_OSD_FLAG_WRITE) == 0))
 		__unregister_request(osdc, req);
 
-	already_completed = req->r_completed;
-	req->r_completed = 1;
 	mutex_unlock(&osdc->request_mutex);
-	if (already_completed)
-		goto done;
 
-	if (req->r_callback)
-		req->r_callback(req, msg);
-	else
-		complete_all(&req->r_completion);
+	if (!already_completed) {
+		if (req->r_callback)
+			req->r_callback(req, msg);
+		else
+			complete_all(&req->r_completion);
+	}
 
 	if (flags & CEPH_OSD_FLAG_ONDISK)
 		complete_request(req);