[PATCH] libceph: add lingering request reference when registered

[PATCH] libceph: add lingering request reference when registered

[Alex Elder]

When an osd request is set to linger, the osd client holds onto the
request so it can be re-submitted following certain osd map changes.
The osd client holds a reference to the request until it is
unregistered.  This is used by rbd for watch requests.

Currently, the reference is taken when the request is marked with
the linger flag.  This means that if an error occurs after that
time but before the the request completes successfully, that
reference is leaked.

There's really no reason to hold the reference until the request is
registered in the the osd client's list of lingering requests, and
that only happens when the lingering (watch) request completes
successfully.

So take that reference only when it gets registered following
succesful completion, and drop it (as before) when the request
gets unregistered.  This avoids the reference problem on error
in rbd.

Rearrange ceph_osdc_unregister_linger_request() to avoid using
the request pointer after it may have been freed.

And hold an extra reference in kick_requests() while handling
a linger request that has not yet been registered, to ensure
it doesn't go away.

This resolves:
    http://tracker.ceph.com/issues/3859

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/osd_client.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 3a246a6..e0abb83 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1174,6 +1174,7 @@ static void __register_linger_request(struct
ceph_osd_client *osdc,
 				    struct ceph_osd_request *req)
 {
 	dout("__register_linger_request %p\n", req);
+	ceph_osdc_get_request(req);
 	list_add_tail(&req->r_linger_item, &osdc->req_linger);
 	if (req->r_osd)
 		list_add_tail(&req->r_linger_osd,
@@ -1196,6 +1197,7 @@ static void __unregister_linger_request(struct
ceph_osd_client *osdc,
 		if (list_empty(&req->r_osd_item))
 			req->r_osd = NULL;
 	}
+	ceph_osdc_put_request(req);
 }

 void ceph_osdc_unregister_linger_request(struct ceph_osd_client *osdc,
@@ -1203,9 +1205,8 @@ void ceph_osdc_unregister_linger_request(struct
ceph_osd_client *osdc,
 {
 	mutex_lock(&osdc->request_mutex);
 	if (req->r_linger) {
-		__unregister_linger_request(osdc, req);
 		req->r_linger = 0;
-		ceph_osdc_put_request(req);
+		__unregister_linger_request(osdc, req);
 	}
 	mutex_unlock(&osdc->request_mutex);
 }
@@ -1217,11 +1218,6 @@ void ceph_osdc_set_request_linger(struct
ceph_osd_client *osdc,
 	if (!req->r_linger) {
 		dout("set_request_linger %p\n", req);
 		req->r_linger = 1;
-		/*
-		 * caller is now responsible for calling
-		 * unregister_linger_request
-		 */
-		ceph_osdc_get_request(req);
 	}
 }
 EXPORT_SYMBOL(ceph_osdc_set_request_linger);
@@ -1633,8 +1629,10 @@ static void kick_requests(struct ceph_osd_client
*osdc, int force_resend)
 			dout("%p tid %llu restart on osd%d\n",
 			     req, req->r_tid,
 			     req->r_osd ? req->r_osd->o_osd : -1);
+			ceph_osdc_get_request(req);
 			__unregister_request(osdc, req);
 			__register_linger_request(osdc, req);
+			ceph_osdc_put_request(req);
 			continue;
 		}

-- 
1.7.9.5

Re: [PATCH] libceph: add lingering request reference when registered

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 05/23/2013 05:11 AM, Alex Elder wrote:
> When an osd request is set to linger, the osd client holds onto the
> request so it can be re-submitted following certain osd map changes.
> The osd client holds a reference to the request until it is
> unregistered.  This is used by rbd for watch requests.
>
> Currently, the reference is taken when the request is marked with
> the linger flag.  This means that if an error occurs after that
> time but before the the request completes successfully, that
> reference is leaked.
>
> There's really no reason to hold the reference until the request is
> registered in the the osd client's list of lingering requests, and
> that only happens when the lingering (watch) request completes
> successfully.
>
> So take that reference only when it gets registered following
> succesful completion, and drop it (as before) when the request
> gets unregistered.  This avoids the reference problem on error
> in rbd.
>
> Rearrange ceph_osdc_unregister_linger_request() to avoid using
> the request pointer after it may have been freed.
>
> And hold an extra reference in kick_requests() while handling
> a linger request that has not yet been registered, to ensure
> it doesn't go away.
>
> This resolves:
>      http://tracker.ceph.com/issues/3859
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   net/ceph/osd_client.c |   12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 3a246a6..e0abb83 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -1174,6 +1174,7 @@ static void __register_linger_request(struct
> ceph_osd_client *osdc,
>   				    struct ceph_osd_request *req)
>   {
>   	dout("__register_linger_request %p\n", req);
> +	ceph_osdc_get_request(req);
>   	list_add_tail(&req->r_linger_item, &osdc->req_linger);
>   	if (req->r_osd)
>   		list_add_tail(&req->r_linger_osd,
> @@ -1196,6 +1197,7 @@ static void __unregister_linger_request(struct
> ceph_osd_client *osdc,
>   		if (list_empty(&req->r_osd_item))
>   			req->r_osd = NULL;
>   	}
> +	ceph_osdc_put_request(req);
>   }
>
>   void ceph_osdc_unregister_linger_request(struct ceph_osd_client *osdc,
> @@ -1203,9 +1205,8 @@ void ceph_osdc_unregister_linger_request(struct
> ceph_osd_client *osdc,
>   {
>   	mutex_lock(&osdc->request_mutex);
>   	if (req->r_linger) {
> -		__unregister_linger_request(osdc, req);
>   		req->r_linger = 0;
> -		ceph_osdc_put_request(req);
> +		__unregister_linger_request(osdc, req);
>   	}
>   	mutex_unlock(&osdc->request_mutex);
>   }
> @@ -1217,11 +1218,6 @@ void ceph_osdc_set_request_linger(struct
> ceph_osd_client *osdc,
>   	if (!req->r_linger) {
>   		dout("set_request_linger %p\n", req);
>   		req->r_linger = 1;
> -		/*
> -		 * caller is now responsible for calling
> -		 * unregister_linger_request
> -		 */
> -		ceph_osdc_get_request(req);
>   	}
>   }
>   EXPORT_SYMBOL(ceph_osdc_set_request_linger);
> @@ -1633,8 +1629,10 @@ static void kick_requests(struct ceph_osd_client
> *osdc, int force_resend)
>   			dout("%p tid %llu restart on osd%d\n",
>   			     req, req->r_tid,
>   			     req->r_osd ? req->r_osd->o_osd : -1);
> +			ceph_osdc_get_request(req);
>   			__unregister_request(osdc, req);
>   			__register_linger_request(osdc, req);
> +			ceph_osdc_put_request(req);
>   			continue;
>   		}
>