* [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance @ 2022-03-02 15:37 Jeff Layton 2022-03-02 15:41 ` Alex Elder 0 siblings, 1 reply; 5+ messages in thread From: Jeff Layton @ 2022-03-02 15:37 UTC (permalink / raw) To: ceph-devel; +Cc: idryomov, xiubli 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance 2022-03-02 15:37 [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance Jeff Layton @ 2022-03-02 15:41 ` Alex Elder 2022-03-02 16:15 ` Jeff Layton 0 siblings, 1 reply; 5+ messages in thread From: Alex Elder @ 2022-03-02 15:41 UTC (permalink / raw) To: Jeff Layton, ceph-devel; +Cc: idryomov, xiubli 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; > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance 2022-03-02 15:41 ` Alex Elder @ 2022-03-02 16:15 ` Jeff Layton 2022-03-02 17:03 ` Ilya Dryomov 0 siblings, 1 reply; 5+ messages in thread From: Jeff Layton @ 2022-03-02 16:15 UTC (permalink / raw) To: Alex Elder, ceph-devel; +Cc: idryomov, xiubli 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance 2022-03-02 16:15 ` Jeff Layton @ 2022-03-02 17:03 ` Ilya Dryomov 2022-03-02 18:12 ` Jeff Layton 0 siblings, 1 reply; 5+ messages in thread From: Ilya Dryomov @ 2022-03-02 17:03 UTC (permalink / raw) To: Jeff Layton; +Cc: Alex Elder, Ceph Development, Xiubo Li 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance 2022-03-02 17:03 ` Ilya Dryomov @ 2022-03-02 18:12 ` Jeff Layton 0 siblings, 0 replies; 5+ messages in thread From: Jeff Layton @ 2022-03-02 18:12 UTC (permalink / raw) To: Ilya Dryomov; +Cc: Alex Elder, Ceph Development, Xiubo Li 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-02 18:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-02 15:37 [PATCH] libceph: fix last_piece calculation in ceph_msg_data_pages_advance Jeff Layton 2022-03-02 15:41 ` Alex Elder 2022-03-02 16:15 ` Jeff Layton 2022-03-02 17:03 ` Ilya Dryomov 2022-03-02 18:12 ` Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.