[PATCH][RESEND] don't allow sys_readahead() on files opened with O_DIRECT

[PATCH][RESEND] don't allow sys_readahead() on files opened with O_DIRECT

[Jan Blunck]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

IMO sys_readahead() doesn't make sense if the file is opened with
O_DIRECT, because the page cache is stuffed but never used. Therefore
this patch changes that by letting the call return with -EINVAL.

Signed-off-by: Jan Blunck <j.blunck@tu-harburg.de>


[-- Attachment #2: filemap.c_direct_IO_readahead.diff --]
[-- Type: text/x-patch, Size: 636 bytes --]

 mm/filemap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

Index: experimental-jb/mm/filemap.c
===================================================================
--- experimental-jb.orig/mm/filemap.c
+++ experimental-jb/mm/filemap.c
@@ -1111,7 +1111,8 @@ static ssize_t
 do_readahead(struct address_space *mapping, struct file *filp,
 	     unsigned long index, unsigned long nr)
 {
-	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage)
+	if (!mapping || !mapping->a_ops || !mapping->a_ops->readpage
+	    || (filp->f_flags & O_DIRECT))
 		return -EINVAL;
 
 	force_page_cache_readahead(mapping, filp, index,

Re: [PATCH][RESEND] don't allow sys_readahead() on files opened with O_DIRECT

[Andrew Morton]

Jan Blunck <j.blunck@tu-harburg.de> wrote:
>
> IMO sys_readahead() doesn't make sense if the file is opened with
> O_DIRECT, because the page cache is stuffed but never used. Therefore
> this patch changes that by letting the call return with -EINVAL.
> 

a) It doesn't hurt, it's just a bit of a silly thing to do.

b) posix_fadvise(POSIX_FADV_WILLNEED) should get the same treatment (and
   it's the preferred way of doing readahead).

c) O_DIRECT fd's should, as much as possible, offer the same ABI as
   buffered fd's.

d) The patch could break existing apps.

Re: [PATCH][RESEND] don't allow sys_readahead() on files opened with O_DIRECT

[Jan Blunck]

Andrew Morton schrieb:
> 
> a) It doesn't hurt, it's just a bit of a silly thing to do.

IMO it may hurt the performance.

> 
> b) posix_fadvise(POSIX_FADV_WILLNEED) should get the same treatment (and
>    it's the preferred way of doing readahead).

Yes, of course.

> 
> c) O_DIRECT fd's should, as much as possible, offer the same ABI as
>    buffered fd's.

Hmm, with XIP fd's we agreed on the following behavior: fadvise() and
madvise() just return without reading anything to the page cache. Since
XIP fd's are similar to O_DIRECT fd's their behavior should be similar,
too. If we don't honor the advises we might also ignore the syscall. At
least redhat's readahead is using them.

Maybe we should agree on one behavior that makes sense. And I don't see
any point in filling the page cache when it is not needed.

> 
> d) The patch could break existing apps.

Since it could break applications that are already (some kind of) broken
that shouldn't be a problem.

So you think it is better to read nothing to the page cache and return
zero instead? This seems like "lying" to the user-space :)

Re: [PATCH][RESEND] don't allow sys_readahead() on files opened with O_DIRECT

[Christoph Hellwig]

On Thu, Aug 25, 2005 at 01:24:40AM -0700, Andrew Morton wrote:
> > IMO sys_readahead() doesn't make sense if the file is opened with
> > O_DIRECT, because the page cache is stuffed but never used. Therefore
> > this patch changes that by letting the call return with -EINVAL.
> > 
> 
> a) It doesn't hurt, it's just a bit of a silly thing to do.

For ocfs that only allows either direct or buffered I/O on a single
inode at a given time it hurts.  It messes up the synchronization
protocol to be exact..