Return-Path: Received: from mail-ww0-f44.google.com ([74.125.82.44]:52244 "EHLO mail-ww0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755896Ab0IUTZJ convert rfc822-to-8bit (ORCPT ); Tue, 21 Sep 2010 15:25:09 -0400 Received: by wwi17 with SMTP id 17so69754wwi.1 for ; Tue, 21 Sep 2010 12:25:07 -0700 (PDT) In-Reply-To: <1285095654-32401-1-git-send-email-bhalevy@panasas.com> References: <1285095654-32401-1-git-send-email-bhalevy@panasas.com> Date: Tue, 21 Sep 2010 15:25:07 -0400 Message-ID: Subject: Re: [PATCH] SQUASHME: pnfs: get layout in proper segments. From: Fred Isaman To: Benny Halevy Cc: linux-nfs@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Tue, Sep 21, 2010 at 3:00 PM, Benny Halevy wrote: > Base the LAYOUTGET arguments on the actual required byte ranges > rather than asking for the whole file layout. > > Add a check in readpage_async_filler that the layout segment > retrieved in pnfs_pageio_init_read still covers the current page > and if not, try getting a new one. > > Signed-off-by: Benny Halevy > --- > ?fs/nfs/file.c | ? ?2 +- > ?fs/nfs/pnfs.c | ? 13 ++++++------- > ?fs/nfs/pnfs.h | ? 21 ++++++++------------- > ?fs/nfs/read.c | ? 17 ++++++++++++++++- > ?4 files changed, 31 insertions(+), 22 deletions(-) > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > index b05c1ff..228f41e 100644 > --- a/fs/nfs/file.c > +++ b/fs/nfs/file.c > @@ -390,7 +390,7 @@ static int nfs_write_begin(struct file *file, struct address_space *mapping, > > ? ? ? ?pnfs_update_layout(mapping->host, > ? ? ? ? ? ? ? ? ? ? ? ? ? nfs_file_open_context(file), > - ? ? ? ? ? ? ? ? ? ? ? ? ?0, NFS4_MAX_UINT64, IOMODE_RW, > + ? ? ? ? ? ? ? ? ? ? ? ? ?pos, len, IOMODE_RW, > ? ? ? ? ? ? ? ? ? ? ? ? ? &lseg); > ?start: > ? ? ? ?/* > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index e6261a3..de716f6 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -600,11 +600,10 @@ send_layoutget(struct inode *ino, > ? ? ? ? ? ? ? ?pnfs_layout_release(lo, NULL); > ? ? ? ? ? ? ? ?return -ENOMEM; > ? ? ? ?} > - ? ? ? lgp->args.minlength = NFS4_MAX_UINT64; > + ? ? ? lgp->args.minlength = PAGE_CACHE_SIZE - > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (range->offset & (PAGE_CACHE_SIZE-1)); > ? ? ? ?lgp->args.maxcount = PNFS_LAYOUT_MAXSIZE; > - ? ? ? lgp->args.range.iomode = range->iomode; > - ? ? ? lgp->args.range.offset = 0; > - ? ? ? lgp->args.range.length = NFS4_MAX_UINT64; > + ? ? ? lgp->args.range = *range; > ? ? ? ?lgp->args.type = server->pnfs_curr_ld->id; > ? ? ? ?lgp->args.inode = ino; > ? ? ? ?lgp->lsegpp = lsegpp; > @@ -1028,8 +1027,8 @@ _pnfs_update_layout(struct inode *ino, > ?{ > ? ? ? ?struct pnfs_layout_range arg = { > ? ? ? ? ? ? ? ?.iomode = iomode, > - ? ? ? ? ? ? ? .offset = 0, > - ? ? ? ? ? ? ? .length = NFS4_MAX_UINT64, > + ? ? ? ? ? ? ? .offset = pos, > + ? ? ? ? ? ? ? .length = count, > ? ? ? ?}; > ? ? ? ?struct nfs_inode *nfsi = NFS_I(ino); > ? ? ? ?struct pnfs_layout_hdr *lo; > @@ -1330,7 +1329,7 @@ pnfs_pageio_init_read(struct nfs_pageio_descriptor *pgio, > ? ? ? ?readahead_range(inode, pages, &loff, &count); > > ? ? ? ?if (count > 0) { > - ? ? ? ? ? ? ? _pnfs_update_layout(inode, ctx, loff, count, IOMODE_READ, > + ? ? ? ? ? ? ? pnfs_update_layout(inode, ctx, loff, count, IOMODE_READ, Why this change? the pnfs_enabled_sb check has already been done at the top of the function, hasn't it? > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&pgio->pg_lseg); > ? ? ? ? ? ? ? ?if (!pgio->pg_lseg) > ? ? ? ? ? ? ? ? ? ? ? ?return; > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > index 81534aa..b666f53 100644 > --- a/fs/nfs/pnfs.h > +++ b/fs/nfs/pnfs.h > @@ -188,19 +188,14 @@ static inline int pnfs_return_layout(struct inode *ino, > ? ? ? ?return 0; > ?} > > -static inline void pnfs_update_layout(struct inode *ino, > - ? ? ? struct nfs_open_context *ctx, > - ? ? ? loff_t pos, u64 count, enum pnfs_iomode access_type, > - ? ? ? struct pnfs_layout_segment **lsegpp) > -{ > - ? ? ? struct nfs_server *nfss = NFS_SERVER(ino); > - > - ? ? ? if (pnfs_enabled_sb(nfss)) > - ? ? ? ? ? ? ? _pnfs_update_layout(ino, ctx, pos, count, access_type, lsegpp); > - ? ? ? else { > - ? ? ? ? ? ? ? if (lsegpp) > - ? ? ? ? ? ? ? ? ? ? ? *lsegpp = NULL; > - ? ? ? } > +#define pnfs_update_layout(ino, ctx, pos, count, access_type, lsegpp) { \ > + ? ? ? if (pnfs_enabled_sb(NFS_SERVER(ino))) { \ > + ? ? ? ? ? ? ? dprintk("%s: updating %s layout pos %llu count %llu\n", __func__, \ > + ? ? ? ? ? ? ? ? ? ? ? (access_type) == IOMODE_READ ? "READ" : "WRITE", \ > + ? ? ? ? ? ? ? ? ? ? ? (unsigned long long)(pos), (unsigned long long)(count)); \ > + ? ? ? ? ? ? ? _pnfs_update_layout(ino, ctx, pos, count, access_type, lsegpp); \ > + ? ? ? } else \ > + ? ? ? ? ? ? ? *lsegpp = NULL; \ > ?} Why this change? I much prefer the inline version to the define, and I thought that this was generally being pushed. > > ?static inline int pnfs_get_write_status(struct nfs_write_data *data) > diff --git a/fs/nfs/read.c b/fs/nfs/read.c > index dde7996..36eef5e 100644 > --- a/fs/nfs/read.c > +++ b/fs/nfs/read.c > @@ -121,12 +121,14 @@ int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode, > ? ? ? ?LIST_HEAD(one_request); > ? ? ? ?struct nfs_page *new; > ? ? ? ?unsigned int len; > + ? ? ? loff_t pgoffs; > ? ? ? ?struct pnfs_layout_segment *lseg; > > ? ? ? ?len = nfs_page_length(page); > ? ? ? ?if (len == 0) > ? ? ? ? ? ? ? ?return nfs_return_empty_page(page); > - ? ? ? pnfs_update_layout(inode, ctx, 0, NFS4_MAX_UINT64, IOMODE_READ, &lseg); > + ? ? ? pgoffs = (loff_t)page->index << PAGE_CACHE_SHIFT; > + ? ? ? pnfs_update_layout(inode, ctx, pgoffs, len, IOMODE_READ, &lseg); > ? ? ? ?new = nfs_create_request(ctx, inode, page, 0, len, lseg); > ? ? ? ?put_lseg(lseg); > ? ? ? ?if (IS_ERR(new)) { > @@ -603,14 +605,27 @@ readpage_async_filler(void *data, struct page *page) > ?{ > ? ? ? ?struct nfs_readdesc *desc = (struct nfs_readdesc *)data; > ? ? ? ?struct inode *inode = page->mapping->host; > + ? ? ? struct pnfs_layout_range *range; > ? ? ? ?struct nfs_page *new; > ? ? ? ?unsigned int len; > + ? ? ? loff_t pgoff; > ? ? ? ?int error; > > ? ? ? ?len = nfs_page_length(page); > ? ? ? ?if (len == 0) > ? ? ? ? ? ? ? ?return nfs_return_empty_page(page); > > + ? ? ? pgoff = (loff_t)page->index << PAGE_CACHE_SHIFT; > + ? ? ? range = desc->pgio->pg_lseg ? &desc->pgio->pg_lseg->range : NULL; > + ? ? ? if (!range || > + ? ? ? ? ? (range->offset > pgoff + len) || > + ? ? ? ? ? (range->offset + range->length < pgoff)) { > + ? ? ? ? ? ? ? put_lseg(desc->pgio->pg_lseg); > + ? ? ? ? ? ? ? desc->pgio->pg_lseg = NULL; > + ? ? ? ? ? ? ? pnfs_update_layout(inode, desc->ctx, pgoff, len, IOMODE_READ, > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&desc->pgio->pg_lseg); > + ? ? ? } > + > ? ? ? ?new = nfs_create_request(desc->ctx, inode, page, 0, len, > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? desc->pgio->pg_lseg); > ? ? ? ?if (IS_ERR(new)) > -- Wouldn't it be easier to just trim any returned layout to page boundaries, and then pnfs_can_coalesce_requests will handle this automatically. Fred > 1.7.2.3 > > -- > 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 >