by Myklebust, Trond

[permalink] [raw]

Subject: Re: [PATCH 2/3] NFS: Cleanup of the nfs_pageio code in preparation for a pnfs bugfix

On Tue, 2011-06-07 at 22:30 -0400, Benny Halevy wrote:
> On 2011-06-06 18:32, Trond Myklebust wrote:
> > We need to ensure that the layouts are set up before we can decide to
> > coalesce requests. To do so, we want to further split up the struct
> > nfs_pageio_descriptor operations into an initialisation callback, a
> > coalescing test callback, and a 'do i/o' callback.
> >
> > This patch cleans up the existing callback methods before adding the
> > 'initialisation' callback.
> >
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > fs/nfs/nfs4filelayout.c | 15 +++++++++++++--
> > fs/nfs/objlayout/objio_osd.c | 13 ++++++++++++-
> > fs/nfs/pagelist.c | 12 +++++-------
> > fs/nfs/pnfs.h | 29 ++++++++++++++++++++++-------
> > fs/nfs/read.c | 42 +++++++++++++++++++++++++++++++-----------
> > fs/nfs/write.c | 27 +++++++++++++++++++++++----
> > include/linux/nfs_page.h | 17 ++++++++++++++---
> > 7 files changed, 120 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> > index 4269088..e9b9b82 100644
> > --- a/fs/nfs/nfs4filelayout.c
> > +++ b/fs/nfs/nfs4filelayout.c
> > @@ -654,7 +654,7 @@ filelayout_alloc_lseg(struct pnfs_layout_hdr *layoutid,
> > * return true : coalesce page
> > * return false : don't coalesce page
> > */
> > -bool
> > +static bool
> > filelayout_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
> > struct nfs_page *req)
> > {
> > @@ -676,6 +676,16 @@ filelayout_pg_test(struct nfs_pageio_descriptor *pgio, struct nfs_page *prev,
> > return (p_stripe == r_stripe);
> > }
> >
> > +static const struct nfs_pageio_ops filelayout_pg_read_ops = {
> > + .pg_test = filelayout_pg_test,
> > + .pg_doio = nfs_generic_pg_readpages,
> > +};
> > +
> > +static const struct nfs_pageio_ops filelayout_pg_write_ops = {
> > + .pg_test = filelayout_pg_test,
> > + .pg_doio = nfs_generic_pg_writepages,
> > +};
> > +
> > static bool filelayout_mark_pnfs_commit(struct pnfs_layout_segment *lseg)
> > {
> > return !FILELAYOUT_LSEG(lseg)->commit_through_mds;
> > @@ -873,7 +883,8 @@ static struct pnfs_layoutdriver_type filelayout_type = {
> > .owner = THIS_MODULE,
> > .alloc_lseg = filelayout_alloc_lseg,
> > .free_lseg = filelayout_free_lseg,
> > - .pg_test = filelayout_pg_test,
> > + .pg_read_ops = &filelayout_pg_read_ops,
> > + .pg_write_ops = &filelayout_pg_write_ops,
> > .mark_pnfs_commit = filelayout_mark_pnfs_commit,
> > .choose_commit_list = filelayout_choose_commit_list,
> > .commit_pagelist = filelayout_commit_pagelist,
> > diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
> > index 4c41a60..31088f3 100644
> > --- a/fs/nfs/objlayout/objio_osd.c
> > +++ b/fs/nfs/objlayout/objio_osd.c
> > @@ -1008,6 +1008,16 @@ static bool objio_pg_test(struct nfs_pageio_descriptor *pgio,
> > OBJIO_LSEG(pgio->pg_lseg)->max_io_size;
> > }
> >
> > +static const struct nfs_pageio_ops objio_pg_read_ops = {
> > + .pg_test = objio_pg_test,
> > + .pg_doio = nfs_generic_pg_readpages,
> > +};
> > +
> > +static const struct nfs_pageio_ops objio_pg_write_ops = {
> > + .pg_test = objio_pg_test,
> > + .pg_doio = nfs_generic_pg_writepages,
>
> Hmm, for the non-files layouts we should ignore the server's wsize
> for pnfs I/O.
>
> How about using the follwing?
>
> int pnfs_generic_pg_readpages(struct nfs_pageio_descriptor *desc)
> {
> return desc->pg_lseg ? nfs_pagein_one(desc) : nfs_generic_pg_readpages(desc);
> }
> EXPORT_SYMBOL_GPL(pnfs_generic_pg_readpages);
>
> int pnfs_generic_pg_writepages(struct nfs_pageio_descriptor *desc)
> {
> return desc->pg_lseg ? nfs_flush_one(desc) : nfs_generic_pg_writepages(desc);
> }
> EXPORT_SYMBOL_GPL(pnfs_generic_pg_writepages);
>
> Benny

I'd prefer to have the objects and blocks set desc->pg_bsize correctly
according to their requirements. Otherwise, some of the tests make no
sense...

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com

2011-06-09 21:31:59

by Boaz Harrosh

[permalink] [raw]

Subject: Re: [PATCH 2/3] NFS: Cleanup of the nfs_pageio code in preparation for a pnfs bugfix

On 06/09/2011 11:58 AM, Benny Halevy wrote:
> On 2011-06-09 14:13, Trond Myklebust wrote:
>> On Thu, 2011-06-09 at 10:51 -0700, Boaz Harrosh wrote:
>>
>>> 1. We will then need to go through the ld in .pg_init
>>
>> No. I'll fix nfs_pageio_init_read() and friends by replacing
>> pnfs_get_read_ops() by something more appropriate.
>>
>
> OK, I just push the pnfs-block stuff out so you can see what I did
> over your previous version so I'll rebase over the new version once
> you send it.
>
>>> 2. What happens in the none pnfs-IO error case pg_bsize will need to be
>>> saved and restored to MDS value.
>>
>> Yes, but if you are falling back to read/write-through-mds, then you
>> need to re-run the coalescing _anyway_, since the total length of the
>> request needs to fit in an rsize/wsize sized request.
>>
>
> Right. So we will need a place holder to keep a separate value for
> minimum OSD's pg_bsize vs. the MDS's (like what we used to have for DS's
> [rw]size in the past)
>

There is no minimum OSD pg_bsize. Just ~0 would be good.

I still don't see what is the fuss? Why can't we just not care about
[rw]size in the LD case and files-layout can take care of itself?
Then pg_bsize is always the MDS value.

One thing I still did not understand in the files-layout case is
that if you are striping over, lets say 3 DSs. And each DS has a
limit of wsize. Than is it not allowed to write "3 * wsize" in total?

I understand that in the sparse case you will need to break up your
IO to a strip_unit each because of the gap in the addressing. but
what about the dense case. Or is it the opposite, I always get the
dense and the sparse mixed up.

> Benny
>
>>> 3. At least in objects there is no such constant limit, it all depends on
>>> the layout, start and end. I thought the all point of .pg_test was
>>> exactly for avoiding a constant pg_bsize. (This is what we had before)
>>> 4. All "the tests make no sense..." should be moved to the no-pnfs case
>>> please point these you found out, we'll need to fix them.
>>>
>>> Please understand that for none-files layouts pg_bsize is when IO goes
>>> through MDS only.
>>
>> As I said, I don't see how fallback to MDS can work correctly today for
>> the objects case for arbitrary values of rsize/wsize.
>>

That's fine then. For the error case we redo the coalescing with the ops
changed to generic nfs and we should be good right?

I'll be testing for these cases in Bakeathone with error injection patches.

>> Cheers
>> Trond

Thanks
Boaz