[PATCH] ceph: remove req from unsafe list before calling __unregister_request

[PATCH] ceph: remove req from unsafe list before calling __unregister_request

[Jeff Layton]

...and while we're at it, let's ensure that no one inadvertantly
leaves this thing on the unsafe list. There's no reason it should
ever be on a s_unsafe list but not in the request tree.

Cc: stable@vger.kernel.org
Link: http://tracker.ceph.com/issues/18474
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/mds_client.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 52521f339745..b47e97680a66 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -628,6 +628,8 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
 {
 	dout("__unregister_request %p tid %lld\n", req, req->r_tid);
 
+	WARN_ON_ONCE(!list_empty(&req->r_unsafe_item));
+
 	if (req->r_tid == mdsc->oldest_tid) {
 		struct rb_node *p = rb_next(&req->r_node);
 		mdsc->oldest_tid = 0;
@@ -3512,6 +3514,7 @@ static void wait_requests(struct ceph_mds_client *mdsc)
 		while ((req = __get_oldest_req(mdsc))) {
 			dout("wait_requests timed out on tid %llu\n",
 			     req->r_tid);
+			list_del_init(&req->r_unsafe_item);
 			__unregister_request(mdsc, req);
 		}
 	}
-- 
2.9.3

Re: [PATCH] ceph: remove req from unsafe list before calling __unregister_request

[Yan, Zheng]

> On 9 Feb 2017, at 20:18, Jeff Layton <jlayton@redhat.com> wrote:
> 
> ...and while we're at it, let's ensure that no one inadvertantly
> leaves this thing on the unsafe list. There's no reason it should
> ever be on a s_unsafe list but not in the request tree.
> 
> Cc: stable@vger.kernel.org
> Link: http://tracker.ceph.com/issues/18474
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/ceph/mds_client.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 52521f339745..b47e97680a66 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -628,6 +628,8 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
> {
> 	dout("__unregister_request %p tid %lld\n", req, req->r_tid);
> 
> +	WARN_ON_ONCE(!list_empty(&req->r_unsafe_item));
> +
> 	if (req->r_tid == mdsc->oldest_tid) {
> 		struct rb_node *p = rb_next(&req->r_node);
> 		mdsc->oldest_tid = 0;
> @@ -3512,6 +3514,7 @@ static void wait_requests(struct ceph_mds_client *mdsc)
> 		while ((req = __get_oldest_req(mdsc))) {
> 			dout("wait_requests timed out on tid %llu\n",
> 			     req->r_tid);
> +			list_del_init(&req->r_unsafe_item);
> 			__unregister_request(mdsc, req);
> 		}
> 	}

Reviewed-by: Yan, Zheng <zyan@redhat.com>
> -- 
> 2.9.3
> 

Re: [PATCH] ceph: remove req from unsafe list before calling __unregister_request

[Jeff Layton]

On Thu, 2017-02-09 at 07:18 -0500, Jeff Layton wrote:
> ...and while we're at it, let's ensure that no one inadvertantly
> leaves this thing on the unsafe list. There's no reason it should
> ever be on a s_unsafe list but not in the request tree.
> 
> Cc: stable@vger.kernel.org
> Link: http://tracker.ceph.com/issues/18474
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/ceph/mds_client.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 52521f339745..b47e97680a66 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -628,6 +628,8 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
>  {
>  	dout("__unregister_request %p tid %lld\n", req, req->r_tid);
>  
> +	WARN_ON_ONCE(!list_empty(&req->r_unsafe_item));
> +
>  	if (req->r_tid == mdsc->oldest_tid) {
>  		struct rb_node *p = rb_next(&req->r_node);
>  		mdsc->oldest_tid = 0;
> @@ -3512,6 +3514,7 @@ static void wait_requests(struct ceph_mds_client *mdsc)
>  		while ((req = __get_oldest_req(mdsc))) {
>  			dout("wait_requests timed out on tid %llu\n",
>  			     req->r_tid);
> +			list_del_init(&req->r_unsafe_item);
>  			__unregister_request(mdsc, req);
>  		}
>  	}

After going over the kernel code more thoroughly, I started to get
concerned that there were some cases (particularly those involving fatal
signals) where we could end up leaving it on the list before calling
__unregister_request.

I've just sent a v2 patch, that moves the list_del_init into
__unregister_request itself, which ensures that it's always removed from
the list in those cases as well.
-- 
Jeff Layton <jlayton@redhat.com>