On Wed, 2022-03-02 at 18:03 +0100, Ilya Dryomov wrote: > On Wed, Mar 2, 2022 at 5:15 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
