Re: [PATCH 4/4] NFS: Fix nfs_direct_dirty_pages()

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

Trond Myklebust wrote:
> On Tue, 2007-05-22 at 10:29 -0400, Trond Myklebust wrote:
>> From: Trond Myklebust <Trond.Myklebust@netapp.com>
>>
>> We only need to dirty the pages that were actually read in.
>>
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>> ---
>>
>>  fs/nfs/direct.c |   20 ++++++++++++++------
>>  1 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
>> index ef0ce2c..4060e6f 100644
>> --- a/fs/nfs/direct.c
>> +++ b/fs/nfs/direct.c
>> @@ -122,9 +122,15 @@ ssize_t nfs_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, loff_
>>  	return -EINVAL;
>>  }
>>  
>> -static void nfs_direct_dirty_pages(struct page **pages, unsigned int npages)
>> +static void nfs_direct_dirty_pages(struct page **pages, unsigned int pgbase, size_t count)
>>  {
>> +	unsigned int npages;
>>  	unsigned int i;
>> +
>> +	if (count == 0)
>> +		return;
>> +	pages += (pgbase >> PAGE_SHIFT);
>> +	npages = (count + (pgbase & ~PAGE_MASK) + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>  	for (i = 0; i < npages; i++) {
>>  		struct page *page = pages[i];
>>  		if (!PageCompound(page))
>> @@ -224,18 +230,20 @@ static void nfs_direct_read_result(struct rpc_task *task, void *calldata)
>>  	if (nfs_readpage_result(task, data) != 0)
>>  		return;
>>  
>> -	nfs_direct_dirty_pages(data->pagevec, data->npages);
>> -	nfs_direct_release_pages(data->pagevec, data->npages);
>> -
>>  	spin_lock(&dreq->lock);
>>  
>> -	if (likely(task->tk_status >= 0))
>> +	if (likely(task->tk_status >= 0)) {
>>  		dreq->count += data->res.count;
>> -	else
>> +		nfs_direct_dirty_pages(data->pagevec,
>> +				data->args.pgbase,
>> +				data->res.count);
>> +	} else
>>  		dreq->error = task->tk_status;
>>  
>>  	spin_unlock(&dreq->lock);
>>  
>> +	nfs_direct_release_pages(data->pagevec, data->npages);
>> +
>>  	if (put_dreq(dreq))
>>  		nfs_direct_complete(dreq);
>>  }
> 
> Having looked at this one more closely I'd say we have a second bug here
> since nfs_direct_dirty_pages() is actually calling
> set_page_dirty_lock(). Firstly it is quite unacceptable for an
> asynchronous RPC callback to be calling lock_page(). Secondly, AFAICS
> you have no guarantees that the pages that were mapped using the call to
> get_user_pages() aren't already locked...
> 
> Chuck?

OK, well...

It looks like the generic dio code preemptively dirties pages in the 
"read buffer" while still in the issuing process's context.  When the 
read operation completes, it checks whether the pages are still dirty, 
and if not, it asks a worker thread to redirty them via set_page_dirty_lock.

There's still some question about whether this needs to be done for both 
the synchronous and asynchronous read case... I'd say for NFS, both are 
needed since all read results arrive via soft IRQ.

I think then that removing set_page_dirty_lock from the read_result path 
makes your question about get_user_pages moot.

What do you think?

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 291 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel/
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs