| Message ID | 1307399551-17489-1-git-send-email-Trond.Myklebust@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2011-06-06 18:32, Trond Myklebust wrote: > 1. If the intention is to coalesce requests 'prev' and 'req' then we > have to ensure at least that we have a layout starting at > req_offset(prev). > > 2. If we're only requesting a minimal layout of length desc->pg_count, > we need to test the length actually returned by the server before > we allow the coalescing to occur. > > 3. We need to deal correctly with (pgio->lseg == NULL) > > 4. Fixup the test guarding the pnfs_update_layout. > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > --- > fs/nfs/objlayout/objio_osd.c | 3 +++ > fs/nfs/pnfs.c | 12 +++++++----- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c > index 9cf208d..4c41a60 100644 > --- a/fs/nfs/objlayout/objio_osd.c > +++ b/fs/nfs/objlayout/objio_osd.c > @@ -1001,6 +1001,9 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio, > if (!pnfs_generic_pg_test(pgio, prev, req)) > return false; > > + if (pgio->pg_lseg == NULL) > + return true; > + > return pgio->pg_count + req->wb_bytes <= > OBJIO_LSEG(pgio->pg_lseg)->max_io_size; > } > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index 8c1309d..12b53ef 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -1059,19 +1059,21 @@ pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev, > gfp_flags = GFP_NOFS; > } > > - if (pgio->pg_count == prev->wb_bytes) { > + if (pgio->pg_lseg == NULL) { > + if (pgio->pg_count != prev->wb_bytes) > + return true; > /* This is first coelesce call for a series of nfs_pages */ > pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode, > prev->wb_context, > - req_offset(req), > + req_offset(prev), > pgio->pg_count, > access_type, > gfp_flags); > - return true; > + if (pgio->pg_lseg == NULL) > + return true; > } > > - if (pgio->pg_lseg && > - req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, > + if (req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, One more issue to fix: the condition should be for ">=", not ">" Benny > pgio->pg_lseg->pls_range.length)) > return false; > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-06-07 at 20:53 -0400, Benny Halevy wrote: > On 2011-06-06 18:32, Trond Myklebust wrote: > > 1. If the intention is to coalesce requests 'prev' and 'req' then we > > have to ensure at least that we have a layout starting at > > req_offset(prev). > > > > 2. If we're only requesting a minimal layout of length desc->pg_count, > > we need to test the length actually returned by the server before > > we allow the coalescing to occur. > > > > 3. We need to deal correctly with (pgio->lseg == NULL) > > > > 4. Fixup the test guarding the pnfs_update_layout. > > > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > > --- > > fs/nfs/objlayout/objio_osd.c | 3 +++ > > fs/nfs/pnfs.c | 12 +++++++----- > > 2 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c > > index 9cf208d..4c41a60 100644 > > --- a/fs/nfs/objlayout/objio_osd.c > > +++ b/fs/nfs/objlayout/objio_osd.c > > @@ -1001,6 +1001,9 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio, > > if (!pnfs_generic_pg_test(pgio, prev, req)) > > return false; > > > > + if (pgio->pg_lseg == NULL) > > + return true; > > + > > return pgio->pg_count + req->wb_bytes <= > > OBJIO_LSEG(pgio->pg_lseg)->max_io_size; > > } > > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > > index 8c1309d..12b53ef 100644 > > --- a/fs/nfs/pnfs.c > > +++ b/fs/nfs/pnfs.c > > @@ -1059,19 +1059,21 @@ pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev, > > gfp_flags = GFP_NOFS; > > } > > > > - if (pgio->pg_count == prev->wb_bytes) { > > + if (pgio->pg_lseg == NULL) { > > + if (pgio->pg_count != prev->wb_bytes) > > + return true; > > /* This is first coelesce call for a series of nfs_pages */ > > pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode, > > prev->wb_context, > > - req_offset(req), > > + req_offset(prev), > > pgio->pg_count, > > access_type, > > gfp_flags); > > - return true; > > + if (pgio->pg_lseg == NULL) > > + return true; > > } > > > > - if (pgio->pg_lseg && > > - req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, > > + if (req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, > > One more issue to fix: the condition should be for ">=", not ">" Hmm.... Shouldn't it rather be: if (end_offset(req_offset(req), req->wb_bytes) > end_offset(pgio->pg_lseg->pls_range.offset, pgio->pg_lseg->pls_range.length)) return false; Otherwise you don't know if the entire request fits in this layout segment... Cheers Trond
On 2011-06-07 21:12, Trond Myklebust wrote: > On Tue, 2011-06-07 at 20:53 -0400, Benny Halevy wrote: >> On 2011-06-06 18:32, Trond Myklebust wrote: >>> 1. If the intention is to coalesce requests 'prev' and 'req' then we >>> have to ensure at least that we have a layout starting at >>> req_offset(prev). >>> >>> 2. If we're only requesting a minimal layout of length desc->pg_count, >>> we need to test the length actually returned by the server before >>> we allow the coalescing to occur. >>> >>> 3. We need to deal correctly with (pgio->lseg == NULL) >>> >>> 4. Fixup the test guarding the pnfs_update_layout. >>> >>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> >>> --- >>> fs/nfs/objlayout/objio_osd.c | 3 +++ >>> fs/nfs/pnfs.c | 12 +++++++----- >>> 2 files changed, 10 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c >>> index 9cf208d..4c41a60 100644 >>> --- a/fs/nfs/objlayout/objio_osd.c >>> +++ b/fs/nfs/objlayout/objio_osd.c >>> @@ -1001,6 +1001,9 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio, >>> if (!pnfs_generic_pg_test(pgio, prev, req)) >>> return false; >>> >>> + if (pgio->pg_lseg == NULL) >>> + return true; >>> + >>> return pgio->pg_count + req->wb_bytes <= >>> OBJIO_LSEG(pgio->pg_lseg)->max_io_size; >>> } >>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c >>> index 8c1309d..12b53ef 100644 >>> --- a/fs/nfs/pnfs.c >>> +++ b/fs/nfs/pnfs.c >>> @@ -1059,19 +1059,21 @@ pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev, >>> gfp_flags = GFP_NOFS; >>> } >>> >>> - if (pgio->pg_count == prev->wb_bytes) { >>> + if (pgio->pg_lseg == NULL) { >>> + if (pgio->pg_count != prev->wb_bytes) >>> + return true; >>> /* This is first coelesce call for a series of nfs_pages */ >>> pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode, >>> prev->wb_context, >>> - req_offset(req), >>> + req_offset(prev), >>> pgio->pg_count, >>> access_type, >>> gfp_flags); >>> - return true; >>> + if (pgio->pg_lseg == NULL) >>> + return true; >>> } >>> >>> - if (pgio->pg_lseg && >>> - req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, >>> + if (req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, >> >> One more issue to fix: the condition should be for ">=", not ">" > > Hmm.... Shouldn't it rather be: > > if (end_offset(req_offset(req), req->wb_bytes) > end_offset(pgio->pg_lseg->pls_range.offset, > pgio->pg_lseg->pls_range.length)) > return false; > > Otherwise you don't know if the entire request fits in this layout > segment... True. Though since we make sure the layout segments are page aligned having the first offset covered is enough, this is the correct way to express the condition. Benny > > Cheers > Trond -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-06-07 at 22:24 -0400, Benny Halevy wrote: > On 2011-06-07 21:12, Trond Myklebust wrote: > > On Tue, 2011-06-07 at 20:53 -0400, Benny Halevy wrote: > >> On 2011-06-06 18:32, Trond Myklebust wrote: > >>> 1. If the intention is to coalesce requests 'prev' and 'req' then we > >>> have to ensure at least that we have a layout starting at > >>> req_offset(prev). > >>> > >>> 2. If we're only requesting a minimal layout of length desc->pg_count, > >>> we need to test the length actually returned by the server before > >>> we allow the coalescing to occur. > >>> > >>> 3. We need to deal correctly with (pgio->lseg == NULL) > >>> > >>> 4. Fixup the test guarding the pnfs_update_layout. > >>> > >>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > >>> --- > >>> fs/nfs/objlayout/objio_osd.c | 3 +++ > >>> fs/nfs/pnfs.c | 12 +++++++----- > >>> 2 files changed, 10 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c > >>> index 9cf208d..4c41a60 100644 > >>> --- a/fs/nfs/objlayout/objio_osd.c > >>> +++ b/fs/nfs/objlayout/objio_osd.c > >>> @@ -1001,6 +1001,9 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio, > >>> if (!pnfs_generic_pg_test(pgio, prev, req)) > >>> return false; > >>> > >>> + if (pgio->pg_lseg == NULL) > >>> + return true; > >>> + > >>> return pgio->pg_count + req->wb_bytes <= > >>> OBJIO_LSEG(pgio->pg_lseg)->max_io_size; > >>> } > >>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > >>> index 8c1309d..12b53ef 100644 > >>> --- a/fs/nfs/pnfs.c > >>> +++ b/fs/nfs/pnfs.c > >>> @@ -1059,19 +1059,21 @@ pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev, > >>> gfp_flags = GFP_NOFS; > >>> } > >>> > >>> - if (pgio->pg_count == prev->wb_bytes) { > >>> + if (pgio->pg_lseg == NULL) { > >>> + if (pgio->pg_count != prev->wb_bytes) > >>> + return true; > >>> /* This is first coelesce call for a series of nfs_pages */ > >>> pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode, > >>> prev->wb_context, > >>> - req_offset(req), > >>> + req_offset(prev), > >>> pgio->pg_count, > >>> access_type, > >>> gfp_flags); > >>> - return true; > >>> + if (pgio->pg_lseg == NULL) > >>> + return true; > >>> } > >>> > >>> - if (pgio->pg_lseg && > >>> - req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, > >>> + if (req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, > >> > >> One more issue to fix: the condition should be for ">=", not ">" > > > > Hmm.... Shouldn't it rather be: > > > > if (end_offset(req_offset(req), req->wb_bytes) > end_offset(pgio->pg_lseg->pls_range.offset, > > pgio->pg_lseg->pls_range.length)) > > return false; > > > > Otherwise you don't know if the entire request fits in this layout > > segment... > > True. > Though since we make sure the layout segments are page aligned > having the first offset covered is enough, this is the correct > way to express the condition. Ugh... That definitely would require a comment, and probably deserves a helper function of its own. Also, the name 'end_offset()' is rather confusing. It really is the offset of the first byte that lies _outside_ the actual layout segment.
On 2011-06-09 12:31, Trond Myklebust wrote: > On Tue, 2011-06-07 at 22:24 -0400, Benny Halevy wrote: >> On 2011-06-07 21:12, Trond Myklebust wrote: >>> On Tue, 2011-06-07 at 20:53 -0400, Benny Halevy wrote: >>>> On 2011-06-06 18:32, Trond Myklebust wrote: >>>>> 1. If the intention is to coalesce requests 'prev' and 'req' then we >>>>> have to ensure at least that we have a layout starting at >>>>> req_offset(prev). >>>>> >>>>> 2. If we're only requesting a minimal layout of length desc->pg_count, >>>>> we need to test the length actually returned by the server before >>>>> we allow the coalescing to occur. >>>>> >>>>> 3. We need to deal correctly with (pgio->lseg == NULL) >>>>> >>>>> 4. Fixup the test guarding the pnfs_update_layout. >>>>> >>>>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> >>>>> --- >>>>> fs/nfs/objlayout/objio_osd.c | 3 +++ >>>>> fs/nfs/pnfs.c | 12 +++++++----- >>>>> 2 files changed, 10 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c >>>>> index 9cf208d..4c41a60 100644 >>>>> --- a/fs/nfs/objlayout/objio_osd.c >>>>> +++ b/fs/nfs/objlayout/objio_osd.c >>>>> @@ -1001,6 +1001,9 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio, >>>>> if (!pnfs_generic_pg_test(pgio, prev, req)) >>>>> return false; >>>>> >>>>> + if (pgio->pg_lseg == NULL) >>>>> + return true; >>>>> + >>>>> return pgio->pg_count + req->wb_bytes <= >>>>> OBJIO_LSEG(pgio->pg_lseg)->max_io_size; >>>>> } >>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c >>>>> index 8c1309d..12b53ef 100644 >>>>> --- a/fs/nfs/pnfs.c >>>>> +++ b/fs/nfs/pnfs.c >>>>> @@ -1059,19 +1059,21 @@ pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev, >>>>> gfp_flags = GFP_NOFS; >>>>> } >>>>> >>>>> - if (pgio->pg_count == prev->wb_bytes) { >>>>> + if (pgio->pg_lseg == NULL) { >>>>> + if (pgio->pg_count != prev->wb_bytes) >>>>> + return true; >>>>> /* This is first coelesce call for a series of nfs_pages */ >>>>> pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode, >>>>> prev->wb_context, >>>>> - req_offset(req), >>>>> + req_offset(prev), >>>>> pgio->pg_count, >>>>> access_type, >>>>> gfp_flags); >>>>> - return true; >>>>> + if (pgio->pg_lseg == NULL) >>>>> + return true; >>>>> } >>>>> >>>>> - if (pgio->pg_lseg && >>>>> - req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, >>>>> + if (req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, >>>> >>>> One more issue to fix: the condition should be for ">=", not ">" >>> >>> Hmm.... Shouldn't it rather be: >>> >>> if (end_offset(req_offset(req), req->wb_bytes) > end_offset(pgio->pg_lseg->pls_range.offset, >>> pgio->pg_lseg->pls_range.length)) >>> return false; >>> >>> Otherwise you don't know if the entire request fits in this layout >>> segment... >> >> True. >> Though since we make sure the layout segments are page aligned >> having the first offset covered is enough, this is the correct >> way to express the condition. > > Ugh... That definitely would require a comment, and probably deserves a > helper function of its own. > Agreed. I'm totally with you on your proposal. > Also, the name 'end_offset()' is rather confusing. It really is the > offset of the first byte that lies _outside_ the actual layout segment. > Correct. I do not mind changing it to a better name if you have something in mind. Benny -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c index 9cf208d..4c41a60 100644 --- a/fs/nfs/objlayout/objio_osd.c +++ b/fs/nfs/objlayout/objio_osd.c @@ -1001,6 +1001,9 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio, if (!pnfs_generic_pg_test(pgio, prev, req)) return false; + if (pgio->pg_lseg == NULL) + return true; + return pgio->pg_count + req->wb_bytes <= OBJIO_LSEG(pgio->pg_lseg)->max_io_size; } diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index 8c1309d..12b53ef 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -1059,19 +1059,21 @@ pnfs_generic_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev, gfp_flags = GFP_NOFS; } - if (pgio->pg_count == prev->wb_bytes) { + if (pgio->pg_lseg == NULL) { + if (pgio->pg_count != prev->wb_bytes) + return true; /* This is first coelesce call for a series of nfs_pages */ pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode, prev->wb_context, - req_offset(req), + req_offset(prev), pgio->pg_count, access_type, gfp_flags); - return true; + if (pgio->pg_lseg == NULL) + return true; } - if (pgio->pg_lseg && - req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, + if (req_offset(req) > end_offset(pgio->pg_lseg->pls_range.offset, pgio->pg_lseg->pls_range.length)) return false;
1. If the intention is to coalesce requests 'prev' and 'req' then we have to ensure at least that we have a layout starting at req_offset(prev). 2. If we're only requesting a minimal layout of length desc->pg_count, we need to test the length actually returned by the server before we allow the coalescing to occur. 3. We need to deal correctly with (pgio->lseg == NULL) 4. Fixup the test guarding the pnfs_update_layout. Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> --- fs/nfs/objlayout/objio_osd.c | 3 +++ fs/nfs/pnfs.c | 12 +++++++----- 2 files changed, 10 insertions(+), 5 deletions(-)