[PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance

[PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance

[Jeff Layton]

It's possible we'll have less than a page's worth of residual data, that
is stradding the last two pages in the array. That will make it
incorrectly set the last_piece boolean when it shouldn't.

Account for a non-zero cursor->page_offset when advancing.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/ceph/messenger.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index d3bb656308b4..3f8453773cc8 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -894,10 +894,9 @@ static bool ceph_msg_data_pages_advance(struct ceph_msg_data_cursor *cursor,
 		return false;   /* no more data */
 
 	/* Move on to the next page; offset is already at 0 */
-
 	BUG_ON(cursor->page_index >= cursor->page_count);
 	cursor->page_index++;
-	cursor->last_piece = cursor->resid <= PAGE_SIZE;
+	cursor->last_piece = cursor->page_offset + cursor->resid <= PAGE_SIZE;
 
 	return true;
 }
-- 
2.35.1

Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance

[Alex Elder]

On 3/2/22 9:37 AM, Jeff Layton wrote:
> It's possible we'll have less than a page's worth of residual data, that
> is stradding the last two pages in the array. That will make it
> incorrectly set the last_piece boolean when it shouldn't.
> 
> Account for a non-zero cursor->page_offset when advancing.

It's been quite a while I looked at this code, but isn't
cursor->resid supposed to be the number of bytes remaining,
irrespective of the offset?

Have you found this to cause a failure?

					-Alex

> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   net/ceph/messenger.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index d3bb656308b4..3f8453773cc8 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -894,10 +894,9 @@ static bool ceph_msg_data_pages_advance(struct ceph_msg_data_cursor *cursor,
>   		return false;   /* no more data */
>   
>   	/* Move on to the next page; offset is already at 0 */
> -
>   	BUG_ON(cursor->page_index >= cursor->page_count);
>   	cursor->page_index++;
> -	cursor->last_piece = cursor->resid <= PAGE_SIZE;
> +	cursor->last_piece = cursor->page_offset + cursor->resid <= PAGE_SIZE;
>   
>   	return true;
>   }

Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance

[Jeff Layton]

On Wed, 2022-03-02 at 09:41 -0600, Alex Elder wrote:
> On 3/2/22 9:37 AM, Jeff Layton wrote:
> > It's possible we'll have less than a page's worth of residual data, that
> > is stradding the last two pages in the array. That will make it
> > incorrectly set the last_piece boolean when it shouldn't.
> > 
> > Account for a non-zero cursor->page_offset when advancing.
> 
> It's been quite a while I looked at this code, but isn't
> cursor->resid supposed to be the number of bytes remaining,
> irrespective of the offset?
> 

Correct. The "residual" bytes in the cursor, AFAICT.

> Have you found this to cause a failure?
> 

Not with the existing code in libceph, as it only ever seems to advance
to the end of the page. I'm working on some patches to allow for sparse
reads though, and with those in place I need to sometimes advance to
arbitrary positions in the array, and this reliably causes a BUG_ON() to
trip.



> 					-Alex
> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   net/ceph/messenger.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> > index d3bb656308b4..3f8453773cc8 100644
> > --- a/net/ceph/messenger.c
> > +++ b/net/ceph/messenger.c
> > @@ -894,10 +894,9 @@ static bool ceph_msg_data_pages_advance(struct ceph_msg_data_cursor *cursor,
> >   		return false;   /* no more data */
> >   
> >   	/* Move on to the next page; offset is already at 0 */
> > -
> >   	BUG_ON(cursor->page_index >= cursor->page_count);
> >   	cursor->page_index++;
> > -	cursor->last_piece = cursor->resid <= PAGE_SIZE;
> > +	cursor->last_piece = cursor->page_offset + cursor->resid <= PAGE_SIZE;
> >   
> >   	return true;
> >   }
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance

[Ilya Dryomov]

On Wed, Mar 2, 2022 at 5:15 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2022-03-02 at 09:41 -0600, Alex Elder wrote:
> > On 3/2/22 9:37 AM, Jeff Layton wrote:
> > > It's possible we'll have less than a page's worth of residual data, that
> > > is stradding the last two pages in the array. That will make it
> > > incorrectly set the last_piece boolean when it shouldn't.
> > >
> > > Account for a non-zero cursor->page_offset when advancing.
> >
> > It's been quite a while I looked at this code, but isn't
> > cursor->resid supposed to be the number of bytes remaining,
> > irrespective of the offset?
> >
>
> Correct. The "residual" bytes in the cursor, AFAICT.
>
> > Have you found this to cause a failure?
> >
>
> Not with the existing code in libceph, as it only ever seems to advance
> to the end of the page. I'm working on some patches to allow for sparse
> reads though, and with those in place I need to sometimes advance to
> arbitrary positions in the array, and this reliably causes a BUG_ON() to
> trip.

Hi Jeff,

Which BUG_ON?  Can you explain what "advance to arbitrary positions in
the array" means?

I think you may be misusing ceph_msg_data_pages_advance() because
cursor->page_offset _has_ to be zero at that point: we have just
determined that we are done with the current page and incremented
cursor->page_index (i.e. moved to the next page).  The way we
determine whether we are done with the current page is by testing
cursor->page_offset == 0, so either your change is a no-op or
something else is broken.

Thanks,

                Ilya

Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance

[Jeff Layton]

On Wed, 2022-03-02 at 18:03 +0100, Ilya Dryomov wrote:
> On Wed, Mar 2, 2022 at 5:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Wed, 2022-03-02 at 09:41 -0600, Alex Elder wrote:
> > > On 3/2/22 9:37 AM, Jeff Layton wrote:
> > > > It's possible we'll have less than a page's worth of residual data, that
> > > > is stradding the last two pages in the array. That will make it
> > > > incorrectly set the last_piece boolean when it shouldn't.
> > > > 
> > > > Account for a non-zero cursor->page_offset when advancing.
> > > 
> > > It's been quite a while I looked at this code, but isn't
> > > cursor->resid supposed to be the number of bytes remaining,
> > > irrespective of the offset?
> > > 
> > 
> > Correct. The "residual" bytes in the cursor, AFAICT.
> > 
> > > Have you found this to cause a failure?
> > > 
> > 
> > Not with the existing code in libceph, as it only ever seems to advance
> > to the end of the page. I'm working on some patches to allow for sparse
> > reads though, and with those in place I need to sometimes advance to
> > arbitrary positions in the array, and this reliably causes a BUG_ON() to
> > trip.
> 
> Hi Jeff,
> 
> Which BUG_ON?  Can you explain what "advance to arbitrary positions in
> the array" means?
> 
> I think you may be misusing ceph_msg_data_pages_advance() because
> cursor->page_offset _has_ to be zero at that point: we have just
> determined that we are done with the current page and incremented
> cursor->page_index (i.e. moved to the next page).  The way we
> determine whether we are done with the current page is by testing
> cursor->page_offset == 0, so either your change is a no-op or
> something else is broken.
> 

My apologies, I've got a stack of other patches sitting on top of this,
and this patch should have been folded into one of them instead of being
sent separately.

In the sparse_read code I have now, I'm creating some temporary
ceph_msg_data_cursors on the stack and revamped the "advance" code to
allow advancing an arbirtrary amount instead of only allowing it to go
to the end of the current page. This fix is necessary with those
changes.

I'm not sure I'm going to need that going forward though, as I may
rework the patches (again) to use the cursor in the con, and in that
case I'll only need to walk through it a page at a time like we do now.

We can drop this patch.
--
Jeff Layton <jlayton@kernel.org>