Re: [PATCH 1/3] NFSv4.1: Fix some issues with pnfs_generic_pg_test

From: Trond Myklebust <Trond.Myklebust@netapp.com>
To: Benny Halevy <bhalevy@panasas.com>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/3] NFSv4.1: Fix some issues with pnfs_generic_pg_test
Date: Thu, 09 Jun 2011 12:31:09 -0400	[thread overview]
Message-ID: <1307637069.20245.8.camel@lade.trondhjem.org> (raw)
In-Reply-To: <4DEEDD59.5030902@panasas.com>

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.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com