linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 5/6] vfs: implement readahead(2) using POSIX_FADV_WILLNEED
@ 2018-08-28  8:00 Amir Goldstein
  2018-08-28 22:02 ` Dave Chinner
  0 siblings, 1 reply; 2+ messages in thread
From: Amir Goldstein @ 2018-08-28  8:00 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Al Viro, Dave Chinner, linux-unionfs, linux-fsdevel

The implementation of readahead(2) syscall is identical to that of
fadvise64(POSIX_FADV_WILLNEED) with a few exceptions:
1. readahead(2) returns -EINVAL for !mapping->a_ops and fadvise64()
   ignores the request and returns 0.
2. fadvise64() checks for integer overflow corner case
3. fadvise64() calls the optional filesystem fadvice() file operation

Unite the two implementations by calling vfs_fadvice() from readahead(2)
syscall. Check the !mapping->a_ops in readahead(2) syscall to preserve
legacy behavior.

Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: d1d04ef8572b ("ovl: stack file ops")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

Following v4 addresses comment from dchinner.

Thanks,
Amir.

Changes from v3:
- fold do_readahead helper and reduce code nesting

 mm/readahead.c | 45 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index a59ea70527b9..a9cda21aeb05 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -20,6 +20,7 @@
 #include <linux/file.h>
 #include <linux/mm_inline.h>
 #include <linux/blk-cgroup.h>
+#include <linux/fadvise.h>
 
 #include "internal.h"
 
@@ -575,24 +576,6 @@ page_cache_async_readahead(struct address_space *mapping,
 }
 EXPORT_SYMBOL_GPL(page_cache_async_readahead);
 
-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;
-
-	/*
-	 * Readahead doesn't make sense for DAX inodes, but we don't want it
-	 * to report a failure either.  Instead, we just return success and
-	 * don't do any work.
-	 */
-	if (dax_mapping(mapping))
-		return 0;
-
-	return force_page_cache_readahead(mapping, filp, index, nr);
-}
-
 ssize_t ksys_readahead(int fd, loff_t offset, size_t count)
 {
 	ssize_t ret;
@@ -600,16 +583,22 @@ ssize_t ksys_readahead(int fd, loff_t offset, size_t count)
 
 	ret = -EBADF;
 	f = fdget(fd);
-	if (f.file) {
-		if (f.file->f_mode & FMODE_READ) {
-			struct address_space *mapping = f.file->f_mapping;
-			pgoff_t start = offset >> PAGE_SHIFT;
-			pgoff_t end = (offset + count - 1) >> PAGE_SHIFT;
-			unsigned long len = end - start + 1;
-			ret = do_readahead(mapping, f.file, start, len);
-		}
-		fdput(f);
-	}
+	if (!f.file || !(f.file->f_mode & FMODE_READ))
+		goto out;
+
+	/*
+	 * fadvise() silently ignores an advice for a file with !a_ops and
+	 * returns -EPIPE for a pipe. Keep this check here to comply with legacy
+	 * -EINVAL behavior of readahead(2).
+	 */
+	ret = -EINVAL;
+	if (!f.file->f_mapping || !f.file->f_mapping->a_ops ||
+	    !S_ISREG(file_inode(f.file)->i_mode))
+		goto out;
+
+	ret = vfs_fadvise(f.file, offset, count, POSIX_FADV_WILLNEED);
+out:
+	fdput(f);
 	return ret;
 }
 
-- 
2.7.4

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v4 5/6] vfs: implement readahead(2) using POSIX_FADV_WILLNEED
  2018-08-28  8:00 [PATCH v4 5/6] vfs: implement readahead(2) using POSIX_FADV_WILLNEED Amir Goldstein
@ 2018-08-28 22:02 ` Dave Chinner
  0 siblings, 0 replies; 2+ messages in thread
From: Dave Chinner @ 2018-08-28 22:02 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, Al Viro, linux-unionfs, linux-fsdevel

On Tue, Aug 28, 2018 at 11:00:33AM +0300, Amir Goldstein wrote:
> The implementation of readahead(2) syscall is identical to that of
> fadvise64(POSIX_FADV_WILLNEED) with a few exceptions:
> 1. readahead(2) returns -EINVAL for !mapping->a_ops and fadvise64()
>    ignores the request and returns 0.
> 2. fadvise64() checks for integer overflow corner case
> 3. fadvise64() calls the optional filesystem fadvice() file operation
                                               fadvise
> 
> Unite the two implementations by calling vfs_fadvice() from readahead(2)
					   vfs_fadvise

> syscall. Check the !mapping->a_ops in readahead(2) syscall to preserve
> legacy behavior.

It's not "legacy behaviour" - it's "documented syscall ABI behaviour"
> 
> Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
> Fixes: d1d04ef8572b ("ovl: stack file ops")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Miklos,
> 
> Following v4 addresses comment from dchinner.

More comments below...

> @@ -600,16 +583,22 @@ ssize_t ksys_readahead(int fd, loff_t offset, size_t count)
>  
>  	ret = -EBADF;
>  	f = fdget(fd);
> -	if (f.file) {
> -		if (f.file->f_mode & FMODE_READ) {
> -			struct address_space *mapping = f.file->f_mapping;
> -			pgoff_t start = offset >> PAGE_SHIFT;
> -			pgoff_t end = (offset + count - 1) >> PAGE_SHIFT;
> -			unsigned long len = end - start + 1;
> -			ret = do_readahead(mapping, f.file, start, len);
> -		}
> -		fdput(f);
> -	}
> +	if (!f.file || !(f.file->f_mode & FMODE_READ))
> +		goto out;
> +
> +	/*
> +	 * fadvise() silently ignores an advice for a file with !a_ops and
> +	 * returns -EPIPE for a pipe. Keep this check here to comply with legacy
> +	 * -EINVAL behavior of readahead(2).
> +	 */
> +	ret = -EINVAL;
> +	if (!f.file->f_mapping || !f.file->f_mapping->a_ops ||
> +	    !S_ISREG(file_inode(f.file)->i_mode))
> +		goto out;

This is not the place to document vfs_advise() behaviour - this is
where you document the readahead syscall error requirements.
"Legacy -EINVAL behaviour" is not a useful description outside of
this specific patch context.

	/*
	 * The readahead() syscall is intended to run only on files
	 * that can execute readahead. If readahead is not possible
	 * on this file, then we must return -EINVAL.
	 */


Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-08-29  1:55 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-28  8:00 [PATCH v4 5/6] vfs: implement readahead(2) using POSIX_FADV_WILLNEED Amir Goldstein
2018-08-28 22:02 ` Dave Chinner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).