[PATCH] mm: readahead: fix do_readahead for no readpage(s)

[PATCH] mm: readahead: fix do_readahead for no readpage(s)

[Mark Rutland]

Commit 63d0f0a3c7e1 (mm/readahead.c:do_readhead(): don't check for
->readpage) unintentionally made do_readahead return 0 for all valid
files regardless of whether readahead was supported, rather than the
expected -EINVAL. This gets forwarded on to userspace, and results in
sys_readahead appearing to succeed in cases that don't make sense (e.g.
when called on pipes or sockets). This issue is detected by the LTP
readahead01 testcase.

As the exact return value of force_page_cache_readahead is currently
never used, we can simplify it to return only 0 or -EINVAL (when
readpage or readpages is missing). With that in place we can simply
forward on the return value of force_page_cache_readahead in
do_readahead.

This patch performs said change, restoring the expected semantics.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/readahead.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 7cdbb44..0de2360d 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;
 
@@ -226,15 +224,13 @@ int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
 			this_chunk = nr_to_read;
 		err = __do_page_cache_readahead(mapping, filp,
 						offset, this_chunk, 0);
-		if (err < 0) {
-			ret = err;
-			break;
-		}
-		ret += err;
+		if (err < 0)
+			return 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.8.1.1

Re: [PATCH] mm: readahead: fix do_readahead for no readpage(s)

[Kirill A. Shutemov]

On Tue, Jan 28, 2014 at 11:14:19AM +0000, Mark Rutland wrote:
> Commit 63d0f0a3c7e1 (mm/readahead.c:do_readhead(): don't check for
> ->readpage) unintentionally made do_readahead return 0 for all valid
> files regardless of whether readahead was supported, rather than the
> expected -EINVAL. This gets forwarded on to userspace, and results in
> sys_readahead appearing to succeed in cases that don't make sense (e.g.
> when called on pipes or sockets). This issue is detected by the LTP
> readahead01 testcase.
> 
> As the exact return value of force_page_cache_readahead is currently
> never used, we can simplify it to return only 0 or -EINVAL (when
> readpage or readpages is missing). With that in place we can simply
> forward on the return value of force_page_cache_readahead in
> do_readahead.
> 
> This patch performs said change, restoring the expected semantics.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [PATCH] mm: readahead: fix do_readahead for no readpage(s)

[Andrew Morton]

On Tue, 28 Jan 2014 11:14:19 +0000 Mark Rutland <mark.rutland@arm.com> wrote:

> Commit 63d0f0a3c7e1 (mm/readahead.c:do_readhead(): don't check for
> ->readpage) unintentionally made do_readahead return 0 for all valid
> files regardless of whether readahead was supported, rather than the
> expected -EINVAL. This gets forwarded on to userspace, and results in
> sys_readahead appearing to succeed in cases that don't make sense (e.g.
> when called on pipes or sockets). This issue is detected by the LTP
> readahead01 testcase.

How can this be?

: static ssize_t
: do_readahead(struct address_space *mapping, struct file *filp,
: 	     pgoff_t index, unsigned long nr)
: {
: 	if (!mapping || !mapping->a_ops)
: 		return -EINVAL;
: 
: 	return force_page_cache_readahead(mapping, filp, index, nr);
: }

and

: int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
: 		pgoff_t offset, unsigned long nr_to_read)
: {
: 	if (unlikely(!mapping->a_ops->readpage && !mapping->a_ops->readpages))
: 		return -EINVAL;

Clearly, do_readahead() will return -EINVAL if neither ->readpage or
->readpages are implemented.

I can see that the behaviour would change if the address_space
implements only one of ->readpage and ->readpages, but that doesn't
appear to match your description and the new behaviour is correct - we
can now perform readahead for address_spaces which implement
->readpages and not ->readpage (which would be odd and might not work
for other reasons..).

Re: [PATCH] mm: readahead: fix do_readahead for no readpage(s)

[Kirill A. Shutemov]

On Tue, Jan 28, 2014 at 12:03:01PM -0800, Andrew Morton wrote:
> On Tue, 28 Jan 2014 11:14:19 +0000 Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Commit 63d0f0a3c7e1 (mm/readahead.c:do_readhead(): don't check for
> > ->readpage) unintentionally made do_readahead return 0 for all valid
> > files regardless of whether readahead was supported, rather than the
> > expected -EINVAL. This gets forwarded on to userspace, and results in
> > sys_readahead appearing to succeed in cases that don't make sense (e.g.
> > when called on pipes or sockets). This issue is detected by the LTP
> > readahead01 testcase.
> 
> How can this be?
> 
> : static ssize_t
> : do_readahead(struct address_space *mapping, struct file *filp,
> : 	     pgoff_t index, unsigned long nr)
> : {
> : 	if (!mapping || !mapping->a_ops)
> : 		return -EINVAL;
> : 
> : 	return force_page_cache_readahead(mapping, filp, index, nr);

It's not what we have in Linus' tree. force_page_cache_readahead() return
code is unused:

	force_page_cache_readahead(mapping, filp, index, nr);
	return 0;

> : }
> 
> and
> 
> : int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
> : 		pgoff_t offset, unsigned long nr_to_read)
> : {
> : 	if (unlikely(!mapping->a_ops->readpage && !mapping->a_ops->readpages))
> : 		return -EINVAL;
> 
> Clearly, do_readahead() will return -EINVAL if neither ->readpage or
> ->readpages are implemented.
> 
> I can see that the behaviour would change if the address_space
> implements only one of ->readpage and ->readpages, but that doesn't
> appear to match your description and the new behaviour is correct - we
> can now perform readahead for address_spaces which implement
> ->readpages and not ->readpage (which would be odd and might not work
> for other reasons..).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
 Kirill A. Shutemov

Re: [PATCH] mm: readahead: fix do_readahead for no readpage(s)

[Andrew Morton]

On Tue, 28 Jan 2014 23:01:26 +0200 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Tue, Jan 28, 2014 at 12:03:01PM -0800, Andrew Morton wrote:
> > On Tue, 28 Jan 2014 11:14:19 +0000 Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > Commit 63d0f0a3c7e1 (mm/readahead.c:do_readhead(): don't check for
> > > ->readpage) unintentionally made do_readahead return 0 for all valid
> > > files regardless of whether readahead was supported, rather than the
> > > expected -EINVAL. This gets forwarded on to userspace, and results in
> > > sys_readahead appearing to succeed in cases that don't make sense (e.g.
> > > when called on pipes or sockets). This issue is detected by the LTP
> > > readahead01 testcase.
> > 
> > How can this be?
> > 
> > : static ssize_t
> > : do_readahead(struct address_space *mapping, struct file *filp,
> > : 	     pgoff_t index, unsigned long nr)
> > : {
> > : 	if (!mapping || !mapping->a_ops)
> > : 		return -EINVAL;
> > : 
> > : 	return force_page_cache_readahead(mapping, filp, index, nr);
> 
> It's not what we have in Linus' tree. force_page_cache_readahead() return
> code is unused:
> 
> 	force_page_cache_readahead(mapping, filp, index, nr);
> 	return 0;
> 

ah, oops, I was looking at the code with Mark's patch applied.

Yes, the force_page_cache_readahead() return value should be propagated.

And I guess the lobotomising of the force_page_cache_readahead() return
value is OK.  force_page_cache_readahead() presently returns an errno
even if it has performed some reads.  A read()-style function shouldn't
do that - it should return a short read in that case.