On Tue, 2011-02-15 at 09:41 -0500, Fred Isaman wrote:
> On Mon, Feb 14, 2011 at 6:14 PM, Trond Myklebust
> <Trond.Myklebust@xxxxxxxxxx> wrote:
> > On Mon, 2011-02-14 at 14:18 -0500, andros@xxxxxxxxxx wrote:
> >> From: Fred Isaman <iisaman@xxxxxxxxxx>
> >>
> >> Move the pnfs_update_layout call location to nfs_pageio_do_add_request().
> >> Grab the lseg sent in the doio function to nfs_read_rpcsetup and attach
> >> it to each nfs_read_data so it can be sent to the layout driver.
> >>
> >> Signed-off-by: Andy Adamon <andros@xxxxxxxxxx>
> >> Signed-off-by: Andy Adamon <andros@xxxxxxxxxxxxxx>
> >> Signed-off-by: Dean Hildebrand <dhildeb@xxxxxxxxxx>
> >> Signed-off-by: Fred Isaman <iisaman@xxxxxxxxxxxxxx>
> >> Signed-off-by: Fred Isaman <iisaman@xxxxxxxxxx>
> >> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> >> Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
> >> Signed-off-by: Oleg Drokin <green@xxxxxxxxxxxxxx>
> >> Signed-off-by: Tao Guo <guotao@xxxxxxxxxxxx>
> >> ---
> >> fs/nfs/file.c | 4 ----
> >> fs/nfs/pagelist.c | 15 ++++++++++++---
> >> fs/nfs/pnfs.c | 4 ++--
> >> fs/nfs/pnfs.h | 1 +
> >> fs/nfs/read.c | 28 ++++++++++++++++------------
> >> fs/nfs/write.c | 4 ++--
> >> include/linux/nfs_page.h | 5 +++--
> >> include/linux/nfs_xdr.h | 1 +
> >> 8 files changed, 37 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> >> index 7bf029e..d85a534 100644
> >> --- a/fs/nfs/file.c
> >> +++ b/fs/nfs/file.c
> >> @@ -387,10 +387,6 @@ static int nfs_write_begin(struct file *file, struct address_space *mapping,
> >> file->f_path.dentry->d_name.name,
> >> mapping->host->i_ino, len, (long long) pos);
> >>
> >> - pnfs_update_layout(mapping->host,
> >> - nfs_file_open_context(file),
> >> - IOMODE_RW);
> >> -
> >> start:
> >> /*
> >> * Prevent starvation issues if someone is doing a consistency
> >> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> >> index e1164e3..e0a0cb4 100644
> >> --- a/fs/nfs/pagelist.c
> >> +++ b/fs/nfs/pagelist.c
> >> @@ -20,6 +20,7 @@
> >> #include <linux/nfs_mount.h>
> >>
> >> #include "internal.h"
> >> +#include "pnfs.h"
> >>
> >> static struct kmem_cache *nfs_page_cachep;
> >>
> >> @@ -213,7 +214,7 @@ nfs_wait_on_request(struct nfs_page *req)
> >> */
> >> void nfs_pageio_init(struct nfs_pageio_descriptor *desc,
> >> struct inode *inode,
> >> - int (*doio)(struct inode *, struct list_head *, unsigned int, size_t, int),
> >> + int (*doio)(struct inode *, struct list_head *, unsigned int, size_t, int, struct pnfs_layout_segment *),
> >> size_t bsize,
> >> int io_flags)
> >> {
> >> @@ -226,6 +227,7 @@ void nfs_pageio_init(struct nfs_pageio_descriptor *desc,
> >> desc->pg_doio = doio;
> >> desc->pg_ioflags = io_flags;
> >> desc->pg_error = 0;
> >> + desc->pg_lseg = NULL;
> >> }
> >>
> >> /**
> >> @@ -288,8 +290,13 @@ static int nfs_pageio_do_add_request(struct nfs_pageio_descriptor *desc,
> >> prev = nfs_list_entry(desc->pg_list.prev);
> >> if (!nfs_can_coalesce_requests(prev, req))
> >> return 0;
> >> - } else
> >> + } else {
> >> + put_lseg(desc->pg_lseg);
> >> desc->pg_base = req->wb_pgbase;
> >> + desc->pg_lseg = pnfs_update_layout(desc->pg_inode,
> >> + req->wb_context,
> >> + IOMODE_READ);
> >
> > Looking at this afresh after a week of vacation. Isn't it more natural
> > to do this as part of the pg_doio() callback?
> >
> > Your only reason for introducing the ->pg_lseg pointer is to be able to
> > pass it to the ->pg_doio() in the first place. Why not do that by simply
> > passing the 'desc' pointer to ->pg_doio(), and then having it call
> > pnfs_update_layout() instead of 'get_layout()'?
> >
>
> The problem is that it is not the only reason. Passing the lseg into
> the nfs_can_coalesce_requests is another. Calling pnfs_update_layout
> in ->pg_doio would be eliminate the opportunity to have a say in
> coalescing based on the layout.
The point is that you are adding intimate knowledge of layouts to sites
like nfs_pageio_do_add_request and nfs_pageio_complete, and then on top
of that adding callbacks whose sole purpose is to support layouts.
A better approach is to keep the layouts in the callbacks (i.e. pg_test
and pg_doio). I don't care if you cache the layout in a pg_lseg field,
but I do object to the proliferation of layout knowledge in places where
we don't need it.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
