Skip to contentSkip to conversation

->direct_IO API change in current 2.4 BK

DISCUSSION Christoph Hellwig 7 participants 27 messages Jul 9, 2003 – Jul 10, 2003

Subsystems (5)
Memory management Documentation VFS NFS client +1 more subsystems
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

But what's the use of it anyway? AFAIK it's mostly for whoracle setups that have their data on netapps but that needs a certified vendor kernel not mainline..

#1 lore raw
Quoted 4 lines · Christoph Hellwig

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

I would have to agree with that sentiment - we shouldn't change the API in an "almost compatible" way, although I would have hoped that compile warnings and/or module symbol versioning would have avoided a crash.

Quoted 3 lines

But what's the use of it anyway? AFAIK it's mostly for whoracle setups that have their data on netapps but that needs a certified vendor kernel not mainline..

Actually, it is useful for Lustre to do this, because it allows us to have a file handle (which, naturally, holds per-file data) at the time the IO is sent over the wire, instead of the "anonymous" writes that happen now. This helps us with readahead on the server and other minor improvements.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

#2 lore raw
Quoted 11 lines · Andreas Dilger
Quoted 4 lines · Christoph Hellwig

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

I would have to agree with that sentiment - we shouldn't change the API in an "almost compatible" way, although I would have hoped that compile warnings and/or module symbol versioning would have avoided a crash.

Quoted 8 lines
Quoted 3 lines

But what's the use of it anyway? AFAIK it's mostly for whoracle setups that have their data on netapps but that needs a certified vendor kernel not mainline..

Actually, it is useful for Lustre to do this, because it allows us to have a file handle (which, naturally, holds per-file data) at the time the IO is sent over the wire, instead of the "anonymous" writes that happen now. This helps us with readahead on the server and other minor improvements.

Fine, I agree. Trond, I'll have to revert your direct IO patches because they break the _stable_ API, indeed.

Please come up with another solution for the problem (->direct_IO2 ? its ugly, but...).

#3 lore raw

On Wednesday 09 July 2003 19:24, Marcelo Tosatti wrote:

Hi,

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

I wonder why -aa and -wolk don't have these problems with O_DIRECT vs. XFS.

ciao, Marc

#4 lore raw
Quoted 9 lines · Marc-Christian Petersen

On Wednesday 09 July 2003 19:24, Marcelo Tosatti wrote:

Hi,

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

I wonder why -aa and -wolk don't have these problems with O_DIRECT vs. XFS.

Do they have the NFS DIRECT IO patch?

#5 lore raw

On Wednesday 09 July 2003 19:46, Marcelo Tosatti wrote:

Hi Marcelo,

Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

I wonder why -aa and -wolk don't have these problems with O_DIRECT vs. XFS.

Do they have the NFS DIRECT IO patch?

Yes. If not, my sentence would be superflous ;)

ciao, Marc

#6 lore raw
Quoted 10 lines · Marc-Christian Petersen

On Wednesday 09 July 2003 19:46, Marcelo Tosatti wrote:

Hi Marcelo,

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

I just got a nice XFS oops due to the direct_IO API change in 2.4. Guys, this is a STABLE series and APIs are supposed to be exactly that, _STABLE_. If you really think O_DIRECT on NFS is soo important please add a ->direct_IO2 for NFS like the reiserfs read_inode2 hack.

Quoted 4 lines
Quoted 3 lines
Quoted 2 lines

I wonder why -aa and -wolk don't have these problems with O_DIRECT vs. XFS.

Do they have the NFS DIRECT IO patch?

Yes. If not, my sentence would be superflous ;)

Ok, right. Well, I dont know why it doesnt happen there. Maybe not enough testing?

Anyway, I'm going to revert the NFS DIRECT IO patch because, as Christoph mentioned, breaks the API.

I except another solution from Trond (maybe ->direct_IO2).

#7 lore raw

On Wednesday 09 July 2003 20:08, Marcelo Tosatti wrote:

Hi Marcelo,

Quoted 2 lines

Ok, right. Well, I dont know why it doesnt happen there. Maybe not enough testing?

aehm, I'll bet -aa is tested _alot_ and also -wolk has tons of testers, but well, this is a different talk ;)

Quoted 3 lines

Anyway, I'm going to revert the NFS DIRECT IO patch because, as Christoph mentioned, breaks the API.
I except another solution from Trond (maybe ->direct_IO2).

I wonder why you merge such stuff then in the first place, if you agree with hch's opinion, that an API should not change in stable kernel series in the 2nd place? ... Did you temporarly forget it? ;)

ciao, Marc

#8 lore raw
Quoted 9 lines · Marc-Christian Petersen

On Wednesday 09 July 2003 20:08, Marcelo Tosatti wrote:

Hi Marcelo,

Quoted 2 lines

Ok, right. Well, I dont know why it doesnt happen there. Maybe not enough testing?

aehm, I'll bet -aa is tested _alot_ and also -wolk has tons of testers, but well, this is a different talk ;)

Quoted 7 lines
Quoted 3 lines

Anyway, I'm going to revert the NFS DIRECT IO patch because, as Christoph mentioned, breaks the API.
I except another solution from Trond (maybe ->direct_IO2).

I wonder why you merge such stuff then in the first place, if you agree with hch's opinion, that an API should not change in stable kernel series in the 2nd place? ... Did you temporarly forget it? ;)

No, I did not.

I applied it because, in my ignorance, I did not noticed it would break the stable API.

I applied it because I wanted comments useful from people (Like hch and others did).

Thank you very much for your flaming.

#9 lore raw

On Wednesday 09 July 2003 21:13, Marcelo Tosatti wrote:

Hi Marcelo,

Quoted 6 lines

No, I did not.
I applied it because, in my ignorance, I did not noticed it would break the stable API.
I applied it because I wanted comments useful from people (Like hch and others did).
Thank you very much for your flaming.

Somewhere deep in my head I knew you'd say that :-( .. When I flame, it looks very different.

It was not any kind of flaming or any kind of bad meaning or such, just a simple question because I was just curious!

ciao, Marc

#10 lore raw
In reply to Marcelo Tosatti
Quoted 5 lines · Marcelo Tosatti

I applied it because, in my ignorance, I did not noticed it would break the stable API.

I applied it because I wanted comments useful from people (Like hch and others did).

I'm not sure I see what the fuss is about a slight API change that is safe since it spews warnings/breaks existing code that isnt fixed. At least one vendor kernel also has the changed API anyway

#11 lore raw
Quoted 5 lines · Alan Cox
Quoted 3 lines · Marcelo Tosatti

I applied it because, in my ignorance, I did not noticed it would break the stable API.

Quoted 7 lines
Quoted 2 lines

I applied it because I wanted comments useful from people (Like hch and others did).

I'm not sure I see what the fuss is about a slight API change that is safe since it spews warnings/breaks existing code that isnt fixed. At least one vendor kernel also has the changed API anyway

"safe" ignores the pain of people trying to support multiple kernels. Each API change like the direct_IO one introduces ifdefs. Changing a function prototype is particularly annoying because you can't create a backwards-compat wrapper

I disagree with the AC97 codec changes being merged into 2.4, too, for the same reason. Yes I recognize it is required to support new hardware. Yes I realize it vastly simplifies supporting some existing hardware. But I don't think you realize (or don't care?) about the maintenance pain created by the change. If a vendor wishes their driver to support 2.4.21 _and_ 2.4.22 (not a lot to ask), they must add a bunch of ifdef crud in their OSS driver.

Feature and API additions are _far_ less painful than API changes in the middle of a stable series.

Overall, I think we are looking at a question which needs to be answered by the community: what constitutes a stable series? when do we stop changing the API and let it stabilize? ... and I am writing a mail right now to ask that question (as requested by Marcelo and a couple others, though I wanted to do it for a while now).

Jeff

#12 lore raw
In reply to Marcelo Tosatti
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Marcelo Tosatti <[email protected]> writes:

> Ok, right. Well, I dont know why it doesnt happen there. Maybe > not enough testing?

Probably nobody is applying the XFS patches on those kernel?

Cheers,
Trond

#13 lore raw

On Wednesday 09 July 2003 20:33, Trond Myklebust wrote:

Hi Trond,

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Marcelo Tosatti <[email protected]> writes:

> Ok, right. Well, I dont know why it doesnt happen there. Maybe > not enough testing?
Probably nobody is applying the XFS patches on those kernel?

err, -aa has XFS per default, -wolk has XFS per default. So ... ;)

ciao, Marc

#14 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Marc-Christian Petersen <[email protected]> writes:

> err, -aa has XFS per default, -wolk has XFS per default. So > ... ;)

So they have both XFS + NFS O_DIRECT?

The answer to your question is then that somebody made the trivial conversion on XFS... It's just a question of replacing the second argument of the direct_IO() method with a filp, then extracting the inode from that. A 2-liner patch at most...

The point here is that Marcelo's tree does not include XFS, so my patch can't fix it up...
As I said, I suggest replacing KERNEL_HAS_O_DIRECT with KERNEL_HAS_O_DIRECT2 so that the XFS patches can switch on that, and hence provide the 2-liner on newer kernels...

Cheers,
Trond

#15 lore raw

On Wednesday 09 July 2003 20:50, Trond Myklebust wrote:

Hi Trond,

Quoted 1 line

So they have both XFS + NFS O_DIRECT?

yes.

Quoted 4 lines

The answer to your question is then that somebody made the trivial conversion on XFS... It's just a question of replacing the second argument of the direct_IO() method with a filp, then extracting the inode from that. A 2-liner patch at most...

EXACT! That was my intention with my small first post ;)

Quoted 5 lines

The point here is that Marcelo's tree does not include XFS, so my patch can't fix it up...
As I said, I suggest replacing KERNEL_HAS_O_DIRECT with KERNEL_HAS_O_DIRECT2 so that the XFS patches can switch on that, and hence provide the 2-liner on newer kernels...

It's very okay with me.

ciao, Marc

#16 lore raw
In reply to Trond Myklebust
Quoted 12 lines · Trond Myklebust
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Marc-Christian Petersen <[email protected]> writes:

> err, -aa has XFS per default, -wolk has XFS per default. So > ... ;)

So they have both XFS + NFS O_DIRECT?

The answer to your question is then that somebody made the trivial conversion on XFS... It's just a question of replacing the second argument of the direct_IO() method with a filp, then extracting the inode from that. A 2-liner patch at most...

Quoted 5 lines

The point here is that Marcelo's tree does not include XFS, so my patch can't fix it up...
As I said, I suggest replacing KERNEL_HAS_O_DIRECT with KERNEL_HAS_O_DIRECT2 so that the XFS patches can switch on that, and hence provide the 2-liner on newer kernels...

s/replacing/adding/

A new ->direct_IO2 hook would be an addition, so you really want to simply add another feature flag.

Since the 2.5 direct_IO API is already different from current 2.4, I would also suggestion considering KERNEL24_HAS_O_DIRECT2 as the name, to specify the feature is specific to 2.4.

Jeff

#17 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Jeff Garzik <[email protected]> writes:

> s/replacing/adding/

Revert ;-)

> A new ->direct_IO2 hook would be an addition, so you really > want to simply add another feature flag.

You missed the point. This is *instead* of ->direct_IO2. It's only purpose would be to distinguish between the old

int (*direct_IO)(int, struct inode *, struct kiobuf *, unsigned long, int);

and the new

int (*direct_IO)(int, struct file *, struct kiobuf *, unsigned long, int);

Cheers,
Trond

#18 lore raw
Quoted 12 lines · Trond Myklebust
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Jeff Garzik <[email protected]> writes:

> s/replacing/adding/

Revert ;-)

> A new ->direct_IO2 hook would be an addition, so you really > want to simply add another feature flag.

You missed the point. This is *instead* of ->direct_IO2. It's only purpose would be to distinguish between the old

Quoted 6 lines

int (*direct_IO)(int, struct inode *, struct kiobuf *, unsigned long, int);

and the new

int (*direct_IO)(int, struct file *, struct kiobuf *, unsigned long, int);

I respectfully disagree, then.

The 2.5 direct_IO API is already different from 2.4, so, changing the 2.4 stable API implies creating yet another different version of the API.

When this situation presented itself earlier, with reiserfs, the solution was ->read_inode2. I think that same situation applies here.

Having the stable API change, conditional on a define, is really nasty and IMO will create maintenance and support headaches down the line. I do not recall Linux VFS _ever_ having a hook's definition conditional. We should not start now...

We need a 2.4-specific solution for this. ->direct_IO2 should suffice, and it follows historical example.

XFS, ocfs, and others need only to test the HAVE_NEW_24_DIRECT_IO define.

In the core kernel implementation, it is trivial to say "if direct_IO2 is non-NULL, use that, otherwise use direct_IO", without needing to make any code conditional at all.

Jeff

#19 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Jeff Garzik <[email protected]> writes:

> Having the stable API change, conditional on a define, is > really nasty and IMO will create maintenance and support > headaches down the line. I do not recall Linux VFS _ever_ > having a hook's definition conditional. We should not start > now...

direct_IO() was precisely such a conditional hook definition. It appeared in 2.4.15, and anybody who does not check for KERNEL_HAS_O_DIRECT is not backward compatible.

To comment further: There is at least one example I can think of which was exactly equivalent to the proposed change, namely the redefinition of the filldir_t type in 2.4.9. It was admittedly not documented using a define...

Note: We could at the same time replace the name direct_IO() with direct_IO2() (that has several precedents). There are currently only a small number of filesystems that provide O_DIRECT, and converting them all is (as has been pointed out before) trivial...

The problem with read_inode2() was rather that it overloaded the the existing iget4() interface...

Cheers,
Trond

#20 lore raw
Quoted 9 lines · Trond Myklebust
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Jeff Garzik <[email protected]> writes:

> Having the stable API change, conditional on a define, is > really nasty and IMO will create maintenance and support > headaches down the line. I do not recall Linux VFS _ever_ > having a hook's definition conditional. We should not start > now...

Quoted 3 lines

direct_IO() was precisely such a conditional hook definition. It appeared in 2.4.15, and anybody who does not check for KERNEL_HAS_O_DIRECT is not backward compatible.

You misunderstand. The 2.4.15 direct_IO hook was _not_ conditionally defined. It appeared in the middle of a stable series, yes. It has a feature macro, yes. But the definition of the hook in include/linux/fs.h does not _change_ based on a define. That is what I mean by a conditional hook definition.

It is far less trouble for everyone to add a new hook, instead of changing an existing hook, in the middle of a stable series.

Quoted 4 lines

To comment further: There is at least one example I can think of which was exactly equivalent to the proposed change, namely the redefinition of the filldir_t type in 2.4.9. It was admittedly not documented using a define...

No doubt you can find more :) That doesn't make the right thing to do, though :)

Quoted 4 lines

Note: We could at the same time replace the name direct_IO() with direct_IO2() (that has several precedents). There are currently only a small number of filesystems that provide O_DIRECT, and converting them all is (as has been pointed out before) trivial...

We cannot just-fix-up filesystems which are not in-tree, which is what the KERNEL_HAS_O_DIRECT2 define would be mainly used for. In-tree filesystems would just unconditionally use the new, or old, interface as they chose.

Quoted 2 lines

The problem with read_inode2() was rather that it overloaded the the existing iget4() interface...

The higher-level problem was that we didn't want to change the VFS API... otherwise we could have simply used the new interface, and converted all in-tree filesystems.

Jeff

#21 lore raw
In reply to Jeff Garzik
Quoted 4 lines · Jeff Garzik

Having the stable API change, conditional on a define, is really nasty and IMO will create maintenance and support headaches down the line. I do not recall Linux VFS _ever_ having a hook's definition conditional. We should not start now...

The new quota code in 2.4.22pre already changed the rules slightly, as do the shmemfs fixes if you are pedantic

#22 lore raw
Quoted 10 lines · Alan Cox
Quoted 4 lines · Jeff Garzik

Having the stable API change, conditional on a define, is really nasty and IMO will create maintenance and support headaches down the line. I do not recall Linux VFS _ever_ having a hook's definition conditional. We should not start now...

The new quota code in 2.4.22pre already changed the rules slightly, as do the shmemfs fixes if you are pedantic

I am ;-)

#23 lore raw
In reply to Trond Myklebust
Quoted 6 lines · Trond Myklebust
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Marc-Christian Petersen <[email protected]> writes:

> err, -aa has XFS per default, -wolk has XFS per default. So > ... ;)

So they have both XFS + NFS O_DIRECT?

-ac has both

#24 lore raw
In reply to Marcelo Tosatti
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

" " == Marcelo Tosatti <[email protected]> writes:

> Fine, I agree. Trond, I'll have to revert your direct IO > patches because they break the _stable_ API, indeed.

> Please come up with another solution for the problem > (->direct_IO2 ? its ugly, but...).

Like this? Blech...

How about instead following Alan's suggestion to replace KERNEL_HAS_O_DIRECT with a KERNEL_HAS_O_DIRECT2 that can be used to switch between the old and new O_DIRECT format in the XFS patches?

Cheers,
Trond

Documentation/Configure.help +24 -0
diff -u --recursive --new-file linux-2.4.22-nodirect/Documentation/Configure.help linux-2.4.22-blech/Documentation/Configure.help
@@ -15938,6 +15938,30 @@
 
   If unsure, say N.
 
+Allow direct I/O on files in NFS
+CONFIG_NFS_DIRECTIO
+  There are important applications whose performance or correctness
+  depends on uncached access to file data.  Database clusters (multiple
+  copies of the same instance running on separate hosts) implement their
+  own cache coherency protocol that subsumes the NFS cache protocols.
+  Applications that process datasets considerably larger than the client's
+  memory do not always benefit from a local cache.  A streaming video
+  server, for instance, has no need to cache the contents of a file.
+
+  This option enables applications to perform direct I/O on files in NFS
+  file systems using the O_DIRECT open() flag.  When O_DIRECT is set for
+  files, their data is not cached in the system's page cache.  Direct
+  read and write operations are aligned to block boundaries.  Data is
+  moved to and from user-level application buffers directly.
+
+  Unless your program is designed to use O_DIRECT properly, you are much
+  better off allowing the NFS client to manage caching for you.  Misusing
+  O_DIRECT can cause poor server performance or network storms.  This
+  kernel build option defaults OFF to avoid exposing system administrators
+  unwittingly to a potentially hazardous feature.
+
+  If unsure, say N.
+
 Root file system on NFS
 CONFIG_ROOT_NFS
   If you want your Linux box to mount its whole root file system (the
fs/Config.in +1 -0
diff -u --recursive --new-file linux-2.4.22-nodirect/fs/Config.in linux-2.4.22-blech/fs/Config.in
@@ -106,6 +106,7 @@
    dep_tristate 'InterMezzo file system support (replicating fs) (EXPERIMENTAL)' CONFIG_INTERMEZZO_FS $CONFIG_INET $CONFIG_EXPERIMENTAL
    dep_tristate 'NFS file system support' CONFIG_NFS_FS $CONFIG_INET
    dep_mbool '  Provide NFSv3 client support' CONFIG_NFS_V3 $CONFIG_NFS_FS
+   dep_mbool '  Allow direct I/O on NFS files (EXPERIMENTAL)' CONFIG_NFS_DIRECTIO $CONFIG_NFS_FS $CONFIG_EXPERIMENTAL
    dep_bool '  Root file system on NFS' CONFIG_ROOT_NFS $CONFIG_NFS_FS $CONFIG_IP_PNP
 
    dep_tristate 'NFS server support' CONFIG_NFSD $CONFIG_INET
fs/nfs/direct.c +382 -0

382 lines not on this page · raw message

fs/nfs/file.c +4 -0
diff -u --recursive --new-file linux-2.4.22-nodirect/fs/nfs/file.c linux-2.4.22-blech/fs/nfs/file.c
@@ -16,6 +16,7 @@
  *  nfs regular file handling functions
  */
 
+#include <linux/config.h>
 #include <linux/sched.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>

@@ -199,6 +200,9 @@
 	readpage: nfs_readpage,
 	sync_page: nfs_sync_page,
 	writepage: nfs_writepage,
+#ifdef CONFIG_NFS_DIRECTIO
+	direct_fileIO: nfs_direct_IO,
+#endif
 	prepare_write: nfs_prepare_write,
 	commit_write: nfs_commit_write
 };
fs/nfs/Makefile +1 -0
diff -u --recursive --new-file linux-2.4.22-nodirect/fs/nfs/Makefile linux-2.4.22-blech/fs/nfs/Makefile
@@ -14,6 +14,7 @@
 
 obj-$(CONFIG_ROOT_NFS) += nfsroot.o mount_clnt.o      
 obj-$(CONFIG_NFS_V3) += nfs3proc.o nfs3xdr.o
+obj-$(CONFIG_NFS_DIRECTIO) += direct.o
 
 obj-m   := $(O_TARGET)
fs/nfs/write.c +0 -17
diff -u --recursive --new-file linux-2.4.22-nodirect/fs/nfs/write.c linux-2.4.22-blech/fs/nfs/write.c
@@ -123,23 +123,6 @@
 }
 
 /*
- * This function will be used to simulate weak cache consistency
- * under NFSv2 when the NFSv3 attribute patch is included.
- * For the moment, we just call nfs_refresh_inode().
- */
-static __inline__ int
-nfs_write_attributes(struct inode *inode, struct nfs_fattr *fattr)
-{
-	if ((fattr->valid & NFS_ATTR_FATTR) && !(fattr->valid & NFS_ATTR_WCC)) {
-		fattr->pre_size  = NFS_CACHE_ISIZE(inode);
-		fattr->pre_mtime = NFS_CACHE_MTIME(inode);
-		fattr->pre_ctime = NFS_CACHE_CTIME(inode);
-		fattr->valid |= NFS_ATTR_WCC;
-	}
-	return nfs_refresh_inode(inode, fattr);
-}
-
-/*
  * Write a page synchronously.
  * Offset is the data offset within the page.
  */
include/linux/fs.h +2 -0
diff -u --recursive --new-file linux-2.4.22-nodirect/include/linux/fs.h linux-2.4.22-blech/include/linux/fs.h
@@ -396,6 +396,8 @@
 	int (*releasepage) (struct page *, int);
 #define KERNEL_HAS_O_DIRECT /* this is for modules out of the kernel */
 	int (*direct_IO)(int, struct inode *, struct kiobuf *, unsigned long, int);
+#define KERNEL_HAS_O_DIRECT2 /* Unfortunate kludge due to lack of foresight */
+	int (*direct_fileIO)(int, struct file *, struct kiobuf *, unsigned long, int);
 	void (*removepage)(struct page *); /* called when page gets removed from the inode */
 };
include/linux/nfs_fs.h +22 -0
diff -u --recursive --new-file linux-2.4.22-nodirect/include/linux/nfs_fs.h linux-2.4.22-blech/include/linux/nfs_fs.h
@@ -274,6 +274,11 @@
 #define NFS_TestClearPageSync(page)	test_and_clear_bit(PG_fs_1, &(page)->flags)
 
 /*
+ * linux/fs/nfs/direct.c
+ */
+extern int  nfs_direct_IO(int, struct file *, struct kiobuf *, unsigned long, int);
+
+/*
  * linux/fs/mount_clnt.c
  * (Used only by nfsroot module)
  */

@@ -302,6 +307,23 @@
 	return __nfs_refresh_inode(inode,fattr);
 }
 
+/*
+ * This function will be used to simulate weak cache consistency
+ * under NFSv2 when the NFSv3 attribute patch is included.
+ * For the moment, we just call nfs_refresh_inode().
+ */
+static __inline__ int
+nfs_write_attributes(struct inode *inode, struct nfs_fattr *fattr)
+{
+	if ((fattr->valid & NFS_ATTR_FATTR) && !(fattr->valid & NFS_ATTR_WCC)) {
+		fattr->pre_size  = NFS_CACHE_ISIZE(inode);
+		fattr->pre_mtime = NFS_CACHE_MTIME(inode);
+		fattr->pre_ctime = NFS_CACHE_CTIME(inode);
+		fattr->valid |= NFS_ATTR_WCC;
+	}
+	return nfs_refresh_inode(inode, fattr);
+}
+
 static inline loff_t
 nfs_size_to_loff_t(__u64 size)
 {
include/linux/nfs_xdr.h +2 -2
diff -u --recursive --new-file linux-2.4.22-nodirect/include/linux/nfs_xdr.h linux-2.4.22-blech/include/linux/nfs_xdr.h
@@ -59,7 +59,7 @@
 /* Arguments to the read call.
  * Note that NFS_READ_MAXIOV must be <= (MAX_IOVEC-2) from sunrpc/xprt.h
  */
-#define NFS_READ_MAXIOV 8
+#define NFS_READ_MAXIOV		(9)
 
 struct nfs_readargs {
 	struct nfs_fh *		fh;

@@ -78,7 +78,7 @@
 /* Arguments to the write call.
  * Note that NFS_WRITE_MAXIOV must be <= (MAX_IOVEC-2) from sunrpc/xprt.h
  */
-#define NFS_WRITE_MAXIOV        8
+#define NFS_WRITE_MAXIOV        (9)
 struct nfs_writeargs {
 	struct nfs_fh *		fh;
 	__u64			offset;
mm/filemap.c +97 -2

117 lines not on this page · raw message

#25 lore raw
Quoted 3 lines · Trond Myklebust

How about instead following Alan's suggestion to replace KERNEL_HAS_O_DIRECT with a KERNEL_HAS_O_DIRECT2 that can be used to switch between the old and new O_DIRECT format in the XFS patches?

Actually, I like that a lot more, as it allows out-of-tree code to know which interface to use, and we don't have to key on kernel version (which is bogus if compiling against a vendor kernel that back-ports this fix).

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

#26 lore raw
Quoted 9 lines · Andreas Dilger
Quoted 3 lines · Trond Myklebust

How about instead following Alan's suggestion to replace KERNEL_HAS_O_DIRECT with a KERNEL_HAS_O_DIRECT2 that can be used to switch between the old and new O_DIRECT format in the XFS patches?

Actually, I like that a lot more, as it allows out-of-tree code to know which interface to use, and we don't have to key on kernel version (which is bogus if compiling against a vendor kernel that back-ports this fix).

Feature test macros are definitely quite useful...

...but making the stable series VFS API definition _conditional_ is quite unprecedented, and IMO wrong.

Jeff