[pnfs] [PATCH 01/25] pnfs: fix pnfs_{read|write}pages

[Benny Halevy]

On Feb. 15, 2008, 18:51 +0200, Fredric Isaman <iisaman at citi.umich.edu> wrote:
> 
> On Fri, 15 Feb 2008, Benny Halevy wrote:
> 
>> On Feb. 14, 2008, 20:21 +0200, Fred Isaman <iisaman at citi.umich.edu> wrote:
>>> THIS WILL BREAK OTHER LAYOUT DRIVERS that rely on old behavior.
>>>
>>> This patch fixes two basic problems:
>>> 1. It is impossible to fall back to nfs from the layout driver's
>>> read_pagelist or write_pagelist routines.
>>> 2. The pnfs.c code is assuming that if these routines return a non-zero
>>> status, that pnfs_writeback_done (or pnfs_read_done) was not called, which
>>> is not consistent with them returning count of bytes read/written.
>>>
>>> This patch alters the call chain, and expects
>>> ld_io_ops->{read|write}_pagelist to return either a 0 or 1, consistent
>>> with its parents, putting any other information that needs to be returned
>>> into the data.res and data.task structures.
>>>
>>> Signed-off-by: Fred Isaman <iisaman at citi.umich.edu>
>>> ---
>>>  fs/nfs/pnfs.c             |   31 ++++++++++++++++++++++---------
>>>  include/linux/nfs4_pnfs.h |   10 ++++++++--
>>>  2 files changed, 30 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index 3f34f34..5e12cd1 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -1364,6 +1364,11 @@ pnfs_writepages(struct nfs_write_data *wdata, int how)
>>>  	if (pnfs_get_type(inode) != LAYOUT_NFSV4_FILES)
>>>  		wdata->pnfsflags |= PNFS_NO_RPC;
>>>  	wdata->lseg = lseg;
>>> +	/* FRED - this should return just 0 (to indicate done for now)
>>> +	 * or 1 (to indicate try normal nfs).  It can indicate bytes
>>> +	 * written in wdata->res.count.  It can indicate error status in
>>> +	 * wdata->task.tk_status.
>>> +	 */
>> This behavior makes sense to me but the comment better be in include/linux/nfs4_pnfs.h
>> where the methods are defined rather than here.
>>
> 
> OK
> 
>>>  	status = nfss->pnfs_curr_ld->ld_io_ops->write_pagelist(
>>>  							nfsi->current_layout,
>>>  							args->pages,
>>> @@ -1374,13 +1379,15 @@ pnfs_writepages(struct nfs_write_data *wdata, int how)
>>>  							how,
>>>  							wdata);
>>>
>>> -	if (status)
>>> +	if (status) {
>>>  		put_lseg(lseg);
>>> -	if (status > 0) {
>>> -		dprintk("%s: LD write_pagelist returned status %d > 0\n", __FUNCTION__, status);
>>> -		pnfs_update_last_write(nfsi, args->offset, status);
>>> +		wdata->pnfsflags &= ~PNFS_NO_RPC;
>>> +	}
>>> +	if (status == 0 && wdata->res.count > 0) {
>> how about "} else if (wdata->res.count > 0) {" instead?
>>
> 
> I'm not sure I unserstand this comment.  My intention is that status==0 
> implies the ld_op->write_pagelist has handled the write, calling 
> pnfs_writeback_done, and set wdata->res fields appropriately.  On the 
> other hand status==1 implies the driver code has not altered anything (or 
> at least left it in a consistent state such that nfs_initiate_write will 
> work), not set any wdata->res codes, not called pnfs_writeback_done, 
> and now expects the code to fall through to nfs_initiate_write.

the logic is:
if (status) {
}
if (status == 0 && ...) {
}

hence the status == 0 part of the second term is just the else lag
if the first condition, therefore it is more efficient to write it
as follows, since there's no need to test status==0 again.

if (status) {
} else if (...) {
}

> 
>>> +		dprintk("%s: LD write_pagelist returned count %d > 0\n",
>>> +			__FUNCTION__, wdata->res.count);
>> minor nit: let's use __func__ rather than __FUNCTION__
>> See http://lkml.org/lkml/2008/1/28/169
> 
> 
> OK.  this is an issue throughout the entire patchset that I'll fix.

thanks!

> 
> 
>>> +		pnfs_update_last_write(nfsi, args->offset, wdata->res.count);
>>>  		pnfs_need_layoutcommit(nfsi, wdata->args.context);
>> Currently, in the sync completion case, where status > 0, we call put_lseg().
>> You expect the layout driver to always call back pnfs_writeback_done
>> (pnfs_client_ops->nfs_writelist_complete), right?
>>
> 
> Only if is returns status==0

OK. That makes sense.
<snio>

>>> @@ -1455,12 +1467,13 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>>  							(loff_t)args->offset,
>>>  							args->count,
>>>  							rdata);
>>> -	if (status)
>>> +	if (status) {
>>>  		put_lseg(lseg);
>>> -	if (status > 0) {
>>> -		dprintk("%s: LD read_pagelist returned status %d > 0\n", __FUNCTION__, status);
>>> -		status = 0;
>>> +		rdata->pnfsflags &= ~PNFS_NO_RPC;
>>>  	}
>>> +	if (status == 0 && rdata->res.count > 0)
>> ditto "} else if ()"
>>
>>> +		dprintk("%s: LD read_pagelist returned count %d > 0\n",
>>> +			__FUNCTION__, status);
>> ditto __FUNCTION__
>> s/status/rdata->res.count/

Just to make sure you saw this comment too :)
s/status/rdata->res.count/ in the dprintk() statement

Benny