[PATCH 1/4] NFS: Fix handful of compiler warnings in direct.c

[PATCH 1/4] NFS: Fix handful of compiler warnings in direct.c

[Trond Myklebust]

From: Chuck Lever <chuck.lever@oracle.com>

This patch fixes a couple of signage issues that were causing an Oops
when running the LTP diotest4 test. get_user_pages() returns a signed
error, hence we need to be careful when comparing with the unsigned
number of pages from data->npages.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/direct.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 345aa5c..6c588ec 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -122,9 +122,9 @@ 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, int npages)
+static void nfs_direct_dirty_pages(struct page **pages, unsigned int npages)
 {
-	int i;
+	unsigned int i;
 	for (i = 0; i < npages; i++) {
 		struct page *page = pages[i];
 		if (!PageCompound(page))
@@ -132,9 +132,9 @@ static void nfs_direct_dirty_pages(struct page **pages, int npages)
 	}
 }
 
-static void nfs_direct_release_pages(struct page **pages, int npages)
+static void nfs_direct_release_pages(struct page **pages, unsigned int npages)
 {
-	int i;
+	unsigned int i;
 	for (i = 0; i < npages; i++)
 		page_cache_release(pages[i]);
 }
@@ -279,9 +279,12 @@ static ssize_t nfs_direct_read_schedule(struct nfs_direct_req *dreq, unsigned lo
 		result = get_user_pages(current, current->mm, user_addr,
 					data->npages, 1, 0, data->pagevec, NULL);
 		up_read(&current->mm->mmap_sem);
-		if (unlikely(result < data->npages)) {
-			if (result > 0)
-				nfs_direct_release_pages(data->pagevec, result);
+		if (result < 0) {
+			nfs_readdata_release(data);
+			break;
+		}
+		if ((unsigned)result < data->npages) {
+			nfs_direct_release_pages(data->pagevec, result);
 			nfs_readdata_release(data);
 			break;
 		}
@@ -610,9 +613,12 @@ static ssize_t nfs_direct_write_schedule(struct nfs_direct_req *dreq, unsigned l
 		result = get_user_pages(current, current->mm, user_addr,
 					data->npages, 0, 0, data->pagevec, NULL);
 		up_read(&current->mm->mmap_sem);
-		if (unlikely(result < data->npages)) {
-			if (result > 0)
-				nfs_direct_release_pages(data->pagevec, result);
+		if (result < 0) {
+			nfs_writedata_release(data);
+			break;
+		}
+		if ((unsigned)result < data->npages) {
+			nfs_direct_release_pages(data->pagevec, result);
 			nfs_writedata_release(data);
 			break;
 		}

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

[PATCH 2/4] NFS: Clean ups in fs/nfs/direct.c

[Trond Myklebust]

From: Chuck Lever <chuck.lever@oracle.com>

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/direct.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 6c588ec..49e4823 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -750,10 +750,8 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, const struct iovec *iov,
 		(unsigned long) count, (long long) pos);
 
 	if (nr_segs != 1)
-		return -EINVAL;
-
-	if (count < 0)
 		goto out;
+
 	retval = -EFAULT;
 	if (!access_ok(VERIFY_WRITE, buf, count))
 		goto out;
@@ -801,7 +799,7 @@ out:
 ssize_t nfs_file_direct_write(struct kiocb *iocb, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos)
 {
-	ssize_t retval;
+	ssize_t retval = -EINVAL;
 	struct file *file = iocb->ki_filp;
 	struct address_space *mapping = file->f_mapping;
 	/* XXX: temporary */
@@ -814,7 +812,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, const struct iovec *iov,
 		(unsigned long) count, (long long) pos);
 
 	if (nr_segs != 1)
-		return -EINVAL;
+		goto out;
 
 	retval = generic_write_checks(file, &pos, &count, 0);
 	if (retval)

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

[PATCH 3/4] NFS: Don't fail an O_DIRECT read/write if get_user_pages() returns pages

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

There is no need to fail the entire O_DIRECT read/write just because
get_user_pages() returned fewer pages than we requested.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/direct.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 49e4823..ef0ce2c 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -284,9 +284,14 @@ static ssize_t nfs_direct_read_schedule(struct nfs_direct_req *dreq, unsigned lo
 			break;
 		}
 		if ((unsigned)result < data->npages) {
-			nfs_direct_release_pages(data->pagevec, result);
-			nfs_readdata_release(data);
-			break;
+			bytes = result * PAGE_SIZE;
+			if (bytes <= pgbase) {
+				nfs_direct_release_pages(data->pagevec, result);
+				nfs_readdata_release(data);
+				break;
+			}
+			bytes -= pgbase;
+			data->npages = result;
 		}
 
 		get_dreq(dreq);
@@ -618,9 +623,14 @@ static ssize_t nfs_direct_write_schedule(struct nfs_direct_req *dreq, unsigned l
 			break;
 		}
 		if ((unsigned)result < data->npages) {
-			nfs_direct_release_pages(data->pagevec, result);
-			nfs_writedata_release(data);
-			break;
+			bytes = result * PAGE_SIZE;
+			if (bytes <= pgbase) {
+				nfs_direct_release_pages(data->pagevec, result);
+				nfs_writedata_release(data);
+				break;
+			}
+			bytes -= pgbase;
+			data->npages = result;
 		}
 
 		get_dreq(dreq);

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

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

[Trond Myklebust]

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);
 }

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

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

[Trond Myklebust]

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?

Trond

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

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

[Chuck Lever]

[-- 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

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

[Trond Myklebust]

On Wed, 2007-05-23 at 17:56 -0400, Chuck Lever wrote:
> 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?

I've simply replaced the call to set_page_dirty_lock() with a standard
call to set_page_dirty().

Cheers
  Trond

-------------------------------------------------------------------------
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/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs