[PATCH 0/2] libceph: lingering request fixes for 4.1

[PATCH 0/2] libceph: lingering request fixes for 4.1

[Ilya Dryomov]

Hello,

Trying to understand how watch/notify works for his v2 work, Doug tried
a simple test which uncovered a regression in lingering request requeue
code dating back to 3.18.  As it pretty much always is, a one liner is
all it took.  The reason it went unnoticed is we can't thrash it
because of watch/notify v1 limitations and the last round of manual
testing I did was around 3.16-3.17 after a bunch of fixes in that area,
so only osd-requeue (as opposed to osdmap-requeue) path was stressed.

While investigating it I found another problem, which probably goes all
the way back to the addition of watch/notify and that is what the first
patch is for.  The regression itself is fixed by a revert, with the
ground for it set up in the first patch.  Both are marked for stable
and will be backported together.

Thanks,

                Ilya


Ilya Dryomov (2):
  libceph: request a new osdmap if lingering request maps to no osd
  Revert "libceph: clear r_req_lru_item in
    __unregister_linger_request()"

 net/ceph/osd_client.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

-- 
1.9.3

[PATCH 1/2] libceph: request a new osdmap if lingering request maps to no osd

[Ilya Dryomov]

This commit does two things.  First, if there are any homeless
lingering requests, we now request a new osdmap even if the osdmap that
is being processed brought no changes, i.e. if a given lingering
request turned homeless in one of the previous epochs and remained
homeless in the current epoch.  Not doing so leaves us with a stale
osdmap and as a result we may miss our window for reestablishing the
watch and lose notifies.

MON=1 OSD=1:

    # cat linger-needmap.sh
    #!/bin/bash
    rbd create --size 1 test
    DEV=$(rbd map test)
    ceph osd out 0
    rbd map dne/dne # obtain a new osdmap as a side effect (!)
    sleep 1
    ceph osd in 0
    rbd resize --size 2 test
    # rbd info test | grep size -> 2M
    # blockdev --getsize $DEV -> 1M

N.B.: Not obtaining a new osdmap in between "osd out" and "osd in"
above is enough to make it miss that resize notify, but that is a
bug^Wlimitation of ceph watch/notify v1.

Second, homeless lingering requests are now kicked just like those
lingering requests whose mapping has changed.  This is mainly to
recognize that a homeless lingering request makes no sense and to
preserve the invariant that a registered lingering request is not
sitting on any of r_req_lru_item lists.  This spares us a WARN_ON,
which commit ba9d114ec557 ("libceph: clear r_req_lru_item in
__unregister_linger_request()") tried to fix the _wrong_ way.

Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 net/ceph/osd_client.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 41a4abc7e98e..31d4b1ebff01 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -2017,20 +2017,29 @@ static void kick_requests(struct ceph_osd_client *osdc, bool force_resend,
 		err = __map_request(osdc, req,
 				    force_resend || force_resend_writes);
 		dout("__map_request returned %d\n", err);
-		if (err == 0)
-			continue;  /* no change and no osd was specified */
 		if (err < 0)
 			continue;  /* hrm! */
-		if (req->r_osd == NULL) {
-			dout("tid %llu maps to no valid osd\n", req->r_tid);
-			needmap++;  /* request a newer map */
-			continue;
-		}
+		if (req->r_osd == NULL || err > 0) {
+			if (req->r_osd == NULL) {
+				dout("lingering %p tid %llu maps to no osd\n",
+				     req, req->r_tid);
+				/*
+				 * A homeless lingering request makes
+				 * no sense, as it's job is to keep
+				 * a particular OSD connection open.
+				 * Request a newer map and kick the
+				 * request, knowing that it won't be
+				 * resent until we actually get a map
+				 * that can tell us where to send it.
+				 */
+				needmap++;
+			}
 
-		dout("kicking lingering %p tid %llu osd%d\n", req, req->r_tid,
-		     req->r_osd ? req->r_osd->o_osd : -1);
-		__register_request(osdc, req);
-		__unregister_linger_request(osdc, req);
+			dout("kicking lingering %p tid %llu osd%d\n", req,
+			     req->r_tid, req->r_osd ? req->r_osd->o_osd : -1);
+			__register_request(osdc, req);
+			__unregister_linger_request(osdc, req);
+		}
 	}
 	reset_changed_osds(osdc);
 	mutex_unlock(&osdc->request_mutex);
-- 
1.9.3

[PATCH 2/2] Revert "libceph: clear r_req_lru_item in __unregister_linger_request()"

[Ilya Dryomov]

This reverts commit ba9d114ec5578e6e99a4dfa37ff8ae688040fd64.

.. which introduced a regression that prevented all lingering requests
requeued in kick_requests() from ever being sent to the OSDs, resulting
in a lot of missed notifies.  In retrospect it's pretty obvious that
r_req_lru_item item in the case of lingering requests can be used not
only for notarget, but also for unsent linkage due to how tightly
actual map and enqueue operations are coupled in __map_request().

The assertion that was being silenced is taken care of in the previous
("libceph: request a new osdmap if lingering request maps to no osd")
commit: by always kicking homeless lingering requests we ensure that
none of them ends up on the notarget list outside of the critical
section guarded by request_mutex.

Cc: stable@vger.kernel.org # 3.18+, needs 3d8c73c93605 "libceph: request a new osdmap if lingering request maps to no osd"
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 net/ceph/osd_client.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 31d4b1ebff01..c4ec9239249a 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1306,8 +1306,6 @@ static void __unregister_linger_request(struct ceph_osd_client *osdc,
 		if (list_empty(&req->r_osd_item))
 			req->r_osd = NULL;
 	}
-
-	list_del_init(&req->r_req_lru_item); /* can be on notarget */
 	ceph_osdc_put_request(req);
 }
 
-- 
1.9.3

Re: [PATCH 1/2] libceph: request a new osdmap if lingering request maps to no osd

[Sage Weil]

On Wed, 13 May 2015, Ilya Dryomov wrote:
> This commit does two things.  First, if there are any homeless
> lingering requests, we now request a new osdmap even if the osdmap that
> is being processed brought no changes, i.e. if a given lingering
> request turned homeless in one of the previous epochs and remained
> homeless in the current epoch.  Not doing so leaves us with a stale
> osdmap and as a result we may miss our window for reestablishing the
> watch and lose notifies.
> 
> MON=1 OSD=1:
> 
>     # cat linger-needmap.sh
>     #!/bin/bash
>     rbd create --size 1 test
>     DEV=$(rbd map test)
>     ceph osd out 0
>     rbd map dne/dne # obtain a new osdmap as a side effect (!)
>     sleep 1
>     ceph osd in 0
>     rbd resize --size 2 test
>     # rbd info test | grep size -> 2M
>     # blockdev --getsize $DEV -> 1M
> 
> N.B.: Not obtaining a new osdmap in between "osd out" and "osd in"
> above is enough to make it miss that resize notify, but that is a
> bug^Wlimitation of ceph watch/notify v1.
> 
> Second, homeless lingering requests are now kicked just like those
> lingering requests whose mapping has changed.  This is mainly to
> recognize that a homeless lingering request makes no sense and to
> preserve the invariant that a registered lingering request is not
> sitting on any of r_req_lru_item lists.  This spares us a WARN_ON,
> which commit ba9d114ec557 ("libceph: clear r_req_lru_item in
> __unregister_linger_request()") tried to fix the _wrong_ way.
> 
> Cc: stable@vger.kernel.org # 3.10+
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  net/ceph/osd_client.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 41a4abc7e98e..31d4b1ebff01 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -2017,20 +2017,29 @@ static void kick_requests(struct ceph_osd_client *osdc, bool force_resend,
>  		err = __map_request(osdc, req,
>  				    force_resend || force_resend_writes);
>  		dout("__map_request returned %d\n", err);
> -		if (err == 0)
> -			continue;  /* no change and no osd was specified */
>  		if (err < 0)
>  			continue;  /* hrm! */
> -		if (req->r_osd == NULL) {
> -			dout("tid %llu maps to no valid osd\n", req->r_tid);
> -			needmap++;  /* request a newer map */
> -			continue;
> -		}
> +		if (req->r_osd == NULL || err > 0) {
> +			if (req->r_osd == NULL) {
> +				dout("lingering %p tid %llu maps to no osd\n",
> +				     req, req->r_tid);
> +				/*
> +				 * A homeless lingering request makes
> +				 * no sense, as it's job is to keep
> +				 * a particular OSD connection open.
> +				 * Request a newer map and kick the
> +				 * request, knowing that it won't be
> +				 * resent until we actually get a map
> +				 * that can tell us where to send it.
> +				 */
> +				needmap++;
> +			}
>  
> -		dout("kicking lingering %p tid %llu osd%d\n", req, req->r_tid,
> -		     req->r_osd ? req->r_osd->o_osd : -1);
> -		__register_request(osdc, req);
> -		__unregister_linger_request(osdc, req);
> +			dout("kicking lingering %p tid %llu osd%d\n", req,
> +			     req->r_tid, req->r_osd ? req->r_osd->o_osd : -1);
> +			__register_request(osdc, req);
> +			__unregister_linger_request(osdc, req);
> +		}

Am I misreading this, or could you accomplish the same thing by just 
dropping the 'continue' statement in the NULL check block?  No real 
opinion either way if this is a style change, just wondering...

sage

>  	}
>  	reset_changed_osds(osdc);
>  	mutex_unlock(&osdc->request_mutex);
> -- 
> 1.9.3
> 
> --
> 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
> 
> 

Re: [PATCH 1/2] libceph: request a new osdmap if lingering request maps to no osd

[Ilya Dryomov]

On Wed, May 20, 2015 at 8:29 PM, Sage Weil <sage@newdream.net> wrote:
> On Wed, 13 May 2015, Ilya Dryomov wrote:
>> This commit does two things.  First, if there are any homeless
>> lingering requests, we now request a new osdmap even if the osdmap that
>> is being processed brought no changes, i.e. if a given lingering
>> request turned homeless in one of the previous epochs and remained
>> homeless in the current epoch.  Not doing so leaves us with a stale
>> osdmap and as a result we may miss our window for reestablishing the
>> watch and lose notifies.
>>
>> MON=1 OSD=1:
>>
>>     # cat linger-needmap.sh
>>     #!/bin/bash
>>     rbd create --size 1 test
>>     DEV=$(rbd map test)
>>     ceph osd out 0
>>     rbd map dne/dne # obtain a new osdmap as a side effect (!)
>>     sleep 1
>>     ceph osd in 0
>>     rbd resize --size 2 test
>>     # rbd info test | grep size -> 2M
>>     # blockdev --getsize $DEV -> 1M
>>
>> N.B.: Not obtaining a new osdmap in between "osd out" and "osd in"
>> above is enough to make it miss that resize notify, but that is a
>> bug^Wlimitation of ceph watch/notify v1.
>>
>> Second, homeless lingering requests are now kicked just like those
>> lingering requests whose mapping has changed.  This is mainly to
>> recognize that a homeless lingering request makes no sense and to
>> preserve the invariant that a registered lingering request is not
>> sitting on any of r_req_lru_item lists.  This spares us a WARN_ON,
>> which commit ba9d114ec557 ("libceph: clear r_req_lru_item in
>> __unregister_linger_request()") tried to fix the _wrong_ way.
>>
>> Cc: stable@vger.kernel.org # 3.10+
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> ---
>>  net/ceph/osd_client.c | 31 ++++++++++++++++++++-----------
>>  1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>> index 41a4abc7e98e..31d4b1ebff01 100644
>> --- a/net/ceph/osd_client.c
>> +++ b/net/ceph/osd_client.c
>> @@ -2017,20 +2017,29 @@ static void kick_requests(struct ceph_osd_client *osdc, bool force_resend,
>>               err = __map_request(osdc, req,
>>                                   force_resend || force_resend_writes);
>>               dout("__map_request returned %d\n", err);
>> -             if (err == 0)
>> -                     continue;  /* no change and no osd was specified */
>>               if (err < 0)
>>                       continue;  /* hrm! */
>> -             if (req->r_osd == NULL) {
>> -                     dout("tid %llu maps to no valid osd\n", req->r_tid);
>> -                     needmap++;  /* request a newer map */
>> -                     continue;
>> -             }
>> +             if (req->r_osd == NULL || err > 0) {
>> +                     if (req->r_osd == NULL) {
>> +                             dout("lingering %p tid %llu maps to no osd\n",
>> +                                  req, req->r_tid);
>> +                             /*
>> +                              * A homeless lingering request makes
>> +                              * no sense, as it's job is to keep
>> +                              * a particular OSD connection open.
>> +                              * Request a newer map and kick the
>> +                              * request, knowing that it won't be
>> +                              * resent until we actually get a map
>> +                              * that can tell us where to send it.
>> +                              */
>> +                             needmap++;
>> +                     }
>>
>> -             dout("kicking lingering %p tid %llu osd%d\n", req, req->r_tid,
>> -                  req->r_osd ? req->r_osd->o_osd : -1);
>> -             __register_request(osdc, req);
>> -             __unregister_linger_request(osdc, req);
>> +                     dout("kicking lingering %p tid %llu osd%d\n", req,
>> +                          req->r_tid, req->r_osd ? req->r_osd->o_osd : -1);
>> +                     __register_request(osdc, req);
>> +                     __unregister_linger_request(osdc, req);
>> +             }
>
> Am I misreading this, or could you accomplish the same thing by just
> dropping the 'continue' statement in the NULL check block?  No real
> opinion either way if this is a style change, just wondering...

No, if I had simply dropped continue I would have only achieved the
second (and secondary) objective, that is do a reregister dance for
homeless requests.  The reason is we do continue on err == 0 slightly
above, so we never get to the req->r_osd == NULL check.

Thanks,

                Ilya

Re: [PATCH 1/2] libceph: request a new osdmap if lingering request maps to no osd

[Sage Weil]

On Wed, 20 May 2015, Ilya Dryomov wrote:
> On Wed, May 20, 2015 at 8:29 PM, Sage Weil <sage@newdream.net> wrote:
> > On Wed, 13 May 2015, Ilya Dryomov wrote:
> >> This commit does two things.  First, if there are any homeless
> >> lingering requests, we now request a new osdmap even if the osdmap that
> >> is being processed brought no changes, i.e. if a given lingering
> >> request turned homeless in one of the previous epochs and remained
> >> homeless in the current epoch.  Not doing so leaves us with a stale
> >> osdmap and as a result we may miss our window for reestablishing the
> >> watch and lose notifies.
> >>
> >> MON=1 OSD=1:
> >>
> >>     # cat linger-needmap.sh
> >>     #!/bin/bash
> >>     rbd create --size 1 test
> >>     DEV=$(rbd map test)
> >>     ceph osd out 0
> >>     rbd map dne/dne # obtain a new osdmap as a side effect (!)
> >>     sleep 1
> >>     ceph osd in 0
> >>     rbd resize --size 2 test
> >>     # rbd info test | grep size -> 2M
> >>     # blockdev --getsize $DEV -> 1M
> >>
> >> N.B.: Not obtaining a new osdmap in between "osd out" and "osd in"
> >> above is enough to make it miss that resize notify, but that is a
> >> bug^Wlimitation of ceph watch/notify v1.
> >>
> >> Second, homeless lingering requests are now kicked just like those
> >> lingering requests whose mapping has changed.  This is mainly to
> >> recognize that a homeless lingering request makes no sense and to
> >> preserve the invariant that a registered lingering request is not
> >> sitting on any of r_req_lru_item lists.  This spares us a WARN_ON,
> >> which commit ba9d114ec557 ("libceph: clear r_req_lru_item in
> >> __unregister_linger_request()") tried to fix the _wrong_ way.
> >>
> >> Cc: stable@vger.kernel.org # 3.10+
> >> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> >> ---
> >>  net/ceph/osd_client.c | 31 ++++++++++++++++++++-----------
> >>  1 file changed, 20 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> >> index 41a4abc7e98e..31d4b1ebff01 100644
> >> --- a/net/ceph/osd_client.c
> >> +++ b/net/ceph/osd_client.c
> >> @@ -2017,20 +2017,29 @@ static void kick_requests(struct ceph_osd_client *osdc, bool force_resend,
> >>               err = __map_request(osdc, req,
> >>                                   force_resend || force_resend_writes);
> >>               dout("__map_request returned %d\n", err);
> >> -             if (err == 0)
> >> -                     continue;  /* no change and no osd was specified */
> >>               if (err < 0)
> >>                       continue;  /* hrm! */
> >> -             if (req->r_osd == NULL) {
> >> -                     dout("tid %llu maps to no valid osd\n", req->r_tid);
> >> -                     needmap++;  /* request a newer map */
> >> -                     continue;
> >> -             }
> >> +             if (req->r_osd == NULL || err > 0) {
> >> +                     if (req->r_osd == NULL) {
> >> +                             dout("lingering %p tid %llu maps to no osd\n",
> >> +                                  req, req->r_tid);
> >> +                             /*
> >> +                              * A homeless lingering request makes
> >> +                              * no sense, as it's job is to keep
> >> +                              * a particular OSD connection open.
> >> +                              * Request a newer map and kick the
> >> +                              * request, knowing that it won't be
> >> +                              * resent until we actually get a map
> >> +                              * that can tell us where to send it.
> >> +                              */
> >> +                             needmap++;
> >> +                     }
> >>
> >> -             dout("kicking lingering %p tid %llu osd%d\n", req, req->r_tid,
> >> -                  req->r_osd ? req->r_osd->o_osd : -1);
> >> -             __register_request(osdc, req);
> >> -             __unregister_linger_request(osdc, req);
> >> +                     dout("kicking lingering %p tid %llu osd%d\n", req,
> >> +                          req->r_tid, req->r_osd ? req->r_osd->o_osd : -1);
> >> +                     __register_request(osdc, req);
> >> +                     __unregister_linger_request(osdc, req);
> >> +             }
> >
> > Am I misreading this, or could you accomplish the same thing by just
> > dropping the 'continue' statement in the NULL check block?  No real
> > opinion either way if this is a style change, just wondering...
> 
> No, if I had simply dropped continue I would have only achieved the
> second (and secondary) objective, that is do a reregister dance for
> homeless requests.  The reason is we do continue on err == 0 slightly
> above, so we never get to the req->r_osd == NULL check.

Gotcha. 

Reviewed-by: Sage Weil <sage@redhat.com> 

Re: [PATCH 2/2] Revert "libceph: clear r_req_lru_item in __unregister_linger_request()"

[Sage Weil]

On Wed, 13 May 2015, Ilya Dryomov wrote:
> This reverts commit ba9d114ec5578e6e99a4dfa37ff8ae688040fd64.
> 
> .. which introduced a regression that prevented all lingering requests
> requeued in kick_requests() from ever being sent to the OSDs, resulting
> in a lot of missed notifies.  In retrospect it's pretty obvious that
> r_req_lru_item item in the case of lingering requests can be used not
> only for notarget, but also for unsent linkage due to how tightly
> actual map and enqueue operations are coupled in __map_request().
> 
> The assertion that was being silenced is taken care of in the previous
> ("libceph: request a new osdmap if lingering request maps to no osd")
> commit: by always kicking homeless lingering requests we ensure that
> none of them ends up on the notarget list outside of the critical
> section guarded by request_mutex.
> 
> Cc: stable@vger.kernel.org # 3.18+, needs 3d8c73c93605 "libceph: request a new osdmap if lingering request maps to no osd"
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Reviewed-by: Sage Weil <sage@redhat.com>



> ---
>  net/ceph/osd_client.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 31d4b1ebff01..c4ec9239249a 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -1306,8 +1306,6 @@ static void __unregister_linger_request(struct ceph_osd_client *osdc,
>  		if (list_empty(&req->r_osd_item))
>  			req->r_osd = NULL;
>  	}
> -
> -	list_del_init(&req->r_req_lru_item); /* can be on notarget */
>  	ceph_osdc_put_request(req);
>  }
>  
> -- 
> 1.9.3
> 
> --
> 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
> 
>