mm: readahead: return the value which force_page_cache_readahead() returns

Quoted 5 lines · On Tue, 20 Aug 2013 11:31:52 +0800 Chen Gang <[email protected]> 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.

mm/fadvise.c SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice) · line 109

Quoted 6 lines

mm/fadvise.c +0 -0

@@ -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.

mm/fadvise.c SYSCALL_DEFINE4(fadvise64_64, int, fd, loff_t, offset, loff_t, len, int, advice) · line 112

Quoted 5 lines

mm/fadvise.c +2 -2

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

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 <[email protected]>

---
 mm/madvise.c   |    4 ++--
 mm/readahead.c |    3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

mm/madvise.c +2 -2

index 936799f..3d0d484 100644
@@ -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);
 }
 
 /*

mm/readahead.c +1 -2

index e4ed041..1b21b5c 100644
@@ -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)

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.

Quoted 3 lines · On Wed, 21 Aug 2013 10:41:20 +0800 Chen Gang <[email protected]> wrote:

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

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

---

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

diff -puN mm/readahead.c~a mm/readahead.c

mm/readahead.c renamed from mm/readahead.c~a +1 -1

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

_

mm/readahead.c SYSCALL_DEFINE3(readahead, int, fd, loff_t, offset, size_t, count) · line 596

Quoted 11 lines · Chen Gang

mm/readahead.c +1 -1

index 1eee42b..83a202e 100644
@@ -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.

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

Quoted 5 lines

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()?

Quoted 3 lines · Chen Gang

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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?

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).

Quoted 8 lines · Chen Gang

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 10 lines · David Rientjes

Quoted 8 lines · Chen Gang

Quoted 7 lines

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.

Quoted 7 lines

Quoted 2 lines

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"?

Quoted 9 lines

Quoted 3 lines

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?"

Quoted 4 lines

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.

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 <[email protected]>

---
 mm/madvise.c   |    4 ++--
 mm/readahead.c |    3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

mm/madvise.c +2 -2

index 936799f..3d0d484 100644
@@ -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);
 }
 
 /*

mm/readahead.c +1 -2

index e4ed041..1b21b5c 100644
@@ -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)

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.

Quoted 42 lines · 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 <[email protected]>

---
 mm/madvise.c   |    4 ++--
 mm/readahead.c |    3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

mm/madvise.c +2 -2

index 936799f..3d0d484 100644
@@ -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);
 }
 
 /*

mm/readahead.c +1 -2

index e4ed041..1b21b5c 100644
@@ -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)