On 06/09/2010 10:29 PM, Fred Isaman wrote:
> On Wed, Jun 9, 2010 at 3:19 PM, Boaz Harrosh <[email protected]> wrote:
>> On 06/08/2010 07:19 AM, Fred Isaman wrote:
>>> Change readpages path to only call LAYOUTGET once.
>>>
>>> Signed-off-by: Fred Isaman <[email protected]>
>>> ---
>>> fs/nfs/pagelist.c | 2 ++
>>> fs/nfs/pnfs.c | 37 +++++++------------------------------
>>> fs/nfs/pnfs.h | 25 ++++++++++++++++---------
>>> 3 files changed, 25 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
>>> index ed647b9..c3e5a1f 100644
>>> --- a/fs/nfs/pagelist.c
>>> +++ b/fs/nfs/pagelist.c
>>> @@ -253,6 +253,8 @@ static int nfs_can_coalesce_requests(struct nfs_page *prev,
>>> return 0;
>>> if (prev->wb_pgbase + prev->wb_bytes != PAGE_CACHE_SIZE)
>>> return 0;
>>> + if (req->wb_lseg != prev->wb_lseg)
>>> + return 0;
>>> #ifdef CONFIG_NFS_V4_1
>>> if (pgio->pg_test && !pgio->pg_test(pgio, prev, req))
>>> return 0;
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index 2b5f6fc..692a18e 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -1689,7 +1689,7 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>> {
>>> struct nfs_readargs *args = &rdata->args;
>>> struct inode *inode = rdata->inode;
>>> - int numpages, status, pgcount, temp;
>>> + int numpages, pgcount, temp;
>>> struct nfs_server *nfss = NFS_SERVER(inode);
>>> struct nfs_inode *nfsi = NFS_I(inode);
>>> struct pnfs_layout_segment *lseg;
>>> @@ -1701,19 +1701,8 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>> args->count,
>>> args->offset);
>>>
>>> - /* Retrieve and set layout if not allready cached */
>>> - status = _pnfs_update_layout(inode,
>>> - args->context,
>>> - args->count,
>>> - args->offset,
>>> - IOMODE_READ,
>>> - &lseg);
>>> - if (status) {
>>> - dprintk("%s: Updating layout failed (%d), retry with NFS \n",
>>> - __func__, status);
>>> - trypnfs = PNFS_NOT_ATTEMPTED;
>>> - goto out;
>>> - }
>>> + lseg = rdata->req->wb_lseg;
>>> + get_lseg(lseg);
>>>
>>> /* Determine number of pages. */
>>> pgcount = args->pgbase + args->count;
>>> @@ -1740,7 +1729,6 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>> rdata->pdata.lseg = NULL;
>>> put_lseg(lseg);
>>> }
>>> - out:
>>> dprintk("%s End (trypnfs:%d)\n", __func__, trypnfs);
>>> return trypnfs;
>>> }
>>> @@ -1749,21 +1737,10 @@ enum pnfs_try_status
>>> _pnfs_try_to_read_data(struct nfs_read_data *data,
>>> const struct rpc_call_ops *call_ops)
>>> {
>>> - struct inode *ino = data->inode;
>>> - struct nfs_server *nfss = NFS_SERVER(ino);
>>> -
>>> - dprintk("--> %s\n", __func__);
>>> - /* Only create an rpc request if utilizing NFSv4 I/O */
>>> - if (!pnfs_enabled_sb(nfss) ||
>>> - !nfss->pnfs_curr_ld->ld_io_ops->read_pagelist) {
>>> - dprintk("<-- %s: not using pnfs\n", __func__);
>>> - return PNFS_NOT_ATTEMPTED;
>>> - } else {
>>> - dprintk("%s: Utilizing pNFS I/O\n", __func__);
>>> - data->pdata.call_ops = call_ops;
>>> - data->pdata.pnfs_error = 0;
>>> - return pnfs_readpages(data);
>>> - }
>>
>> Wahoo, nice stuff Ha
>>
>> By now this can go into it's only caller.
>> (And caller de-inlined, what was that all about)
>>
>>> + dprintk("%s: Utilizing pNFS I/O\n", __func__);
>>> + data->pdata.call_ops = call_ops;
>>> + data->pdata.pnfs_error = 0;
>>> + return pnfs_readpages(data);
>>> }
>>>
>>> enum pnfs_try_status
>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>>> index 6326ed5..816ebe1 100644
>>> --- a/fs/nfs/pnfs.h
>>> +++ b/fs/nfs/pnfs.h
>>> @@ -94,22 +94,29 @@ static inline int pnfs_enabled_sb(struct nfs_server *nfss)
>>> return nfss->pnfs_curr_ld != NULL;
>>> }
>>>
>>> +static inline void _pnfs_clear_lseg_from_pages(struct list_head *head)
>>> +{
>>> + struct nfs_page *req;
>>> +
>>> + list_for_each_entry(req, head, wb_list) {
>>> + put_lseg(req->wb_lseg);
>>> + req->wb_lseg = NULL;
>>> + }
>>> +}
>>> +
>>> static inline enum pnfs_try_status
>>> pnfs_try_to_read_data(struct nfs_read_data *data,
>>> const struct rpc_call_ops *call_ops)
>>
>> Don't think this needs to be inline, whats the point?
>>
>
> The point is that it is in the header file, not a c file.
>
That's what I meant. Why is it in the header file. Why not
in .c file and declared.
> Fred
>
(-Bz
>>> {
>>> - struct inode *inode = data->inode;
>>> - struct nfs_server *nfss = NFS_SERVER(inode);
>>> enum pnfs_try_status ret;
>>>
>>> - /* FIXME: read_pagelist should probably be mandated */
>>> - if (PNFS_EXISTS_LDIO_OP(nfss, read_pagelist))
>>> - ret = _pnfs_try_to_read_data(data, call_ops);
>>> - else
>>> - ret = PNFS_NOT_ATTEMPTED;
>>> -
>>> + if (!data->req->wb_lseg)
>>> + return PNFS_NOT_ATTEMPTED;
>>> + ret = _pnfs_try_to_read_data(data, call_ops);
>>> if (ret == PNFS_ATTEMPTED)
>>> - nfs_inc_stats(inode, NFSIOS_PNFS_READ);
>>> + nfs_inc_stats(data->inode, NFSIOS_PNFS_READ);
>>> + else
>>> + _pnfs_clear_lseg_from_pages(&data->pages);
>>> return ret;
>>> }
>>>
>>
>> Boaz
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
On 06/09/2010 10:46 PM, Fred Isaman wrote:
>>>>> static inline enum pnfs_try_status
>>>>> pnfs_try_to_read_data(struct nfs_read_data *data,
>>>>> const struct rpc_call_ops *call_ops)
>>>>
>>>> Don't think this needs to be inline, whats the point?
>>>>
>>>
>>> The point is that it is in the header file, not a c file.
>>>
>>
>> That's what I meant. Why is it in the header file. Why not
>> in .c file and declared.
>>
>
> To make it easy to ifdef out if CONFIG_NFS_V4_1 is not set.
>
> Fred
>
It's not easier, just:
#ifdef CONFIG_NFS_V4_1
enum pnfs_try_status
pnfs_try_to_read_data(struct nfs_read_data *data,
const struct rpc_call_ops *call_ops); /* declaration */
#else
static inline enum pnfs_try_status
pnfs_try_to_read_data(struct nfs_read_data *data,
const struct rpc_call_ops *call_ops) /* Empty inline */
{
return ...;
}
#endif
We do that everywhere. Should also here
Boaz
>>> Fred
>>>
>>
>> (-Bz
>>
On Wed, Jun 9, 2010 at 3:39 PM, Boaz Harrosh <[email protected]> wro=
te:
> On 06/09/2010 10:29 PM, Fred Isaman wrote:
>> On Wed, Jun 9, 2010 at 3:19 PM, Boaz Harrosh <[email protected]> =
wrote:
>>> On 06/08/2010 07:19 AM, Fred Isaman wrote:
>>>> Change readpages path to only call LAYOUTGET once.
>>>>
>>>> Signed-off-by: Fred Isaman <[email protected]>
>>>> ---
>>>> =A0fs/nfs/pagelist.c | =A0 =A02 ++
>>>> =A0fs/nfs/pnfs.c =A0 =A0 | =A0 37 +++++++-------------------------=
-----
>>>> =A0fs/nfs/pnfs.h =A0 =A0 | =A0 25 ++++++++++++++++---------
>>>> =A03 files changed, 25 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
>>>> index ed647b9..c3e5a1f 100644
>>>> --- a/fs/nfs/pagelist.c
>>>> +++ b/fs/nfs/pagelist.c
>>>> @@ -253,6 +253,8 @@ static int nfs_can_coalesce_requests(struct nf=
s_page *prev,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>>>> =A0 =A0 =A0 if (prev->wb_pgbase + prev->wb_bytes !=3D PAGE_CACHE_S=
IZE)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>>>> + =A0 =A0 if (req->wb_lseg !=3D prev->wb_lseg)
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>>>> =A0#ifdef CONFIG_NFS_V4_1
>>>> =A0 =A0 =A0 if (pgio->pg_test && !pgio->pg_test(pgio, prev, req))
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>>> index 2b5f6fc..692a18e 100644
>>>> --- a/fs/nfs/pnfs.c
>>>> +++ b/fs/nfs/pnfs.c
>>>> @@ -1689,7 +1689,7 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>>> =A0{
>>>> =A0 =A0 =A0 struct nfs_readargs *args =3D &rdata->args;
>>>> =A0 =A0 =A0 struct inode *inode =3D rdata->inode;
>>>> - =A0 =A0 int numpages, status, pgcount, temp;
>>>> + =A0 =A0 int numpages, pgcount, temp;
>>>> =A0 =A0 =A0 struct nfs_server *nfss =3D NFS_SERVER(inode);
>>>> =A0 =A0 =A0 struct nfs_inode *nfsi =3D NFS_I(inode);
>>>> =A0 =A0 =A0 struct pnfs_layout_segment *lseg;
>>>> @@ -1701,19 +1701,8 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 args->count,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 args->offset);
>>>>
>>>> - =A0 =A0 /* Retrieve and set layout if not allready cached */
>>>> - =A0 =A0 status =3D _pnfs_update_layout(inode,
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
args->context,
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
args->count,
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
args->offset,
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
IOMODE_READ,
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
&lseg);
>>>> - =A0 =A0 if (status) {
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 dprintk("%s: Updating layout failed (%d)=
, retry with NFS \n",
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 __func__, status);
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 trypnfs =3D PNFS_NOT_ATTEMPTED;
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 goto out;
>>>> - =A0 =A0 }
>>>> + =A0 =A0 lseg =3D rdata->req->wb_lseg;
>>>> + =A0 =A0 get_lseg(lseg);
>>>>
>>>> =A0 =A0 =A0 /* Determine number of pages. */
>>>> =A0 =A0 =A0 pgcount =3D args->pgbase + args->count;
>>>> @@ -1740,7 +1729,6 @@ pnfs_readpages(struct nfs_read_data *rdata)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 rdata->pdata.lseg =3D NULL;
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_lseg(lseg);
>>>> =A0 =A0 =A0 }
>>>> - out:
>>>> =A0 =A0 =A0 dprintk("%s End (trypnfs:%d)\n", __func__, trypnfs);
>>>> =A0 =A0 =A0 return trypnfs;
>>>> =A0}
>>>> @@ -1749,21 +1737,10 @@ enum pnfs_try_status
>>>> =A0_pnfs_try_to_read_data(struct nfs_read_data *data,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const struct rpc_call_o=
ps *call_ops)
>>>> =A0{
>>>> - =A0 =A0 struct inode *ino =3D data->inode;
>>>> - =A0 =A0 struct nfs_server *nfss =3D NFS_SERVER(ino);
>>>> -
>>>> - =A0 =A0 dprintk("--> %s\n", __func__);
>>>> - =A0 =A0 /* Only create an rpc request if utilizing NFSv4 I/O */
>>>> - =A0 =A0 if (!pnfs_enabled_sb(nfss) ||
>>>> - =A0 =A0 =A0 =A0 !nfss->pnfs_curr_ld->ld_io_ops->read_pagelist) {
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 dprintk("<-- %s: not using pnfs\n", __fu=
nc__);
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 return PNFS_NOT_ATTEMPTED;
>>>> - =A0 =A0 } else {
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 dprintk("%s: Utilizing pNFS I/O\n", __fu=
nc__);
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 data->pdata.call_ops =3D call_ops;
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 data->pdata.pnfs_error =3D 0;
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 return pnfs_readpages(data);
>>>> - =A0 =A0 }
>>>
>>> Wahoo, nice stuff Ha
>>>
>>> By now this can go into it's only caller.
>>> (And caller de-inlined, what was that all about)
>>>
>>>> + =A0 =A0 dprintk("%s: Utilizing pNFS I/O\n", __func__);
>>>> + =A0 =A0 data->pdata.call_ops =3D call_ops;
>>>> + =A0 =A0 data->pdata.pnfs_error =3D 0;
>>>> + =A0 =A0 return pnfs_readpages(data);
>>>> =A0}
>>>>
>>>> =A0enum pnfs_try_status
>>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>>>> index 6326ed5..816ebe1 100644
>>>> --- a/fs/nfs/pnfs.h
>>>> +++ b/fs/nfs/pnfs.h
>>>> @@ -94,22 +94,29 @@ static inline int pnfs_enabled_sb(struct nfs_s=
erver *nfss)
>>>> =A0 =A0 =A0 return nfss->pnfs_curr_ld !=3D NULL;
>>>> =A0}
>>>>
>>>> +static inline void _pnfs_clear_lseg_from_pages(struct list_head *=
head)
>>>> +{
>>>> + =A0 =A0 struct nfs_page *req;
>>>> +
>>>> + =A0 =A0 list_for_each_entry(req, head, wb_list) {
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 put_lseg(req->wb_lseg);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 req->wb_lseg =3D NULL;
>>>> + =A0 =A0 }
>>>> +}
>>>> +
>>>> =A0static inline enum pnfs_try_status
>>>> =A0pnfs_try_to_read_data(struct nfs_read_data *data,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const struct rpc_call_ops =
*call_ops)
>>>
>>> Don't think this needs to be inline, whats the point?
>>>
>>
>> The point is that it is in the header file, not a c file.
>>
>
> That's what I meant. Why is it in the header file. Why not
> in .c file and declared.
>
To make it easy to ifdef out if CONFIG_NFS_V4_1 is not set.
=46red
>> Fred
>>
>
> (-Bz
>
>>>> =A0{
>>>> - =A0 =A0 struct inode *inode =3D data->inode;
>>>> - =A0 =A0 struct nfs_server *nfss =3D NFS_SERVER(inode);
>>>> =A0 =A0 =A0 enum pnfs_try_status ret;
>>>>
>>>> - =A0 =A0 /* FIXME: read_pagelist should probably be mandated */
>>>> - =A0 =A0 if (PNFS_EXISTS_LDIO_OP(nfss, read_pagelist))
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 ret =3D _pnfs_try_to_read_data(data, cal=
l_ops);
>>>> - =A0 =A0 else
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 ret =3D PNFS_NOT_ATTEMPTED;
>>>> -
>>>> + =A0 =A0 if (!data->req->wb_lseg)
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 return PNFS_NOT_ATTEMPTED;
>>>> + =A0 =A0 ret =3D _pnfs_try_to_read_data(data, call_ops);
>>>> =A0 =A0 =A0 if (ret =3D=3D PNFS_ATTEMPTED)
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 nfs_inc_stats(inode, NFSIOS_PNFS_READ);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 nfs_inc_stats(data->inode, NFSIOS_PNFS_R=
EAD);
>>>> + =A0 =A0 else
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 _pnfs_clear_lseg_from_pages(&data->pages=
);
>>>> =A0 =A0 =A0 return ret;
>>>> =A0}
>>>>
>>>
>>> Boaz
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs=
" in
>>> the body of a message to [email protected]
>>> More majordomo info at =A0http://vger.kernel.org/majordomo-info.htm=
l
>>>
>
>