On Wed, May 20, 2015 at 8:29 PM, Sage Weil <sage@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx # 3.10+
>> Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
