On 08/08/2014 06:18 AM, Ilya Dryomov wrote:
> Determining ->last_piece based on the value of ->page_offset + length
> is incorrect because length here is the length of the entire message.
> ->last_piece set to false even if page array data item length is <=
> PAGE_SIZE, which results in invalid length passed to
> ceph_tcp_{send,recv}page() and causes various asserts to fire.I glanced at this this morning and thought it might have a problem. Now that I've had the time to look at it a little more closely I conclude it's fine. I'm going to explain my reasoning, which just reaffirms what you've explained. (Doing this helps me brush up on things, I'm starting to get rusty...) A message has a list of data items, and a data item is broken into pieces, each of which (for a pages data item) holds data from a single page. A cursor keeps the data item now being processed in cursor->data, and cursor->resid is the number of un-consumed bytes in the current data item. We want to set cursor->last_piece when the next piece to process is the last in the *data item*, not the last in the *message*, so we need to compare against cursor->resid, not the message length. Before your change, any message with more than one data item including a page array data item that was not last in the list would have problems. So in summary: Looks good. Reviewed-by: Alex Elder <el...@linaro.org> > # cat pages-cursor-init.sh > #!/bin/bash > rbd create --size 10 --image-format 2 foo > FOO_DEV=$(rbd map foo) > dd if=/dev/urandom of=$FOO_DEV bs=1M &>/dev/null > rbd snap create foo@snap > rbd snap protect foo@snap > rbd clone foo@snap bar > # rbd_resize calls librbd rbd_resize(), size is in bytes > ./rbd_resize bar $(((4 << 20) + 512)) > rbd resize --size 10 bar > BAR_DEV=$(rbd map bar) > # trigger a 512-byte copyup -- 512-byte page array data item > dd if=/dev/urandom of=$BAR_DEV bs=1M count=1 seek=5 > > The problem exists only in ceph_msg_data_pages_cursor_init(), > ceph_msg_data_pages_advance() does the right thing. The size_t cast is > unnecessary. > > Signed-off-by: Ilya Dryomov <ilya.dryo...@inktank.com> > --- > net/ceph/messenger.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c > index e51cad0db580..b2f571dd933d 100644 > --- a/net/ceph/messenger.c > +++ b/net/ceph/messenger.c > @@ -901,7 +901,7 @@ static void ceph_msg_data_pages_cursor_init(struct > ceph_msg_data_cursor *cursor, > BUG_ON(page_count > (int)USHRT_MAX); > cursor->page_count = (unsigned short)page_count; > BUG_ON(length > SIZE_MAX - cursor->page_offset); > - cursor->last_piece = (size_t)cursor->page_offset + length <= PAGE_SIZE; > + cursor->last_piece = cursor->page_offset + cursor->resid <= PAGE_SIZE; > } > > static struct page * > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
