[PATCH] mm: readahead: return the value which force_page_cache_readahead() returns

[PATCH] mm: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

force_page_cache_readahead() may fail, so need let the related upper
system calls know about it by its return value.

Also let related code pass "scripts/checkpatch.pl's" checking.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/fadvise.c   |    4 ++--
 mm/madvise.c   |    4 ++--
 mm/readahead.c |    3 +--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/fadvise.c b/mm/fadvise.c
index 3bcfd81..7da9eb1 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -107,8 +107,8 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
 		 * Ignore return value because fadvise() shall return
 		 * success even if filesystem can't retrieve a hint,
 		 */
-		force_page_cache_readahead(mapping, f.file, start_index,
-					   nrpages);
+		ret = force_page_cache_readahead(mapping, f.file, start_index,
+						 nrpages);
 		break;
 	case POSIX_FADV_NOREUSE:
 		break;
diff --git a/mm/madvise.c b/mm/madvise.c
index 936799f..3d0d484 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -247,8 +247,8 @@ static long madvise_willneed(struct vm_area_struct *vma,
 		end = vma->vm_end;
 	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
-	force_page_cache_readahead(file->f_mapping, file, start, end - start);
-	return 0;
+	return force_page_cache_readahead(file->f_mapping, file,
+					start, end - start);
 }
 
 /*
diff --git a/mm/readahead.c b/mm/readahead.c
index 829a77c..5b9ac62 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -572,8 +572,7 @@ do_readahead(struct address_space *mapping, struct file *filp,
 	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
 		return -EINVAL;
 
-	force_page_cache_readahead(mapping, filp, index, nr);
-	return 0;
+	return force_page_cache_readahead(mapping, filp, index, nr);
 }
 
 SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: readahead: return the value which force_page_cache_readahead() returns

[Andrew Morton]

On Tue, 20 Aug 2013 11:31:52 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> force_page_cache_readahead() may fail, so need let the related upper
> system calls know about it by its return value.
> 
> Also let related code pass "scripts/checkpatch.pl's" checking.
> 
> --- a/mm/fadvise.c
> +++ b/mm/fadvise.c
> @@ -107,8 +107,8 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
>  		 * Ignore return value because fadvise() shall return
>  		 * success even if filesystem can't retrieve a hint,
>  		 */

		^^ look.

> -		force_page_cache_readahead(mapping, f.file, start_index,
> -					   nrpages);
> +		ret = force_page_cache_readahead(mapping, f.file, start_index,
> +						 nrpages);
>  		break;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

On 08/21/2013 07:16 AM, Andrew Morton wrote:
> On Tue, 20 Aug 2013 11:31:52 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>> force_page_cache_readahead() may fail, so need let the related upper
>> system calls know about it by its return value.
>>
>> Also let related code pass "scripts/checkpatch.pl's" checking.
>>
>> --- a/mm/fadvise.c
>> +++ b/mm/fadvise.c
>> @@ -107,8 +107,8 @@ SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice)
>>  		 * Ignore return value because fadvise() shall return
>>  		 * success even if filesystem can't retrieve a hint,
>>  		 */
> 
> 		^^ look.
> 

Oh, thanks.

It is my fault, I will send patch v2 for it.

>> -		force_page_cache_readahead(mapping, f.file, start_index,
>> -					   nrpages);
>> +		ret = force_page_cache_readahead(mapping, f.file, start_index,
>> +						 nrpages);
>>  		break;
> 
> 
> 


Thanks.
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2] m: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

force_page_cache_readahead() may fail, so need let the related upper
system calls know about it by its return value.

For system call fadvise64_64(), ignore return value because fadvise()
shall return success even if filesystem can't retrieve a hint.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/madvise.c   |    4 ++--
 mm/readahead.c |    3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 936799f..3d0d484 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -247,8 +247,8 @@ static long madvise_willneed(struct vm_area_struct *vma,
 		end = vma->vm_end;
 	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
-	force_page_cache_readahead(file->f_mapping, file, start, end - start);
-	return 0;
+	return force_page_cache_readahead(file->f_mapping, file,
+					start, end - start);
 }
 
 /*
diff --git a/mm/readahead.c b/mm/readahead.c
index e4ed041..1b21b5c 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -572,8 +572,7 @@ do_readahead(struct address_space *mapping, struct file *filp,
 	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
 		return -EINVAL;
 
-	force_page_cache_readahead(mapping, filp, index, nr);
-	return 0;
+	return force_page_cache_readahead(mapping, filp, index, nr);
 }
 
 SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] m: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

Hello Maintainers:

Please help check this patch, when you have time.

If need a related test, please let me know, I should try (better to
provide some suggestions for test).


Thanks.

On 08/21/2013 10:41 AM, Chen Gang wrote:
> force_page_cache_readahead() may fail, so need let the related upper
> system calls know about it by its return value.
> 
> For system call fadvise64_64(), ignore return value because fadvise()
> shall return success even if filesystem can't retrieve a hint.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  mm/madvise.c   |    4 ++--
>  mm/readahead.c |    3 +--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 936799f..3d0d484 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -247,8 +247,8 @@ static long madvise_willneed(struct vm_area_struct *vma,
>  		end = vma->vm_end;
>  	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
>  
> -	force_page_cache_readahead(file->f_mapping, file, start, end - start);
> -	return 0;
> +	return force_page_cache_readahead(file->f_mapping, file,
> +					start, end - start);
>  }
>  
>  /*
> diff --git a/mm/readahead.c b/mm/readahead.c
> index e4ed041..1b21b5c 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -572,8 +572,7 @@ do_readahead(struct address_space *mapping, struct file *filp,
>  	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
>  		return -EINVAL;
>  
> -	force_page_cache_readahead(mapping, filp, index, nr);
> -	return 0;
> +	return force_page_cache_readahead(mapping, filp, index, nr);
>  }
>  
>  SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
> 


-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] m: readahead: return the value which force_page_cache_readahead() returns

[Andrew Morton]

On Wed, 21 Aug 2013 10:41:20 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> force_page_cache_readahead() may fail, so need let the related upper
> system calls know about it by its return value.
> 
> For system call fadvise64_64(), ignore return value because fadvise()
> shall return success even if filesystem can't retrieve a hint.
> 

Actually, force_page_cache_readahead() cannot fail - I see no code path
via which it returns a -ve errno.

Of course, that might change in the future and although readahead is
usually a best-effort-dont-care-if-it-fails thing, I suppose that in
the case of madvise() and sys_readahead() we should inform userspace,
as readhead is the primary reason for thier performing the syscall.


While we're there, please review...

From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm/readahead.c:do_readhead(): don't check for ->readpage

The callee force_page_cache_readahead() already does this and unlike
do_readahead(), force_page_cache_readahead() remembers to check for
->readpages() as well.



Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/readahead.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN mm/readahead.c~a mm/readahead.c
--- a/mm/readahead.c~a
+++ a/mm/readahead.c
@@ -569,7 +569,7 @@ static ssize_t
 do_readahead(struct address_space *mapping, struct file *filp,
 	     pgoff_t index, unsigned long nr)
 {
-	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
+	if (!mapping || !mapping->a_ops)
 		return -EINVAL;
 
 	return force_page_cache_readahead(mapping, filp, index, nr);
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] m: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

On 09/18/2013 06:56 AM, Andrew Morton wrote:
> On Wed, 21 Aug 2013 10:41:20 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>> force_page_cache_readahead() may fail, so need let the related upper
>> system calls know about it by its return value.
>>
>> For system call fadvise64_64(), ignore return value because fadvise()
>> shall return success even if filesystem can't retrieve a hint.
>>
> 
> Actually, force_page_cache_readahead() cannot fail - I see no code path
> via which it returns a -ve errno.
>

Hmm... except return -EINVAL, currently it is.

> Of course, that might change in the future and although readahead is
> usually a best-effort-dont-care-if-it-fails thing, I suppose that in
> the case of madvise() and sys_readahead() we should inform userspace,
> as readhead is the primary reason for thier performing the syscall.
> 

Yeah.

> 
> While we're there, please review...
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm/readahead.c:do_readhead(): don't check for ->readpage
> 
> The callee force_page_cache_readahead() already does this and unlike
> do_readahead(), force_page_cache_readahead() remembers to check for
> ->readpages() as well.
> 
> 
> 
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/readahead.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -puN mm/readahead.c~a mm/readahead.c
> --- a/mm/readahead.c~a
> +++ a/mm/readahead.c
> @@ -569,7 +569,7 @@ static ssize_t
>  do_readahead(struct address_space *mapping, struct file *filp,
>  	     pgoff_t index, unsigned long nr)
>  {
> -	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
> +	if (!mapping || !mapping->a_ops)
>  		return -EINVAL;
>  
>  	return force_page_cache_readahead(mapping, filp, index, nr);
> _
> 
> 
> 

At least for me, this patch sounds good.


And for the code below in "mm/readahead.c", I guess, skipping return
value of force_page_cache_readahead() is acceptable, but still better to
get some comments for it


505 void page_cache_sync_readahead(struct address_space *mapping,
506                                struct file_ra_state *ra, struct file
*filp,
507                                pgoff_t offset, unsigned long req_size)
508 {
509         /* no read-ahead */
510         if (!ra->ra_pages)
511                 return;
512
513         /* be dumb */
514         if (filp && (filp->f_mode & FMODE_RANDOM)) {
515                 force_page_cache_readahead(mapping, filp, offset,
req_size);
516                 return;
517         }
518
519         /* do read-ahead */
520         ondemand_readahead(mapping, ra, filp, false, offset, req_size);
521 }
522 EXPORT_SYMBOL_GPL(page_cache_sync_readahead);


Thanks.
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[Chen Gang]

For system call readahead(), need always return 0 instead of bytes read
when succeed. The related commit "fee53ce mm/readahead.c: return the
value which force_page_cache_readahead() returns" causes this issue.

This bug is found by LTP readahead02 test, the related output:

  [root@gchenlinux readahead]# ./readahead02
  readahead02    0  TINFO  :  creating test file of size: 67108864
  readahead02    0  TINFO  :  read_testfile(0)
  readahead02    0  TINFO  :  read_testfile(1)
  readahead02    1  TFAIL  :  unexpected failure - returned value = 16384, expected: 0
  readahead02    2  TPASS  :  offset is still at 0 as expected
  readahead02    0  TINFO  :  read_testfile(0) took: 2292819 usec
  readahead02    0  TINFO  :  read_testfile(1) took: 3524116 usec
  readahead02    0  TINFO  :  read_testfile(0) read: 67108864 bytes
  readahead02    0  TINFO  :  read_testfile(1) read: 0 bytes
  readahead02    3  TPASS  :  readahead saved some I/O
  readahead02    0  TINFO  :  cache can hold at least: 624316 kB
  readahead02    0  TINFO  :  read_testfile(0) used cache: 65476 kB
  readahead02    0  TINFO  :  read_testfile(1) used cache: 65632 kB
  readahead02    4  TPASS  :  using cache as expected

After this fix, it can pass LTP common test by readahead01 and readahead02.

  [root@gchenlinux readahead]# ./readahead01 
  readahead01    0  TINFO  :  test_bad_fd -1
  readahead01    1  TPASS  :  expected ret success - returned value = -1
  readahead01    2  TPASS  :  expected failure: TEST_ERRNO=EBADF(9): Bad file descriptor
  readahead01    0  TINFO  :  test_bad_fd O_WRONLY
  readahead01    3  TPASS  :  expected ret success - returned value = -1
  readahead01    4  TPASS  :  expected failure: TEST_ERRNO=EBADF(9): Bad file descriptor
  readahead01    0  TINFO  :  test_invalid_fd pipe
  readahead01    5  TPASS  :  expected ret success - returned value = -1
  readahead01    6  TPASS  :  expected failure: TEST_ERRNO=EINVAL(22): Invalid argument
  readahead01    0  TINFO  :  test_invalid_fd socket
  readahead01    7  TPASS  :  expected ret success - returned value = -1
  readahead01    8  TPASS  :  expected failure: TEST_ERRNO=EINVAL(22): Invalid argument
  [root@gchenlinux readahead]# ./readahead02
  readahead02    0  TINFO  :  creating test file of size: 67108864
  readahead02    0  TINFO  :  read_testfile(0)
  readahead02    0  TINFO  :  read_testfile(1)
  readahead02    1  TPASS  :  expected ret success - returned value = 0
  readahead02    2  TPASS  :  offset is still at 0 as expected
  readahead02    0  TINFO  :  read_testfile(0) took: 3327468 usec
  readahead02    0  TINFO  :  read_testfile(1) took: 2802184 usec
  readahead02    0  TINFO  :  read_testfile(0) read: 67108864 bytes
  readahead02    0  TINFO  :  read_testfile(1) read: 0 bytes
  readahead02    3  TPASS  :  readahead saved some I/O
  readahead02    0  TINFO  :  cache can hold at least: 794800 kB
  readahead02    0  TINFO  :  read_testfile(0) used cache: 66704 kB
  readahead02    0  TINFO  :  read_testfile(1) used cache: 65528 kB
  readahead02    4  TPASS  :  using cache as expected


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/readahead.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 1eee42b..83a202e 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -592,5 +592,5 @@ SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
 		}
 		fdput(f);
 	}
-	return ret;
+	return ret < 0 ? ret : 0;
 }
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] m: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

This patch fix one issue, but cause 2 issues: *readahead() will return
read bytes when succeed (just like common read functions).

One for readahead(), which I already sent related patch for it.

The other for madvise(), I fix it, just use LTP test it (after finish
test, I will send fix patch for it).


Thanks.

On 08/21/2013 10:41 AM, Chen Gang wrote:
> force_page_cache_readahead() may fail, so need let the related upper
> system calls know about it by its return value.
> 
> For system call fadvise64_64(), ignore return value because fadvise()
> shall return success even if filesystem can't retrieve a hint.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  mm/madvise.c   |    4 ++--
>  mm/readahead.c |    3 +--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 936799f..3d0d484 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -247,8 +247,8 @@ static long madvise_willneed(struct vm_area_struct *vma,
>  		end = vma->vm_end;
>  	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
>  
> -	force_page_cache_readahead(file->f_mapping, file, start, end - start);
> -	return 0;
> +	return force_page_cache_readahead(file->f_mapping, file,
> +					start, end - start);
>  }
>  
>  /*
> diff --git a/mm/readahead.c b/mm/readahead.c
> index e4ed041..1b21b5c 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -572,8 +572,7 @@ do_readahead(struct address_space *mapping, struct file *filp,
>  	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
>  		return -EINVAL;
>  
> -	force_page_cache_readahead(mapping, filp, index, nr);
> -	return 0;
> +	return force_page_cache_readahead(mapping, filp, index, nr);
>  }
>  
>  SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
> 


-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm/madvise.c: return 0 instead of read bytes after force_page_cache_readahead() succeeds.

[Chen Gang]

madvise_willneed() will return 0 when succeed, so need return 0 instead
of read bytes after force_page_cache_readahead() succeeds.

The related commit: "fee53ce mm/readahead.c: return the value which
force_page_cache_readahead() returns" causes this issue.

After modification, it can pass LTP common test (disable CONFIG_SWAP).
Although the original one also can pass LTP common test, still better
to fix it.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/madvise.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index dee8d46..3a739cd 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -220,6 +220,7 @@ static long madvise_willneed(struct vm_area_struct *vma,
 			     unsigned long start, unsigned long end)
 {
 	struct file *file = vma->vm_file;
+	int ret = 0;
 
 #ifdef CONFIG_SWAP
 	if (!file || mapping_cap_swap_backed(file->f_mapping)) {
@@ -247,8 +248,9 @@ static long madvise_willneed(struct vm_area_struct *vma,
 		end = vma->vm_end;
 	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
-	return force_page_cache_readahead(file->f_mapping, file,
+	ret = force_page_cache_readahead(file->f_mapping, file,
 					start, end - start);
+	return ret < 0 ? ret : 0;
 }
 
 /*
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[David Rientjes]

On Tue, 15 Oct 2013, Chen Gang wrote:

> diff --git a/mm/readahead.c b/mm/readahead.c
> index 1eee42b..83a202e 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -592,5 +592,5 @@ SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
>  		}
>  		fdput(f);
>  	}
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }

This was broken by your own "mm/readahead.c: return the value which 
force_page_cache_readahead() returns" patch in -mm, luckily Linus's tree 
isn't affected.

Nack to this and nack to the problem patch, which is absolutely pointless 
and did nothing but introduce this error.  readahead() is supposed to 
return 0, -EINVAL, or -EBADF and your original patch broke it.  That's 
because your original patch was completely pointless to begin with.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[Chen Gang]

On 10/17/2013 07:06 AM, David Rientjes wrote:
> On Tue, 15 Oct 2013, Chen Gang wrote:
> 
>> diff --git a/mm/readahead.c b/mm/readahead.c
>> index 1eee42b..83a202e 100644
>> --- a/mm/readahead.c
>> +++ b/mm/readahead.c
>> @@ -592,5 +592,5 @@ SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
>>  		}
>>  		fdput(f);
>>  	}
>> -	return ret;
>> +	return ret < 0 ? ret : 0;
>>  }
> 
> This was broken by your own "mm/readahead.c: return the value which 
> force_page_cache_readahead() returns" patch in -mm, luckily Linus's tree 
> isn't affected.
> 

Of cause it is.

And every member knows about it: in my comments, already mentioned about
it in a standard way.

Hmm... isn't it enough? (it seems you don't think so)

If possible, you can help me check all my patches again (at least, it is
not a bad idea to me).  ;-)


> Nack to this and nack to the problem patch, which is absolutely pointless 
> and did nothing but introduce this error.  readahead() is supposed to 
> return 0, -EINVAL, or -EBADF and your original patch broke it.  That's 
> because your original patch was completely pointless to begin with.
> 
> 

Do you mean: in do_readahead(), we need not check the return value of
force_page_cache_readahead()?

In my opinion, the system call of readahead() wants to notice whether
force_page_cache_readahead() fails or not (may return -EINVAL), which is
the mainly callee of readahead().

Don't you think so??


Thanks.
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[David Rientjes]

On Thu, 17 Oct 2013, Chen Gang wrote:

> If possible, you can help me check all my patches again (at least, it is
> not a bad idea to me).  ;-)
> 

I think your patches should be acked before being merged into linux-next, 
Hugh just had to revert another one that did affect Linus's tree in 
1ecfd533f4c5 ("mm/mremap.c: call pud_free() after fail calling
pmd_alloc()").  I had to revert your entire series of mpol_to_str() 
changes in -mm.  It's getting ridiculous and a waste of other people's 
time.

> > Nack to this and nack to the problem patch, which is absolutely pointless 
> > and did nothing but introduce this error.  readahead() is supposed to 
> > return 0, -EINVAL, or -EBADF and your original patch broke it.  That's 
> > because your original patch was completely pointless to begin with.
> > 
> 
> Do you mean: in do_readahead(), we need not check the return value of
> force_page_cache_readahead()?
> 

I'm saying we should revert 
mm-readaheadc-return-the-value-which-force_page_cache_readahead-returns.patch 
which violates the API of a syscall.  I see that patch has since been 
removed from -mm, so I'm happy with the result.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[Chen Gang]

On 10/17/2013 09:17 AM, David Rientjes wrote:
> On Thu, 17 Oct 2013, Chen Gang wrote:
> 
>> If possible, you can help me check all my patches again (at least, it is
>> not a bad idea to me).  ;-)
>>
> 
> I think your patches should be acked before being merged into linux-next, 
> Hugh just had to revert another one that did affect Linus's tree in 
> 1ecfd533f4c5 ("mm/mremap.c: call pud_free() after fail calling
> pmd_alloc()").  I had to revert your entire series of mpol_to_str() 
> changes in -mm.  It's getting ridiculous and a waste of other people's 
> time.
> 

If always get no reply, what to do, next?

In fact, in the whole kernel wide, at least, I have almost 10 patches
pending at least 2-3 weeks which got no reply (neither say ack, nor
nack), is it necessary to list them in this mail thread. ;-)

But all together, I welcome you to help ack/nack my patches for mm
sub-system (although I don't know your ack/nack whether have effect or not).

:-)

>>> Nack to this and nack to the problem patch, which is absolutely pointless 
>>> and did nothing but introduce this error.  readahead() is supposed to 
>>> return 0, -EINVAL, or -EBADF and your original patch broke it.  That's 
>>> because your original patch was completely pointless to begin with.
>>>
>>
>> Do you mean: in do_readahead(), we need not check the return value of
>> force_page_cache_readahead()?
>>
> 
> I'm saying we should revert 
> mm-readaheadc-return-the-value-which-force_page_cache_readahead-returns.patch 
> which violates the API of a syscall.  I see that patch has since been 
> removed from -mm, so I'm happy with the result.
> 
> 

Excuse me, I am not quite familiar with the upstream kernel version
trees merging.

Hmm... I think the final result need be: "still need check the return
value of force_patch_cache_readahead(), but need return 0 in readahead()
and madvise_willneed()".

Do you also think so, or do you happy with this result?


Thanks.
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[David Rientjes]

On Thu, 17 Oct 2013, Chen Gang wrote:

> > I think your patches should be acked before being merged into linux-next, 
> > Hugh just had to revert another one that did affect Linus's tree in 
> > 1ecfd533f4c5 ("mm/mremap.c: call pud_free() after fail calling
> > pmd_alloc()").  I had to revert your entire series of mpol_to_str() 
> > changes in -mm.  It's getting ridiculous and a waste of other people's 
> > time.
> > 
> 
> If always get no reply, what to do, next?
> 

If nobody ever acks your patches, they probably aren't that important.  At 
the very least, something that nobody has looked at shouldn't be included 
if it's going to introduce a regression.

> But all together, I welcome you to help ack/nack my patches for mm
> sub-system (although I don't know your ack/nack whether have effect or not).
> 

If it touches mm, then there is someone on this list who can ack it and 
you can cc them by looking at the output of scripts/get_maintainer.pl.  If 
nobody is interested in it, or if it doesn't do anything important, nobody 
is going to spend their time reviewing it.

I'm not going to continue this thread, the patch in question has been 
removed from -mm so I have no further interest in discussing it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[Chen Gang]

On 10/17/2013 10:21 AM, David Rientjes wrote:
> On Thu, 17 Oct 2013, Chen Gang wrote:
> 
>>> I think your patches should be acked before being merged into linux-next, 
>>> Hugh just had to revert another one that did affect Linus's tree in 
>>> 1ecfd533f4c5 ("mm/mremap.c: call pud_free() after fail calling
>>> pmd_alloc()").  I had to revert your entire series of mpol_to_str() 
>>> changes in -mm.  It's getting ridiculous and a waste of other people's 
>>> time.
>>>
>>
>> If always get no reply, what to do, next?
>>
> 
> If nobody ever acks your patches, they probably aren't that important.  At 
> the very least, something that nobody has looked at shouldn't be included 
> if it's going to introduce a regression.
> 

At least, that is not quite polite.

And when get conclusion, please based on the proofs: "is it necessary to
list them to check whether they are 'important' or not"?


>> But all together, I welcome you to help ack/nack my patches for mm
>> sub-system (although I don't know your ack/nack whether have effect or not).
>>
> 
> If it touches mm, then there is someone on this list who can ack it and 
> you can cc them by looking at the output of scripts/get_maintainer.pl.  If 
> nobody is interested in it, or if it doesn't do anything important, nobody 
> is going to spend their time reviewing it.
> 

Of cause, every time, I send patch according to "scripts/get_maintainer.pl".

So again: "is it necessary to list them to check whether they are
'important' or not?"


> I'm not going to continue this thread, the patch in question has been 
> removed from -mm so I have no further interest in discussing it.
> 
> 

OK, end discussing if you no reply.

Thanks.
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/readahead.c: need always return 0 when system call readahead() succeeds

[Chen Gang]

On 10/17/2013 10:37 AM, Chen Gang wrote:
> On 10/17/2013 10:21 AM, David Rientjes wrote:
>> On Thu, 17 Oct 2013, Chen Gang wrote:
>>
>>>> I think your patches should be acked before being merged into linux-next, 
>>>> Hugh just had to revert another one that did affect Linus's tree in 
>>>> 1ecfd533f4c5 ("mm/mremap.c: call pud_free() after fail calling
>>>> pmd_alloc()").  I had to revert your entire series of mpol_to_str() 
>>>> changes in -mm.  It's getting ridiculous and a waste of other people's 
>>>> time.
>>>>
>>>
>>> If always get no reply, what to do, next?
>>>
>>
>> If nobody ever acks your patches, they probably aren't that important.  At 
>> the very least, something that nobody has looked at shouldn't be included 
>> if it's going to introduce a regression.
>>

Or, need I provide 1-3 patches to let us evaluate whether they are
'important' or not?

> 
> At least, that is not quite polite.
> 
> And when get conclusion, please based on the proofs: "is it necessary to
> list them to check whether they are 'important' or not"?
> 
> 
>>> But all together, I welcome you to help ack/nack my patches for mm
>>> sub-system (although I don't know your ack/nack whether have effect or not).
>>>
>>
>> If it touches mm, then there is someone on this list who can ack it and 
>> you can cc them by looking at the output of scripts/get_maintainer.pl.  If 
>> nobody is interested in it, or if it doesn't do anything important, nobody 
>> is going to spend their time reviewing it.
>>
> 
> Of cause, every time, I send patch according to "scripts/get_maintainer.pl".
> 
> So again: "is it necessary to list them to check whether they are
> 'important' or not?"
> 
> 
>> I'm not going to continue this thread, the patch in question has been 
>> removed from -mm so I have no further interest in discussing it.
>>
>>
> 
> OK, end discussing if you no reply.
> 
> Thanks.
> 


-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] m: readahead: return the value which force_page_cache_readahead() returns

[Chen Gang]

Hello Andrew:

Sorry for may bothering you or other version mergers for these patches
(I am not quit familiar with upstream kernel version merging).

I have sent 2 fix patches for it, if they are not suitable (e.g. let
version merging or regression complex), please tell me how to do next, I
will/should follow.


Thanks.


On 10/15/2013 04:20 PM, Chen Gang wrote:
> 
> This patch fix one issue, but cause 2 issues: *readahead() will return
> read bytes when succeed (just like common read functions).
> 
> One for readahead(), which I already sent related patch for it.
> 
> The other for madvise(), I fix it, just use LTP test it (after finish
> test, I will send fix patch for it).
> 
> 
> Thanks.
> 
> On 08/21/2013 10:41 AM, Chen Gang wrote:
>> force_page_cache_readahead() may fail, so need let the related upper
>> system calls know about it by its return value.
>>
>> For system call fadvise64_64(), ignore return value because fadvise()
>> shall return success even if filesystem can't retrieve a hint.
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  mm/madvise.c   |    4 ++--
>>  mm/readahead.c |    3 +--
>>  2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 936799f..3d0d484 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -247,8 +247,8 @@ static long madvise_willneed(struct vm_area_struct *vma,
>>  		end = vma->vm_end;
>>  	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
>>  
>> -	force_page_cache_readahead(file->f_mapping, file, start, end - start);
>> -	return 0;
>> +	return force_page_cache_readahead(file->f_mapping, file,
>> +					start, end - start);
>>  }
>>  
>>  /*
>> diff --git a/mm/readahead.c b/mm/readahead.c
>> index e4ed041..1b21b5c 100644
>> --- a/mm/readahead.c
>> +++ b/mm/readahead.c
>> @@ -572,8 +572,7 @@ do_readahead(struct address_space *mapping, struct file *filp,
>>  	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
>>  		return -EINVAL;
>>  
>> -	force_page_cache_readahead(mapping, filp, index, nr);
>> -	return 0;
>> +	return force_page_cache_readahead(mapping, filp, index, nr);
>>  }
>>  
>>  SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
>>
> 
> 


-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3] mm: readahead: check return value which force_page_cache_readahead() returns

[Chen Gang]

force_page_cache_readahead() may fail, so the callers who want to know
about it need check the return value.

force_page_cache_readahead() need not return actual read length, since
no callers care about it, and which may lead callers misunderstanding:
treat non-zero as failure.

And now, 2 callers need not check the return value:

 - in fadvise64_64(), it contents the related comment near above.
   (return success even if filesystem can't retrieve a hint).

 - page_cache_sync_readahead() itself need not return value
   (only can not improve performance when it fails).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/madvise.c   |    4 ++--
 mm/readahead.c |   11 +++--------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 539eeb9..dee8d46 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -247,8 +247,8 @@ static long madvise_willneed(struct vm_area_struct *vma,
 		end = vma->vm_end;
 	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
-	force_page_cache_readahead(file->f_mapping, file, start, end - start);
-	return 0;
+	return force_page_cache_readahead(file->f_mapping, file,
+					start, end - start);
 }
 
 /*
diff --git a/mm/readahead.c b/mm/readahead.c
index 7cdbb44..b186d93 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -211,8 +211,6 @@ out:
 int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
 		pgoff_t offset, unsigned long nr_to_read)
 {
-	int ret = 0;
-
 	if (unlikely(!mapping->a_ops->readpage && !mapping->a_ops->readpages))
 		return -EINVAL;
 
@@ -227,14 +225,12 @@ int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
 		err = __do_page_cache_readahead(mapping, filp,
 						offset, this_chunk, 0);
 		if (err < 0) {
-			ret = err;
-			break;
+			return err;
 		}
-		ret += err;
 		offset += this_chunk;
 		nr_to_read -= this_chunk;
 	}
-	return ret;
+	return 0;
 }
 
 /*
@@ -576,8 +572,7 @@ do_readahead(struct address_space *mapping, struct file *filp,
 	if (!mapping || !mapping->a_ops)
 		return -EINVAL;
 
-	force_page_cache_readahead(mapping, filp, index, nr);
-	return 0;
+	return force_page_cache_readahead(mapping, filp, index, nr);
 }
 
 SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count)
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>