On 01/30/2013 01:43 PM, Josh Durgin wrote:
> On 01/26/2013 12:41 PM, Alex Elder wrote:
>> The new request code arranges to get a callback for every osd
>> request we submit (this was not the case previously).
>>
>> We register a lingering object watch request for the header object
>> for each mapped rbd image.
>>
>> If a connection problem occurs, the osd client will re-submit
>> lingering requests. And each time such a request is re-submitted,
>> its callback function will get called again.
>
> I think this should be fixed in the osd_client - rbd should only get
> the callback once, when the watch is first registered. Later we
> could add a separate callback to handle re-registration if we need to.
I agree. Even so, I would like to maintain a reference
to this lingering object request as is done in this patch.
I think it makes sense even if we'll never get another
callback.
I would like to therefore address the multiple callback
from the osd client as a separate issue. If I update
the comments here accordingly, and open a tracker issue
for the other thing, would that be OK with you?
-Alex
>> We therefore need to ensure the object request associated with the
>> lingering osd request stays valid, and the way to do that is to have
>> an extra reference to the lingering osd request.
>>
>> So when a request to initiate a watch has completed, do not drop a
>> reference as one normally would. Instead, hold off dropping that
>> reference until the request to tear down that watch request is done.
>>
>> Also, only set the rbd device's watch_request pointer after the
>> watch request has been completed successfully, and clear the
>> pointer once it's been torn down.
>>
>> Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
>> ---
>> drivers/block/rbd.c | 31 ++++++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 340773f..177ba0c 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -1716,6 +1716,7 @@ static int rbd_dev_header_watch_sync(struct
>> rbd_device *rbd_dev, int start)
>> &rbd_dev->watch_event);
>> if (ret < 0)
>> return ret;
>> + rbd_assert(rbd_dev->watch_event != NULL);
>> }
>>
>> ret = -ENOMEM;
>> @@ -1735,32 +1736,44 @@ static int rbd_dev_header_watch_sync(struct
>> rbd_device *rbd_dev, int start)
>> if (!obj_request->osd_req)
>> goto out_cancel;
>>
>> - if (start) {
>> + if (start)
>> ceph_osdc_set_request_linger(osdc, obj_request->osd_req);
>> - rbd_dev->watch_request = obj_request;
>> - } else {
>> + else
>> ceph_osdc_unregister_linger_request(osdc,
>> rbd_dev->watch_request->osd_req);
>> - rbd_dev->watch_request = NULL;
>> - }
>> ret = rbd_obj_request_submit(osdc, obj_request);
>> if (ret)
>> goto out_cancel;
>> ret = rbd_obj_request_wait(obj_request);
>> if (ret)
>> goto out_cancel;
>> -
>> ret = obj_request->result;
>> if (ret)
>> goto out_cancel;
>>
>> - if (start)
>> - goto done; /* Done if setting up the watch request */
>> + /*
>> + * Since a watch request is set to linger the osd client
>> + * will hang onto it in case it needs to be re-sent in the
>> + * event of connection loss. If we're initiating the watch
>> + * we therefore do *not* want to drop our reference to the
>> + * object request now; we'll effectively transfer ownership
>> + * of it to the osd client instead. Instead, we'll drop
>> + * that reference when the watch request gets torn down.
>> + */
>> + if (start) {
>> + rbd_dev->watch_request = obj_request;
>> +
>> + return 0;
>> + }
>> +
>> + /* We have successfully torn down the watch request */
>> +
>> + rbd_obj_request_put(rbd_dev->watch_request);
>> + rbd_dev->watch_request = NULL;
>> out_cancel:
>> /* Cancel the event if we're tearing down, or on error */
>> ceph_osdc_cancel_event(rbd_dev->watch_event);
>> rbd_dev->watch_event = NULL;
>> -done:
>> if (obj_request)
>> rbd_obj_request_put(obj_request);
>>
>
--
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
