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

[Jeff Layton <jlayton@kernel.org>]

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>