NFS client llseek

NFS client llseek

[dzafman]

I noticed that generic_file_llseek looks at i_size without performing a
revalidate, so I propose the following patch for NFS client.  It applies to
both 2.4.16 and 2.5.0 kernels.


diff -Naur linux-2.4.16.orig/fs/nfs/file.c linux-2.4.16/fs/nfs/file.c
--- linux-2.4.16.orig/fs/nfs/file.c	Sun Sep 23 09:48:01 2001
+++ linux-2.4.16/fs/nfs/file.c	Thu Dec 13 15:35:05 2001
@@ -39,9 +39,10 @@
 static ssize_t nfs_file_write(struct file *, const char *, size_t, loff_t *);
 static int  nfs_file_flush(struct file *);
 static int  nfs_fsync(struct file *, struct dentry *dentry, int datasync);
+static loff_t nfs_file_llseek(struct file *, loff_t, int origin);
 
 struct file_operations nfs_file_operations = {
-	llseek:		generic_file_llseek,
+	llseek:		nfs_file_llseek,
 	read:		nfs_file_read,
 	write:		nfs_file_write,
 	mmap:		nfs_file_mmap,
@@ -142,6 +143,24 @@
 	}
 	unlock_kernel();
 	return status;
+}
+
+static loff_t
+nfs_file_llseek(struct file *file, loff_t offset, int origin)
+{
+	struct dentry * dentry = file->f_dentry;
+	struct inode * inode = dentry->d_inode;
+	loff_t result;
+
+	/*
+	 * Make sure inode fields are up-to-date, since generic_file_llseek()
+	 * might look at anything in the inode.  Currently, i_size may be
+	 * used.
+	 */
+	result = nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	if (!result)
+		result = generic_file_llseek(file, offset, origin);
+	return result;
 }
 
 /*

NFS client llseek

[Trond Myklebust]

>>>>> " " == dzafman  <dzafman@kahuna.cag.cpqcorp.net> writes:

     > I noticed that generic_file_llseek looks at i_size without
     > performing a revalidate, so I propose the following patch for
     > NFS client.  It applies to both 2.4.16 and 2.5.0 kernels.

Good point.

Thanks,
   Trond

NFS client llseek

[Trond Myklebust]

>>>>> " " == dzafman  <dzafman@kahuna.cag.cpqcorp.net> writes:

     > linux-2.4.16/fs/nfs/file.c
     > --- linux-2.4.16.orig/fs/nfs/file.c Sun Sep 23 09:48:01 2001
     > +++ linux-2.4.16/fs/nfs/file.c Thu Dec 13 15:35:05 2001
     > @@ -39,9 +39,10 @@
     >  static ssize_t nfs_file_write(struct file *, const char *,
     >  size_t, loff_t *); static int nfs_file_flush(struct file *);
     >  static int nfs_fsync(struct file *, struct dentry *dentry, int
     >  datasync);
     > +static loff_t nfs_file_llseek(struct file *, loff_t, int
     > origin);
 
     >  struct file_operations nfs_file_operations = {
     > - llseek: generic_file_llseek,
     > +	llseek:		nfs_file_llseek,
     >  	read: nfs_file_read, write: nfs_file_write, mmap:
     >  	nfs_file_mmap,
     > @@ -142,6 +143,24 @@
     >  	} unlock_kernel(); return status;
     > +} + +static loff_t +nfs_file_llseek(struct file *file, loff_t
     > offset, int origin) +{
     > + struct dentry * dentry = file->f_dentry;
     > + struct inode * inode = dentry->d_inode;
     > + loff_t result;
     > +
     > + /*
     > + * Make sure inode fields are up-to-date, since
     >  	   generic_file_llseek()
     > + * might look at anything in the inode.  Currently, i_size may
     >  	   be
     > + * used.
     > + */
     > + result = nfs_revalidate_inode(NFS_SERVER(inode), inode);
     > + if (!result)
     > + result = generic_file_llseek(file, offset, origin);
     > + return result;
     >  }
 
Just one comment: Isn't it easier to do this in generic_file_llseek()
itself using inode->i_op->revalidate()? That would make it work for
coda and smbfs too...

Cheers,
   Trond

Re: NFS client llseek

[Jan Harkes]

On Fri, Dec 14, 2001 at 01:51:36PM +0100, Trond Myklebust wrote:
> Just one comment: Isn't it easier to do this in generic_file_llseek()
> itself using inode->i_op->revalidate()? That would make it work for
> coda and smbfs too...

Actually, as far as Coda is concerned this only adds overhead. Coda uses
AFS2 session semantics instead of UNIX semantics, so updates are only
propagated when a file is closed.

Adding this to the generic_file_llseek will force an useless but
expensive upcall (and RPC call to the server) to every seek to check for
an updated i_size while we already know that the i_size of the file
won't have to change until it is closed and reopened.

I guess we're just (mis-)using the revalidate call as a replacement of a
missing call to i_ops->getattr from sys_stat. So perhaps adding the
revalidate to the generic_llseek is fine, but I'll just have to get that
missing getattr call into the tree.

Jan

Re: NFS client llseek

[Alexander Viro]

On Mon, 17 Dec 2001, Jan Harkes wrote:

> On Fri, Dec 14, 2001 at 01:51:36PM +0100, Trond Myklebust wrote:
> > Just one comment: Isn't it easier to do this in generic_file_llseek()
> > itself using inode->i_op->revalidate()? That would make it work for
> > coda and smbfs too...
> 
> Actually, as far as Coda is concerned this only adds overhead. Coda uses
> AFS2 session semantics instead of UNIX semantics, so updates are only
> propagated when a file is closed.
> 
> Adding this to the generic_file_llseek will force an useless but
> expensive upcall (and RPC call to the server) to every seek to check for
> an updated i_size while we already know that the i_size of the file
> won't have to change until it is closed and reopened.
> 
> I guess we're just (mis-)using the revalidate call as a replacement of a
> missing call to i_ops->getattr from sys_stat. So perhaps adding the
> revalidate to the generic_llseek is fine, but I'll just have to get that
> missing getattr call into the tree.

As far as I'm concerned it's not fine.

->getattr() is needed and will be added (patch exists), but the thing
about ->revalidate()...  It's a bloody mess that will need serious
cleanups.  And I'd rather have fewer code paths involved into that
cleanup.

	So please, do it in nfs_lseek() explicitly.  If you want to use
generic_file_lseek() - wonderful, call it from nfs_lseek() after you've
done revalidation.

Re: NFS client llseek

[Trond Myklebust]

>>>>> Alexander Viro <viro@math.psu.edu> writes:

     > getattr() is needed and will be added (patch exists), but the
     > thing about ->revalidate()...  It's a bloody mess that will
     > need serious cleanups.  And I'd rather have fewer code paths
     > involved into that cleanup.

AFAIK, revalidate() was originally simply meant to check the validity
of the cached attributes, refreshing them if the cache is stale.

If it is being used as a replacement for getattr() in some cases but
not others, then I agree it needs to go.

That said, I would still like the ability to inform the filesystem
that it needs to refresh the attribute cache. This is required in
order to close the remaining hole in the close-to-open cache semantics
when doing opendir(".").
For the moment, I'm hacking in a call 'check_stale(inode)' that is
used by the 'cto' patch to notify NFS in the above case, when the VFS
tries to open() a file while bypassing the dentry revalidation call.

Cheers,
  Trond

re: NFS client llseek

[Pedro M. Rodrigues]

   Like http://www.opengfs.org ?


/Pedro

On 14 Dec 2001 at 12:45, dzafman@kahuna.cag.cpqcorp.net wrote:

> 
> By the way, we are looking at the challenges of integrating a fully
> coherent distributed/cluster filesystem into the Linux filesystem
> architecture.
> 

re: NFS client llseek

[dzafman]

Trond Myklebust wrote:

>    Just one comment: Isn't it easier to do this in generic_file_llseek()
>    itself using inode->i_op->revalidate()? That would make it work for
>    coda and smbfs too...
>
>    Cheers,
>       Trond

Yes, you are right.  I've attached a patch which does the revalidate in
both default_llseek() and generic_file_llseek().  Also, it only happens
if i_size is going to be used.  This makes NFS client, smbfs, opengfs, and coda
work right, among others.  I copied do_revalidate() from fs/stat.c.  It would be
nice if it was in a header file instead.

By the way, we are looking at the challenges of integrating a fully coherent
distributed/cluster filesystem into the Linux filesystem architecture.

--- linux-2.4.16.orig/fs/read_write.c	Fri Dec 14 12:06:44 2001
+++ linux-2.4.16/fs/read_write.c	Fri Dec 14 12:54:02 2001
@@ -20,6 +20,19 @@
 	mmap:		generic_file_mmap,
 };
 
+/*
+ * Revalidate the inode. This is required for proper NFS attribute caching.
+ * ARG! Copied from fs/stat.c   (move to a header file)
+ */
+static __inline__ int
+do_revalidate(struct dentry *dentry)
+{
+	struct inode * inode = dentry->d_inode;
+	if (inode->i_op && inode->i_op->revalidate)
+		return inode->i_op->revalidate(dentry);
+	return 0;
+}
+
 ssize_t generic_read_dir(struct file *filp, char *buf, size_t siz, loff_t *ppos)
 {
 	return -EISDIR;
@@ -31,6 +44,8 @@
 
 	switch (origin) {
 		case 2:
+			if ((retval = do_revalidate(file->f_dentry)) < 0)
+				return retval;
 			offset += file->f_dentry->d_inode->i_size;
 			break;
 		case 1:
@@ -59,6 +74,8 @@
 
 	switch (origin) {
 		case 2:
+			if ((retval = do_revalidate(file->f_dentry)) < 0)
+				return retval;
 			offset += file->f_dentry->d_inode->i_size;
 			break;
 		case 1: