Messages in this thread Patch in this message | | | Date | Wed, 07 Jan 1998 22:23:47 -0500 | From | Bill Hawes <> | Subject | patch for 2.1.78 nfs client |
| |
The attached patch takes care of a few loose ends from the VFS inode->dentry changes, and fixes a oops when using the NFS debugging facility.
The patch makes a few additional changes to the VFS to notify the filesystem when the inode is being released from a dentry. NFS needs to be sure that the dentry remain valid for any pending writes, so when the inode is being released it needs to flush any writes depending on the dentry.
I've implemented this by always using a routine dentry_iput() in dcache.c to free the inode, and this routine in turn checks for a d_iput dentry_op. If defined, d_iput() is called with the dentry and inode as arguments. (The dentry d_inode field is cleared in advance to forestall any races.)
The nfs_dentry_iput() routine checks first for any pending writes for the inode, and if necessary waits until all writes for the particular dentry have been completed.
While testing using NFS debugging I found an oops caused by dereferencing a NULL page->inode pointer when flushing dirty pages. This was easily fixed in nfs/write.c by changing the debugging print to use the dentry name instead of inode dev/ino.
However, it raises a question about how the page with the pending write has been freed from the inode's page cache. There's no problem with the page disappearing until the write completes (the wb_req holds a page count), but I'm concerned that a subsequent write to the same file could add another page back into the inode cache. This could create an incoherency by having two dirty pages with inconsistent information.
Regards, Bill--- linux-2.1.78/include/linux/nfs_fs.h.old Tue Jan 6 12:01:32 1998 +++ linux-2.1.78/include/linux/nfs_fs.h Tue Jan 6 12:27:23 1998 @@ -171,6 +171,7 @@ * linux/fs/nfs/write.c */ extern int nfs_writepage(struct dentry *, struct page *); +extern int nfs_find_dentry_request(struct inode *, struct dentry *); extern int nfs_check_failed_request(struct inode *); extern int nfs_check_error(struct inode *); extern int nfs_flush_dirty_pages(struct inode *, pid_t, off_t, off_t); --- linux-2.1.78/fs/nfs/dir.c.old Tue Jan 6 11:39:07 1998 +++ linux-2.1.78/fs/nfs/dir.c Wed Jan 7 18:09:22 1998 @@ -35,6 +35,7 @@ extern void nfs_renew_times(struct dentry *); #define NFS_PARANOIA 1 +/* #define NFS_DEBUG_VERBOSE 1 */ /* * Head for a dircache entry. Currently still very simple; when @@ -458,6 +459,28 @@ } /* + * Called to free the inode from the dentry. We must flush + * any pending writes for this dentry before freeing the inode. + */ +static void nfs_dentry_iput(struct dentry *dentry, struct inode *inode) +{ + if (NFS_WRITEBACK(inode)) { +#ifdef NFS_PARANOIA +printk("nfs_dentry_iput: pending writes for %s/%s, i_count=%d\n", +dentry->d_parent->d_name.name, dentry->d_name.name, inode->i_count); +#endif + while (nfs_find_dentry_request(inode, dentry)) { +#ifdef NFS_PARANOIA +printk("nfs_dentry_iput: flushing %s/%s\n", +dentry->d_parent->d_name.name, dentry->d_name.name); +#endif + nfs_flush_dirty_pages(inode, 0, 0, 0); + } + } + iput(inode); +} + +/* * Called when the dentry is being freed to release private memory. */ static void nfs_dentry_release(struct dentry *dentry) @@ -471,9 +494,53 @@ NULL, /* d_hash */ NULL, /* d_compare */ nfs_dentry_delete, /* d_delete(struct dentry *) */ + nfs_dentry_iput, /* d_iput(struct dentry *, struct inode *) */ nfs_dentry_release /* d_release(struct dentry *) */ }; +#ifdef NFS_PARANOIA +/* + * Display all dentries holding the specified inode. + */ +static void show_dentry(struct inode * inode) +{ + struct dentry *parent = inode->i_sb->s_root; + struct dentry *this_parent = parent; + struct list_head *next; + +repeat: + next = this_parent->d_subdirs.next; +resume: + while (next != &this_parent->d_subdirs) { + struct list_head *tmp = next; + struct dentry *dentry = list_entry(tmp, struct dentry, d_child); + next = tmp->next; + if (dentry->d_inode == inode) { + int unhashed = list_empty(&dentry->d_hash); + printk("show_dentry: %s/%s, d_count=%d%s\n", + dentry->d_parent->d_name.name, + dentry->d_name.name, dentry->d_count, + unhashed ? "(unhashed)" : ""); + } + /* + * Descend a level if the d_subdirs list is non-empty. + */ + if (!list_empty(&dentry->d_subdirs)) { + this_parent = dentry; + goto repeat; + } + } + /* + * All done at this level ... ascend and resume the search. + */ + if (this_parent != parent) { + next = this_parent->d_child.next; + this_parent = this_parent->d_parent; + goto resume; + } +} +#endif + /* * Whenever a lookup succeeds, we know the parent directories * are all valid, so we want to update the dentry timestamps. @@ -531,10 +598,12 @@ inode = nfs_fhget(dentry->d_sb, &fhandle, &fattr); if (inode) { #ifdef NFS_PARANOIA -if (inode->i_count > (S_ISDIR(inode->i_mode) ? 1 : inode->i_nlink)) +if (inode->i_count > (S_ISDIR(inode->i_mode) ? 1 : inode->i_nlink)) { printk("nfs_lookup: %s/%s ino=%ld in use, count=%d, nlink=%d\n", dentry->d_parent->d_name.name, dentry->d_name.name, inode->i_ino, inode->i_count, inode->i_nlink); +show_dentry(inode); +} #endif no_entry: d_add(dentry, inode); @@ -564,10 +633,12 @@ inode = nfs_fhget(dentry->d_sb, fhandle, fattr); if (inode) { #ifdef NFS_PARANOIA -if (inode->i_count > (S_ISDIR(inode->i_mode) ? 1 : inode->i_nlink)) +if (inode->i_count > (S_ISDIR(inode->i_mode) ? 1 : inode->i_nlink)) { printk("nfs_instantiate: %s/%s ino=%ld in use, count=%d, nlink=%d\n", dentry->d_parent->d_name.name, dentry->d_name.name, inode->i_ino, inode->i_count, inode->i_nlink); +show_dentry(inode); +} #endif d_instantiate(dentry, inode); nfs_renew_times(dentry); @@ -682,10 +753,6 @@ sattr.uid = sattr.gid = sattr.size = (unsigned) -1; sattr.atime.seconds = sattr.mtime.seconds = (unsigned) -1; - nfs_invalidate_dircache(dir); - error = nfs_proc_mkdir(NFS_SERVER(dir), NFS_FH(dentry->d_parent), - dentry->d_name.name, &sattr, &fhandle, &fattr); - /* * Always drop the dentry, we can't always depend on * the fattr returned by the server (AIX seems to be @@ -693,6 +760,9 @@ * depending on potentially bogus information. */ d_drop(dentry); + nfs_invalidate_dircache(dir); + error = nfs_proc_mkdir(NFS_DSERVER(dentry), NFS_FH(dentry->d_parent), + dentry->d_name.name, &sattr, &fhandle, &fattr); out: return error; } --- linux-2.1.78/fs/nfs/write.c.old Tue Jan 6 11:39:08 1998 +++ linux-2.1.78/fs/nfs/write.c Wed Jan 7 22:57:42 1998 @@ -66,6 +66,7 @@ */ #define IS_SOFT 0 +#define NFS_PARANOIA 1 #define NFSDBG_FACILITY NFSDBG_PAGECACHE static void nfs_wback_lock(struct rpc_task *task); @@ -282,6 +283,27 @@ } /* + * Find any requests for the specified dentry. + */ +int +nfs_find_dentry_request(struct inode *inode, struct dentry *dentry) +{ + struct nfs_wreq *head, *req; + int found = 0; + + req = head = NFS_WRITEBACK(inode); + while (req != NULL) { + if (req->wb_dentry == dentry) { + found = 1; + break; + } + if ((req = WB_NEXT(req)) == head) + break; + } + return found; +} + +/* * Find a failed write request by pid */ static struct nfs_wreq * @@ -612,14 +634,18 @@ /* * Flush out a dirty page. */ -static inline void +static void nfs_flush_request(struct nfs_wreq *req) { struct page *page = req->wb_page; - dprintk("NFS: nfs_flush_request(%x/%ld, @%ld)\n", - page->inode->i_dev, page->inode->i_ino, - page->offset); +#ifdef NFS_DEBUG_VERBOSE +if (req->wb_inode != page->inode) +printk("NFS: inode %ld no longer has page %p\n", req->wb_inode->i_ino, page); +#endif + dprintk("NFS: nfs_flush_request(%s/%s, @%ld)\n", + req->wb_dentry->d_parent->d_name.name, + req->wb_dentry->d_name.name, page->offset); req->wb_flags |= NFS_WRITE_WANTLOCK; if (!test_and_set_bit(PG_locked, &page->flags)) { @@ -656,7 +682,7 @@ if (rqoffset < end && offset < rqend && (pid == 0 || req->wb_pid == pid)) { if (!WB_HAVELOCK(req)) { -#ifdef NFS_PARANOIA +#ifdef NFS_DEBUG_VERBOSE printk("nfs_flush: flushing inode=%ld, %d @ %lu\n", req->wb_inode->i_ino, req->wb_bytes, rqoffset); #endif @@ -664,11 +690,6 @@ } last = req; } - } else { -#ifdef NFS_PARANOIA -printk("nfs_flush_pages: in progress inode=%ld, %d @ %lu\n", -req->wb_inode->i_ino, req->wb_bytes, rqoffset); -#endif } if (invalidate) req->wb_flags |= NFS_WRITE_INVALIDATE; --- linux-2.1.78/include/linux/dcache.h.old Tue Jan 6 11:39:09 1998 +++ linux-2.1.78/include/linux/dcache.h Tue Jan 6 11:58:23 1998 @@ -71,9 +71,10 @@ struct dentry_operations { int (*d_revalidate)(struct dentry *); - int (*d_hash) (struct dentry *,struct qstr *); - int (*d_compare) (struct dentry *,struct qstr *, struct qstr *); + int (*d_hash) (struct dentry *, struct qstr *); + int (*d_compare) (struct dentry *, struct qstr *, struct qstr *); void (*d_delete)(struct dentry *); + void (*d_iput)(struct dentry *, struct inode *); void (*d_release)(struct dentry *); }; --- linux-2.1.78/fs/dcache.c.old Tue Jan 6 11:39:05 1998 +++ linux-2.1.78/fs/dcache.c Wed Jan 7 14:06:56 1998 @@ -59,6 +60,22 @@ } /* + * Release the dentry's inode, using the fileystem + * d_iput() operation if defined. + */ +static inline void dentry_iput(struct dentry * dentry) +{ + struct inode *inode = dentry->d_inode; + if (inode) { + dentry->d_inode = NULL; + if (dentry->d_op && dentry->d_op->d_iput) + dentry->d_op->d_iput(dentry, inode); + else + iput(inode); + } +} + +/* * dput() * * This is complicated by the fact that we do not want to put @@ -104,13 +121,10 @@ list_del(&dentry->d_lru); } if (list_empty(&dentry->d_hash)) { - struct inode *inode = dentry->d_inode; struct dentry * parent; + list_del(&dentry->d_child); - if (inode) { - dentry->d_inode = NULL; - iput(inode); - } + dentry_iput(dentry); parent = dentry->d_parent; d_free(dentry); if (dentry == parent) @@ -259,12 +273,7 @@ list_del(&dentry->d_hash); list_del(&dentry->d_child); - if (dentry->d_inode) { - struct inode * inode = dentry->d_inode; - - dentry->d_inode = NULL; - iput(inode); - } + dentry_iput(dentry); parent = dentry->d_parent; d_free(dentry); dput(parent); @@ -636,11 +713,7 @@ * Are we the only user? */ if (dentry->d_count == 1) { - struct inode * inode = dentry->d_inode; - if (inode) { - dentry->d_inode = NULL; - iput(inode); - } + dentry_iput(dentry); return; } | |