I've started doing a little work on NFS and have a preliminary patch for
testing. It has a number of changes, so I'll mention the main points:
(1) Some rearrangements in read_super to help avoid races at mount time.
(2) A check for negative dentries in silly_delete to avoid an oops.
(3) Changes in nfs_refresh_inode to detect and handle the "inode changes
mode" problem. This detects a change from file->dir or dir->file and
uses the bad_inode_ops to return error on any subsequent use. I've also
modified the dentry validation callback to recognize invalid inodes so
that the dentry will be invalidated.
(4) nfs_put_inode now gets rid of inodes on the last use, as there's no
point in keeping the inode once the dentry has released it.
I'd appreciate having some NFS users check this out and report back,
especially regarding the "mode change" behavior. It's easy to test:
just cat a file, then on the server delete it and make a directory of
the same name, then try to cat it again. Then ls a dir, on the server
change it to a file, then try to ls it again.
Regards,
Bill
--------------8B592A2A08D58C532AEAEA4E
Content-Type: text/plain; charset=us-ascii; name="nfs_57-patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="nfs_57-patch"
--- linux-2.1.57/fs/nfs/inode.c.old Sat Sep 6 16:03:33 1997
+++ linux-2.1.57/fs/nfs/inode.c Fri Sep 26 08:29:42 1997
@@ -67,6 +67,7 @@
{
inode->i_blksize = inode->i_sb->s_blocksize;
inode->i_mode = 0;
+ inode->i_rdev = 0;
inode->i_op = NULL;
NFS_CACHEINV(inode);
}
@@ -75,6 +76,11 @@
nfs_put_inode(struct inode * inode)
{
dprintk("NFS: put_inode(%x/%ld)\n", inode->i_dev, inode->i_ino);
+ /*
+ * We want to get rid of unused inodes ...
+ */
+ if (inode->i_count == 1)
+ inode->i_nlink = 0;
}
static void
@@ -90,13 +96,16 @@
struct nfs_server *server = &sb->u.nfs_sb.s_server;
struct rpc_clnt *rpc;
+ /*
+ * Lock the super block while we bring down the daemons.
+ */
+ lock_super(sb);
if ((rpc = server->client) != NULL)
rpc_shutdown_client(rpc);
if (!(server->flags & NFS_MOUNT_NONLM))
lockd_down(); /* release rpc.lockd */
rpciod_down(); /* release rpciod */
- lock_super(sb);
sb->s_dev = 0;
unlock_super(sb);
MOD_DEC_USE_COUNT;
@@ -147,14 +156,12 @@
unsigned int authflavor;
int tcp;
kdev_t dev = sb->s_dev;
+ struct inode *root_inode;
MOD_INC_USE_COUNT;
- if (!data) {
- printk("nfs_read_super: missing data argument\n");
- sb->s_dev = 0;
- MOD_DEC_USE_COUNT;
- return NULL;
- }
+ if (!data)
+ goto out_miss_args;
+
if (data->version != NFS_MOUNT_VERSION) {
printk("nfs warning: mount version %s than kernel\n",
data->version < NFS_MOUNT_VERSION ? "older" : "newer");
@@ -164,13 +171,19 @@
data->bsize = 0;
}
+ /* We now require that the mount process passes the remote address */
+ memcpy(&srvaddr, &data->addr, sizeof(srvaddr));
+ if (srvaddr.sin_addr.s_addr == INADDR_ANY)
+ goto out_no_remote;
+
lock_super(sb);
- server = &sb->u.nfs_sb.s_server;
sb->s_magic = NFS_SUPER_MAGIC;
sb->s_dev = dev;
sb->s_op = &nfs_sops;
sb->s_blocksize = nfs_block_size(data->bsize, &sb->s_blocksize_bits);
+ sb->u.nfs_sb.s_root = data->root;
+ server = &sb->u.nfs_sb.s_server;
server->rsize = nfs_block_size(data->rsize, NULL);
server->wsize = nfs_block_size(data->wsize, NULL);
server->flags = data->flags;
@@ -179,15 +192,6 @@
server->acdirmin = data->acdirmin*HZ;
server->acdirmax = data->acdirmax*HZ;
strcpy(server->hostname, data->hostname);
- sb->u.nfs_sb.s_root = data->root;
-
- /* We now require that the mount process passes the remote address */
- memcpy(&srvaddr, &data->addr, sizeof(srvaddr));
- if (srvaddr.sin_addr.s_addr == INADDR_ANY) {
- printk("NFS: mount program didn't pass remote address!\n");
- MOD_DEC_USE_COUNT;
- return NULL;
- }
/* Which protocol do we use? */
tcp = (data->flags & NFS_MOUNT_TCP);
@@ -210,18 +214,13 @@
/* Now create transport and client */
xprt = xprt_create_proto(tcp? IPPROTO_TCP : IPPROTO_UDP,
&srvaddr, &timeparms);
- if (xprt == NULL) {
- printk("NFS: cannot create RPC transport.\n");
- goto failure;
- }
+ if (xprt == NULL)
+ goto out_no_xprt;
clnt = rpc_create_client(xprt, server->hostname, &nfs_program,
NFS_VERSION, authflavor);
- if (clnt == NULL) {
- printk("NFS: cannot create RPC client.\n");
- xprt_destroy(xprt);
- goto failure;
- }
+ if (clnt == NULL)
+ goto out_no_client;
clnt->cl_intr = (data->flags & NFS_MOUNT_INTR)? 1 : 0;
clnt->cl_softrtry = (data->flags & NFS_MOUNT_SOFT)? 1 : 0;
@@ -229,29 +228,51 @@
server->client = clnt;
/* Fire up rpciod if not yet running */
+ /* N.B. can this fail?? */
rpciod_up();
- /* Unlock super block and try to get root fh attributes */
+ /*
+ * Keep the super block locked while we try to get
+ * the root fh attributes.
+ */
+ root_inode = nfs_fhget(sb, &data->root, NULL);
+ if (!root_inode)
+ goto out_no_root;
+ sb->s_root = d_alloc_root(root_inode, NULL);
+ if (!sb->s_root)
+ goto out_no_root;
unlock_super(sb);
- sb->s_root = d_alloc_root(nfs_fhget(sb, &data->root, NULL), NULL);
- if (sb->s_root != NULL) {
- /* We're airborne */
- if (!(server->flags & NFS_MOUNT_NONLM))
- lockd_up();
- return sb;
- }
+ /* We're airborne */
+ if (!(server->flags & NFS_MOUNT_NONLM))
+ lockd_up();
+ return sb;
/* Yargs. It didn't work out. */
+out_no_root:
printk("nfs_read_super: get root inode failed\n");
- rpc_shutdown_client(server->client);
+ iput(root_inode);
rpciod_down();
+ rpc_shutdown_client(server->client);
+ goto out_unlock;
+out_no_client:
+ printk("NFS: cannot create RPC client.\n");
+ xprt_destroy(xprt);
+ goto out_unlock;
+out_no_xprt:
+ printk("NFS: cannot create RPC transport.\n");
+out_unlock:
+ unlock_super(sb);
+ goto out_fail;
-failure:
- MOD_DEC_USE_COUNT;
- if (sb->s_lock)
- unlock_super(sb);
+out_no_remote:
+ printk("NFS: mount program didn't pass remote address!\n");
+ goto out_fail;
+out_miss_args:
+ printk("nfs_read_super: missing data argument\n");
+out_fail:
sb->s_dev = 0;
+ MOD_DEC_USE_COUNT;
return NULL;
}
--- linux-2.1.57/fs/nfs/dir.c.old Fri Sep 26 08:10:51 1997
+++ linux-2.1.57/fs/nfs/dir.c Sat Sep 27 16:18:45 1997
@@ -94,6 +94,10 @@
NULL, /* updatepage */
nfs_revalidate, /* revalidate */
};
+/*
+ * This is used if an inode changes mode.
+ */
+extern struct inode_operations bad_inode_ops;
static int
nfs_dir_open(struct inode *dir, struct file *file)
@@ -350,18 +354,29 @@
unsigned long time = jiffies - dentry->d_time;
unsigned long max = 5*HZ;
- if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode))
- max = 10*HZ;
+ if (dentry->d_inode) {
+ if (dentry->d_inode->i_op == &bad_inode_ops) {
+#if 1
+printk("nfs_lookup_validate: %s/%s has dud inode\n",
+dentry->d_parent->d_name.name, dentry->d_name.name);
+#endif
+ goto bad;
+ }
+ if (S_ISDIR(dentry->d_inode->i_mode))
+ max = 10*HZ;
+ }
return time < max;
+bad:
+ return 0;
}
static void nfs_silly_delete(struct dentry *);
static struct dentry_operations nfs_dentry_operations = {
- nfs_lookup_revalidate,
+ nfs_lookup_revalidate, /* d_validate(struct dentry *) */
0, /* d_hash */
0, /* d_compare */
- nfs_silly_delete,
+ nfs_silly_delete, /* d_delete(struct dentry *) */
};
static int nfs_lookup(struct inode *dir, struct dentry * dentry)
@@ -383,20 +398,41 @@
if (len > NFS_MAXNAMLEN)
return -ENAMETOOLONG;
- error = nfs_proc_lookup(NFS_SERVER(dir), NFS_FH(dir), dentry->d_name.name, &fhandle, &fattr);
-
+ error = nfs_proc_lookup(NFS_SERVER(dir), NFS_FH(dir),
+ dentry->d_name.name, &fhandle, &fattr);
inode = NULL;
+ if (error == -ENOENT)
+ goto no_entry;
if (!error) {
+ error = -EACCES;
inode = nfs_fhget(dir->i_sb, &fhandle, &fattr);
- if (!inode)
- return -EACCES;
- } else if (error != -ENOENT)
- return error;
+ if (inode) {
+ no_entry:
+ dentry->d_time = jiffies;
+ dentry->d_op = &nfs_dentry_operations;
+ d_add(dentry, inode);
+ error = 0;
+ }
+ }
+ return error;
+}
- dentry->d_time = jiffies;
- dentry->d_op = &nfs_dentry_operations;
- d_add(dentry, inode);
- return 0;
+/*
+ * Code common to create, mkdir, and mknod.
+ */
+static int nfs_instantiate(struct inode *dir, struct dentry *dentry,
+ struct nfs_fattr *fattr, struct nfs_fh *fhandle)
+{
+ struct inode *inode;
+ int error = -EACCES;
+
+ inode = nfs_fhget(dir->i_sb, fhandle, fattr);
+ if (inode) {
+ nfs_invalidate_dircache(dir);
+ d_instantiate(dentry, inode);
+ error = 0;
+ }
+ return error;
}
static int nfs_create(struct inode *dir, struct dentry * dentry, int mode)
@@ -404,7 +440,6 @@
struct nfs_sattr sattr;
struct nfs_fattr fattr;
struct nfs_fh fhandle;
- struct inode *inode;
int error;
dfprintk(VFS, "NFS: create(%x/%ld, %s\n",
@@ -422,18 +457,10 @@
sattr.uid = sattr.gid = sattr.size = (unsigned) -1;
sattr.atime.seconds = sattr.mtime.seconds = (unsigned) -1;
error = nfs_proc_create(NFS_SERVER(dir), NFS_FH(dir),
- dentry->d_name.name, &sattr, &fhandle, &fattr);
-
- if (error)
- return error;
-
- inode = nfs_fhget(dir->i_sb, &fhandle, &fattr);
- if (!inode)
- return -EACCES;
-
- nfs_invalidate_dircache(dir);
- d_instantiate(dentry, inode);
- return 0;
+ dentry->d_name.name, &sattr, &fhandle, &fattr);
+ if (!error)
+ error = nfs_instantiate(dir, dentry, &fattr, &fhandle);
+ return error;
}
static int nfs_mknod(struct inode *dir, struct dentry *dentry, int mode, int rdev)
@@ -441,7 +468,6 @@
struct nfs_sattr sattr;
struct nfs_fattr fattr;
struct nfs_fh fhandle;
- struct inode *inode;
int error;
dfprintk(VFS, "NFS: mknod(%x/%ld, %s\n",
@@ -462,18 +488,10 @@
sattr.atime.seconds = sattr.mtime.seconds = (unsigned) -1;
error = nfs_proc_create(NFS_SERVER(dir), NFS_FH(dir),
- dentry->d_name.name, &sattr, &fhandle, &fattr);
-
- if (error)
- return error;
-
- inode = nfs_fhget(dir->i_sb, &fhandle, &fattr);
- if (!inode)
- return -EACCES;
-
- nfs_invalidate_dircache(dir);
- d_instantiate(dentry, inode);
- return 0;
+ dentry->d_name.name, &sattr, &fhandle, &fattr);
+ if (!error)
+ error = nfs_instantiate(dir, dentry, &fattr, &fhandle);
+ return error;
}
static int nfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
@@ -481,7 +499,6 @@
struct nfs_sattr sattr;
struct nfs_fattr fattr;
struct nfs_fh fhandle;
- struct inode * inode;
int error;
dfprintk(VFS, "NFS: mkdir(%x/%ld, %s\n",
@@ -500,20 +517,16 @@
sattr.atime.seconds = sattr.mtime.seconds = (unsigned) -1;
error = nfs_proc_mkdir(NFS_SERVER(dir), NFS_FH(dir),
- dentry->d_name.name, &sattr, &fhandle, &fattr);
-
- if (error)
- return error;
-
- inode = nfs_fhget(dir->i_sb, &fhandle, &fattr);
- if (!inode)
- return -EACCES;
-
- nfs_invalidate_dircache(dir);
- d_instantiate(dentry, inode);
- return 0;
+ dentry->d_name.name, &sattr, &fhandle, &fattr);
+ if (!error)
+ error = nfs_instantiate(dir, dentry, &fattr, &fhandle);
+ return error;
}
+/*
+ * Update inode->i_nlink immediately after a successful operation.
+ * (See comments for nfs_unlink.)
+ */
static int nfs_rmdir(struct inode *dir, struct dentry *dentry)
{
int error;
@@ -530,12 +543,13 @@
return -ENAMETOOLONG;
error = nfs_proc_rmdir(NFS_SERVER(dir), NFS_FH(dir), dentry->d_name.name);
- if (error)
- return error;
-
- nfs_invalidate_dircache(dir);
- d_delete(dentry);
- return 0;
+ if (!error) {
+ if (dentry->d_inode->i_nlink)
+ dentry->d_inode->i_nlink --;
+ nfs_invalidate_dircache(dir);
+ d_delete(dentry);
+ }
+ return error;
}
@@ -642,16 +656,14 @@
error = nfs_proc_rename(NFS_SERVER(dir),
NFS_FH(dir), dentry->d_name.name,
NFS_FH(dir), silly);
- if (error) {
- dput(sdentry);
- return error;
+ if (!error) {
+ nfs_invalidate_dircache(dir);
+ d_move(dentry, sdentry);
+ dentry->d_flags |= DCACHE_NFSFS_RENAMED;
+ /* If we return 0 we don't unlink */
}
- nfs_invalidate_dircache(dir);
- d_move(dentry, sdentry);
dput(sdentry);
- dentry->d_flags |= DCACHE_NFSFS_RENAMED;
-
- return 0; /* don't unlink */
+ return error;
}
static void nfs_silly_delete(struct dentry *dentry)
@@ -670,8 +682,18 @@
if (error < 0)
printk("NFS " __FUNCTION__ " failed (err = %d)\n",
-error);
- dentry->d_inode->i_nlink --;
+ if (dentry->d_inode) {
+ if (dentry->d_inode->i_nlink)
+ dentry->d_inode->i_nlink --;
+ } else
+ printk("nfs_silly_delete: negative dentry %s/%s\n",
+ dentry->d_parent->d_name.name,
+ dentry->d_name.name);
nfs_invalidate_dircache(dir);
+ /*
+ * The dentry is unhashed, but we want to make it negative.
+ */
+ d_delete(dentry);
}
}
@@ -680,8 +702,11 @@
*
* If sillyrename() returns 0, we do nothing, otherwise we unlink.
*
- * inode->i_nlink is updated here rather than waiting for the next
- * nfs_refresh_inode() for cosmetic reasons only.
+ * Updating inode->i_nlink here rather than waiting for the next
+ * nfs_refresh_inode() is not merely cosmetic; once an object has
+ * been deleted, we want to get rid of the inode locally. The NFS
+ * server may reuse the fileid for a new inode, and we don't want
+ * that to be confused with this inode.
*/
static int nfs_unlink(struct inode *dir, struct dentry *dentry)
{
@@ -700,20 +725,18 @@
error = nfs_sillyrename(dir, dentry);
- if (error == -EBUSY) {
- return -EBUSY;
- } else if (error < 0) {
+ if (error && error != -EBUSY) {
error = nfs_proc_remove(NFS_SERVER(dir),
NFS_FH(dir), dentry->d_name.name);
- if (error < 0)
- return error;
-
- dentry->d_inode->i_nlink --;
- nfs_invalidate_dircache(dir);
- d_delete(dentry);
+ if (!error) {
+ if (dentry->d_inode->i_nlink)
+ dentry->d_inode->i_nlink --;
+ nfs_invalidate_dircache(dir);
+ d_delete(dentry);
+ }
}
- return 0;
+ return error;
}
static int nfs_symlink(struct inode *dir, struct dentry *dentry, const char *symname)
@@ -774,17 +797,16 @@
if (dentry->d_name.len > NFS_MAXNAMLEN)
return -ENAMETOOLONG;
- error = nfs_proc_link(NFS_SERVER(inode), NFS_FH(inode),
- NFS_FH(dir), dentry->d_name.name);
-
- if (error)
- return error;
-
- nfs_invalidate_dircache(dir);
- inode->i_count ++;
- inode->i_nlink ++; /* no need to wait for nfs_refresh_inode() */
- d_instantiate(dentry, inode);
- return 0;
+ error = nfs_proc_link(NFS_SERVER(inode), NFS_FH(inode), NFS_FH(dir),
+ dentry->d_name.name);
+ if (!error) {
+ nfs_invalidate_dircache(dir);
+ inode->i_count ++;
+ inode->i_nlink ++; /* no need to wait for nfs_refresh_inode() */
+ d_instantiate(dentry, inode);
+ error = 0;
+ }
+ return error;
}
/*
@@ -860,23 +882,65 @@
* of the server's inode.
*/
-void nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
+int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
{
- int was_empty;
+ int error = -EIO;
dfprintk(VFS, "NFS: refresh_inode(%x/%ld ct=%d)\n",
inode->i_dev, inode->i_ino, inode->i_count);
if (!inode || !fattr) {
printk("nfs_refresh_inode: inode or fattr is NULL\n");
- return;
+ goto out;
}
if (inode->i_ino != fattr->fileid) {
printk("nfs_refresh_inode: inode number mismatch\n");
- return;
+ goto out;
}
- was_empty = (inode->i_mode == 0);
- inode->i_mode = fattr->mode;
+ /*
+ * Check whether the mode has been set, as we only want to
+ * do this once. (We don't allow inodes to change types.)
+ */
+ if (inode->i_mode == 0) {
+ inode->i_mode = fattr->mode;
+ if (S_ISREG(inode->i_mode))
+ inode->i_op = &nfs_file_inode_operations;
+ else if (S_ISDIR(inode->i_mode))
+ inode->i_op = &nfs_dir_inode_operations;
+ else if (S_ISLNK(inode->i_mode))
+ inode->i_op = &nfs_symlink_inode_operations;
+ else if (S_ISCHR(inode->i_mode)) {
+ inode->i_op = &chrdev_inode_operations;
+ inode->i_rdev = to_kdev_t(fattr->rdev);
+ } else if (S_ISBLK(inode->i_mode)) {
+ inode->i_op = &blkdev_inode_operations;
+ inode->i_rdev = to_kdev_t(fattr->rdev);
+ } else if (S_ISFIFO(inode->i_mode))
+ init_fifo(inode);
+ else
+ inode->i_op = NULL;
+ } else if ((inode->i_mode & S_IFMT) == (fattr->mode & S_IFMT)) {
+ inode->i_mode = fattr->mode;
+ } else {
+ /*
+ * Big trouble! The inode has become a different object.
+ */
+#if 1
+printk("nfs_refresh_inode: mode changed, %07o to %07o\n",
+inode->i_mode, fattr->mode);
+#endif
+ inode->i_nlink = 0;
+ inode->i_op = &bad_inode_ops;
+ /*
+ * No need to worry about unhashing the dentry, as the
+ * lookup validation will know that the inode is bad.
+ * (But we do want to invalidate the caches.)
+ */
+ invalidate_inode_pages(inode);
+ nfs_invalidate_dircache(inode);
+ goto out;
+ }
+
inode->i_nlink = fattr->nlink;
inode->i_uid = fattr->uid;
inode->i_gid = fattr->gid;
@@ -893,29 +957,13 @@
NFS_ATTRTIMEO(inode) = NFS_MINATTRTIMEO(inode);
}
inode->i_size = fattr->size;
- if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
- inode->i_rdev = to_kdev_t(fattr->rdev);
- else
- inode->i_rdev = 0;
inode->i_blocks = fattr->blocks;
inode->i_atime = fattr->atime.seconds;
inode->i_mtime = fattr->mtime.seconds;
inode->i_ctime = fattr->ctime.seconds;
- if (S_ISREG(inode->i_mode))
- inode->i_op = &nfs_file_inode_operations;
- else if (S_ISDIR(inode->i_mode))
- inode->i_op = &nfs_dir_inode_operations;
- else if (S_ISLNK(inode->i_mode))
- inode->i_op = &nfs_symlink_inode_operations;
- else if (S_ISCHR(inode->i_mode))
- inode->i_op = &chrdev_inode_operations;
- else if (S_ISBLK(inode->i_mode))
- inode->i_op = &blkdev_inode_operations;
- else if (S_ISFIFO(inode->i_mode)) {
- if (was_empty)
- init_fifo(inode);
- } else
- inode->i_op = NULL;
+ error = 0;
+out:
+ return error;
}
/*
--- linux-2.1.57/include/linux/nfs_fs.h.old Sat Sep 20 08:16:16 1997
+++ linux-2.1.57/include/linux/nfs_fs.h Sat Sep 27 15:11:08 1997
@@ -138,7 +138,7 @@
extern int init_nfs_fs(void);
extern struct inode *nfs_fhget(struct super_block *sb, struct nfs_fh *fhandle,
struct nfs_fattr *fattr);
-extern void nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr);
+extern int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr);
extern int nfs_revalidate(struct inode *);
extern int _nfs_revalidate_inode(struct nfs_server *, struct inode *);
--------------8B592A2A08D58C532AEAEA4E--