On 2011-02-16 14:55, Fred Isaman wrote:
>
> On Feb 16, 2011, at 2:42 PM, Benny Halevy wrote:
>
>> On 2011-02-15 03:38, 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.
>>>
>>> @@ -131,10 +132,12 @@ int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode,
>>> zero_user_segment(page, len, PAGE_CACHE_SIZE);
>>>
>>> nfs_list_add_request(new, &one_request);
>>> + lseg = pnfs_update_layout(inode, ctx, IOMODE_READ);
>>> if (NFS_SERVER(inode)->rsize < PAGE_CACHE_SIZE)
>>> - nfs_pagein_multi(inode, &one_request, 1, len, 0);
>>> + nfs_pagein_multi(inode, &one_request, 1, len, 0, lseg);
>>> else
>>> - nfs_pagein_one(inode, &one_request, 1, len, 0);
>>> + nfs_pagein_one(inode, &one_request, 1, len, 0, lseg);
>>> + put_lseg(lseg);
>>> return 0;
>>> }
>>>
>>> @@ -160,7 +163,8 @@ static void nfs_readpage_release(struct nfs_page *req)
>>> */
>>> static int nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data,
>>> const struct rpc_call_ops *call_ops,
>>> - unsigned int count, unsigned int offset)
>>> + unsigned int count, unsigned int offset,
>>> + struct pnfs_layout_segment *lseg)
>>> {
>>> struct inode *inode = req->wb_context->path.dentry->d_inode;
>>> int swap_flags = IS_SWAPFILE(inode) ? NFS_RPC_SWAPFLAGS : 0;
>>> @@ -183,6 +187,7 @@ static int nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data,
>>> data->req = req;
>>> data->inode = inode;
>>> data->cred = msg.rpc_cred;
>>> + data->lseg = get_lseg(lseg);
>>>
>>> data->args.fh = NFS_FH(inode);
>>> data->args.offset = req_offset(req) + offset;
>>> @@ -240,7 +245,7 @@ nfs_async_read_error(struct list_head *head)
>>> * won't see the new data until our attribute cache is updated. This is more
>>> * or less conventional NFS client behavior.
>>> */
>>> -static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags)
>>> +static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags, struct pnfs_layout_segment *lseg)
>>> {
>>> struct nfs_page *req = nfs_list_entry(head->next);
>>> struct page *page = req->wb_page;
>>> @@ -266,6 +271,8 @@ static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigne
>>> } while(nbytes != 0);
>>> atomic_set(&req->wb_complete, requests);
>>>
>>> + /* We know lseg==NULL */
Can you provide more details?
If it's always NULL why bother to pass it in?
>>> + lseg = pnfs_update_layout(inode, req->wb_context, IOMODE_READ);
>>> ClearPageError(page);
>>> offset = 0;
>>> nbytes = count;
>>> @@ -280,12 +287,13 @@ static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigne
>>> if (nbytes < rsize)
>>> rsize = nbytes;
>>> ret2 = nfs_read_rpcsetup(req, data, &nfs_read_partial_ops,
>>> - rsize, offset);
>>> + rsize, offset, lseg);
>>> if (ret == 0)
>>> ret = ret2;
>>> offset += rsize;
>>> nbytes -= rsize;
>>> } while (nbytes != 0);
>>> + put_lseg(lseg);
>>>
>>> return ret;
>>>
>>> @@ -300,7 +308,7 @@ out_bad:
>>> return -ENOMEM;
>>> }
>>>
>>> -static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags)
>>> +static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags, struct pnfs_layout_segment *lseg)
>>> {
>>> struct nfs_page *req;
>>> struct page **pages;
>>> @@ -320,9 +328,14 @@ static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned
>>> *pages++ = req->wb_page;
>>> }
>>> req = nfs_list_entry(data->pages.next);
>>> + if ((!lseg) && list_is_singular(&data->pages))
>>> + lseg = pnfs_update_layout(inode, req->wb_context, IOMODE_READ);
When is lseg NULL and why getting it here works better than in nfs_readpage_async?
>>>
>>> - return nfs_read_rpcsetup(req, data, &nfs_read_full_ops, count, 0);
>>> + ret = nfs_read_rpcsetup(req, data, &nfs_read_full_ops, count, 0, lseg);
>>> + put_lseg(lseg);
>>
>> Shouldn't that be done only if pnfs_update_layout was called here?
>> Otherwise, the caller, nfs_readpage_async puts the lseg it passes down.
>>
>
> You are right there is a problem. But it needs to be fixed by removing the put_lseg from nfs_readpage_async.
>
>
If we can avoid getting the lseg in one place and putting it in another that would be better.
Benny
>>> + return ret;
>>> out_bad:
>>> + put_lseg(lseg);
>>
>> I'd unify the common exit path by doing nfs_async_read_error on the error path
>> and then goto out for the common code.
>>
>
> OK.
>
> Fred
>
--
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
