Re: [PATCH 5/8] libceph: define and use in_msg_pos_next()

[Josh Durgin <josh.durgin@inktank.com>]

On 03/11/2013 12:16 PM, Alex Elder wrote:
> On 03/11/2013 01:57 PM, Josh Durgin wrote:
>> On 03/09/2013 07:14 AM, Alex Elder wrote:
>>> Define a new function in_msg_pos_next() to match out_msg_pos_next(),
>>> and use it in place of code at the end of read_partial_message_pages()
>>> and read_partial_message_bio().
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    net/ceph/messenger.c |   57
>>> ++++++++++++++++++++++++++++++++------------------
>>>    1 file changed, 37 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
>>> index 2017b88..fb5f6e7 100644
>>> --- a/net/ceph/messenger.c
>>> +++ b/net/ceph/messenger.c
>>> @@ -1052,6 +1052,28 @@ static void out_msg_pos_next(struct
>>> ceph_connection *con, struct page *page,
>>>    #endif
>>>    }
>>>
>>> +static void in_msg_pos_next(struct ceph_connection *con, size_t len,
>>> +                size_t received)
>>> +{
>>> +    struct ceph_msg *msg = con->in_msg;
>>> +
>>> +    BUG_ON(!msg);
>>> +    BUG_ON(!received);
>>> +
>>> +    con->in_msg_pos.data_pos += received;
>>> +    con->in_msg_pos.page_pos += received;
>>> +    if (received < len)
>>> +        return;
>>
>> This is different from the condition in read_partial_message_pages()
>> case, which only increments in_msg_pos.page and resets page_pos when
>> page_pos == PAGE_SIZE. Maybe this is equivalent, but if so it's not
>> obvious.
>
> You are correct.  And, despite my usual thoroughness and
> verbosity, I didn't explain this one.
>
> This message position structure (in_msg_pos) is tracking
> how much of the message has been consumed.  Each time a
> an incoming message is going to arrive, we find out how
> much room is left--not surpassing the current page--and
> provide that as the "len" (number of bytes) to receive:
>
>        left = min((int)(data_len - con->in_msg_pos.data_pos),
>               (int)(bv->bv_len - con->in_msg_pos.page_pos));
>
> This is saying the amount we'll use is the lesser of:
> - all that's left of the entire request
> - all that's left in the current page
>
> The number of bytes received is then used, along with
> the "len" value, to determine how to advance the position.
> If we received exactly how many were requested, we either
> reached the end of the request or the end of the page.
> In the first case, we're done, in the second, we move
> onto the next page in the array.
>
> In all cases but (possibly) on the last page, after adding
> the number of bytes received, page_pos == PAGE_SIZE.  On the
> last page, it doesn't really matter whether we increment
> the page number and reset the page position, because we're
> done and we won't come back here again.  The code previously
> skipped over that last case, basically.  The new code
> handles that case the same as the others, incrementing
> and resetting.
>
> So the short answer is, they are still equivalent.  And
> I agree, it is not obvious.
>
> If you are satisfied, I can add the above (or maybe an
> abbreviated form of it) to the commit message.

Thanks for the explanation, that makes sense now.
It'd be nice to have some form of it in the commit message.

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