* 2.6.38-rc2... NFS sillyrename is broken... @ 2011-01-25 23:30 Trond Myklebust 2011-01-26 8:20 ` J. R. Okajima 0 siblings, 1 reply; 16+ messages in thread From: Trond Myklebust @ 2011-01-25 23:30 UTC (permalink / raw) To: linux-nfs, Linux Filesystem Development Something in the recent VFS churn appears to have broken NFS sillyrename. Currently, when I try to unlink() a file that is being held open by another process, I do indeed see that file getting renamed to a .nfsxxxxxxx file, but when the file is closed, the .nfsxxxxx file sticks around until I unlink() it again. I'll have a look tomorrow at what is going wrong, but I figured I'd ask on the list in case someone has a suspect... Cheers Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-25 23:30 2.6.38-rc2... NFS sillyrename is broken Trond Myklebust @ 2011-01-26 8:20 ` J. R. Okajima 2011-01-26 20:14 ` Trond Myklebust 0 siblings, 1 reply; 16+ messages in thread From: J. R. Okajima @ 2011-01-26 8:20 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, Nick Piggin, Linux Filesystem Development (CC-ed Nick Piggin) Trond Myklebust: > Something in the recent VFS churn appears to have broken NFS > sillyrename. > > Currently, when I try to unlink() a file that is being held open by > another process, I do indeed see that file getting renamed to > a .nfsxxxxxxx file, but when the file is closed, the .nfsxxxxx file > sticks around until I unlink() it again. > > I'll have a look tomorrow at what is going wrong, but I figured I'd ask > on the list in case someone has a suspect... I noticed this issue yesterday and found the change for removing dcache_lock is suspicious. By the commit 949854d 2011-01-07 fs: Use rename lock and RCU for multi-step operations dentry->d_parent = NULL; is added into the beginning of VFS d_kill(). Later d_kill() calls dentry_iput(), d_op->d_iput() which is nfs_dentry_iput() in NFS. Then nfs_dentry_iput() calls nfs_complete_unlink(), nfs_call_unlink(). Here nfs_call_unlink() calls dget_parent() and when the result is NULL, it skips the actual unlink. Finally the "silly-renamed" dentry remains. Can we stop "dentry->d_parent = NULL" when (d->flags & DCACHE_NFSFS_RENAMED) is true? J. R. Okajima ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-26 8:20 ` J. R. Okajima @ 2011-01-26 20:14 ` Trond Myklebust 2011-01-26 20:43 ` Trond Myklebust 0 siblings, 1 reply; 16+ messages in thread From: Trond Myklebust @ 2011-01-26 20:14 UTC (permalink / raw) To: J. R. Okajima; +Cc: linux-nfs, Nick Piggin, Linux Filesystem Development On Wed, 2011-01-26 at 17:20 +0900, J. R. Okajima wrote: > (CC-ed Nick Piggin) > > Trond Myklebust: > > Something in the recent VFS churn appears to have broken NFS > > sillyrename. > > > > Currently, when I try to unlink() a file that is being held open by > > another process, I do indeed see that file getting renamed to > > a .nfsxxxxxxx file, but when the file is closed, the .nfsxxxxx file > > sticks around until I unlink() it again. > > > > I'll have a look tomorrow at what is going wrong, but I figured I'd ask > > on the list in case someone has a suspect... > > I noticed this issue yesterday and found the change for removing > dcache_lock is suspicious. > By the commit 949854d > 2011-01-07 fs: Use rename lock and RCU for multi-step operations > dentry->d_parent = NULL; > is added into the beginning of VFS d_kill(). > > Later d_kill() calls dentry_iput(), d_op->d_iput() which is > nfs_dentry_iput() in NFS. > Then nfs_dentry_iput() calls nfs_complete_unlink(), nfs_call_unlink(). > Here nfs_call_unlink() calls dget_parent() and when the result is NULL, > it skips the actual unlink. Finally the "silly-renamed" dentry > remains. Agreed. That looks like it explains the breakage. > Can we stop "dentry->d_parent = NULL" > when (d->flags & DCACHE_NFSFS_RENAMED) is true? Nick? The alternative would be to add a callback that can be called after dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent and (negative) dentry as the arguments. sillyrename doesn't need the inode as an argument, but it definitely needs the parent dentry so that it can check for races with ->lookup()... -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-26 20:14 ` Trond Myklebust @ 2011-01-26 20:43 ` Trond Myklebust 0 siblings, 0 replies; 16+ messages in thread From: Trond Myklebust @ 2011-01-26 20:43 UTC (permalink / raw) To: J. R. Okajima; +Cc: linux-nfs, Nick Piggin, Linux Filesystem Development On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: > The alternative would be to add a callback that can be called after > dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent > and (negative) dentry as the arguments. > sillyrename doesn't need the inode as an argument, but it definitely > needs the parent dentry so that it can check for races with > ->lookup()... The following (compile tested only!) patch illustrates what I mean. Cheers Trond 8<-------------------------------------------------------------------------- >From 1f55305d8cc82d2cb4ed37d3782f62bd378d7fd4 Mon Sep 17 00:00:00 2001 From: Trond Myklebust <Trond.Myklebust@netapp.com> Date: Wed, 26 Jan 2011 15:41:56 -0500 Subject: [PATCH] NFS/VFS: Fix the nfs sillyrename regression Commit 949854d0 (fs: Use rename lock and RCU for multi-step operations) broke sillyrename on NFS because it NULLs out the dentry->d_parent prior to calling dentry->d_op->iput() The following patch adds a new callback that is used by d_kill() to notify the filesystem that the sillyrenamed dentry is no longer in use, and that it may be unlinked. It takes the parent dentry and negative dentry as its argument, and is used by the NFS layer to complete the sillyrename. Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> --- Documentation/filesystems/Locking | 2 ++ Documentation/filesystems/vfs.txt | 9 +++++++++ fs/dcache.c | 3 +++ fs/nfs/dir.c | 12 ++++++++---- fs/nfs/unlink.c | 22 ++++++++-------------- include/linux/dcache.h | 1 + include/linux/nfs_fs.h | 2 +- 7 files changed, 32 insertions(+), 19 deletions(-) diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index 4471a41..5362714 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -21,6 +21,7 @@ prototypes: char *(*d_dname)((struct dentry *dentry, char *buffer, int buflen); struct vfsmount *(*d_automount)(struct path *path); int (*d_manage)(struct dentry *, bool); + void (*d_unlink)(struct dentry *, struct dentry *); locking rules: rename_lock ->d_lock may block rcu-walk @@ -33,6 +34,7 @@ d_iput: no no yes no d_dname: no no no no d_automount: no no yes no d_manage: no no yes (ref-walk) maybe +d_unlink no no yes no --------------------------- inode_operations --------------------------- prototypes: diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 94cf97b..a69d978 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -866,6 +866,7 @@ struct dentry_operations { char *(*d_dname)(struct dentry *, char *, int); struct vfsmount *(*d_automount)(struct path *); int (*d_manage)(struct dentry *, bool, bool); + void (*d_unlink)(struct dentry *, struct dentry *); }; d_revalidate: called when the VFS needs to revalidate a dentry. This @@ -973,6 +974,14 @@ struct dentry_operations { This function is only used if DCACHE_MANAGE_TRANSIT is set on the dentry being transited from. + d_unlink: called to allow the filesystem to unlink the dentry after final + use. It is only called when DCACHE_NFSFS_RENAMED is set, and is + designed for use by 'sillyrename' schemes that are commonly + implemented on distributed filesystems such as NFS. + + Note that the filesystem is still responsible for protecting against + races with other lookups. + Example : static char *pipefs_dname(struct dentry *dent, char *buffer, int buflen) diff --git a/fs/dcache.c b/fs/dcache.c index 2a6bd9a..f4dea50 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -301,6 +301,9 @@ static struct dentry *d_kill(struct dentry *dentry, struct dentry *parent) if (parent) spin_unlock(&parent->d_lock); dentry_iput(dentry); + + if (dentry->d_flags & DCACHE_NFSFS_RENAMED) + dentry->d_op->d_unlink(parent, dentry); /* * dentry_iput drops the locks, at which point nobody (except * transient RCU lookups) can reach this dentry. diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 2c3eb33..322c882 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1161,19 +1161,22 @@ static void nfs_dentry_iput(struct dentry *dentry, struct inode *inode) if (S_ISDIR(inode->i_mode)) /* drop any readdir cache as it could easily be old */ NFS_I(inode)->cache_validity |= NFS_INO_INVALID_DATA; - - if (dentry->d_flags & DCACHE_NFSFS_RENAMED) { + if (dentry->d_flags & DCACHE_NFSFS_RENAMED) drop_nlink(inode); - nfs_complete_unlink(dentry, inode); - } iput(inode); } +static void nfs_d_unlink(struct dentry *parent, struct dentry *dentry) +{ + nfs_complete_unlink(parent, dentry); +} + const struct dentry_operations nfs_dentry_operations = { .d_revalidate = nfs_lookup_revalidate, .d_delete = nfs_dentry_delete, .d_iput = nfs_dentry_iput, .d_automount = nfs_d_automount, + .d_unlink = nfs_d_unlink, }; static struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, struct nameidata *nd) @@ -1248,6 +1251,7 @@ const struct dentry_operations nfs4_dentry_operations = { .d_delete = nfs_dentry_delete, .d_iput = nfs_dentry_iput, .d_automount = nfs_d_automount, + .d_unlink = nfs_d_unlink, }; /* diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c index e313a51..9160cad 100644 --- a/fs/nfs/unlink.c +++ b/fs/nfs/unlink.c @@ -184,19 +184,17 @@ static int nfs_do_call_unlink(struct dentry *parent, struct inode *dir, struct n return 1; } -static int nfs_call_unlink(struct dentry *dentry, struct nfs_unlinkdata *data) +static int nfs_call_unlink(struct dentry *parent, struct dentry *dentry, struct nfs_unlinkdata *data) { - struct dentry *parent; struct inode *dir; int ret = 0; - parent = dget_parent(dentry); if (parent == NULL) - goto out_free; + goto out; dir = parent->d_inode; if (nfs_copy_dname(dentry, data) != 0) - goto out_dput; + goto out; /* Non-exclusive lock protects against concurrent lookup() calls */ spin_lock(&dir->i_lock); if (atomic_inc_not_zero(&NFS_I(dir)->silly_count) == 0) { @@ -204,13 +202,11 @@ static int nfs_call_unlink(struct dentry *dentry, struct nfs_unlinkdata *data) hlist_add_head(&data->list, &NFS_I(dir)->silly_list); spin_unlock(&dir->i_lock); ret = 1; - goto out_dput; + goto out; } spin_unlock(&dir->i_lock); ret = nfs_do_call_unlink(parent, dir, data); -out_dput: - dput(parent); -out_free: +out: return ret; } @@ -283,26 +279,24 @@ out: /** * nfs_complete_unlink - Initialize completion of the sillydelete + * @parent: parent directory * @dentry: dentry to delete - * @inode: inode * * Since we're most likely to be called by dentry_iput(), we * only use the dentry to find the sillydelete. We then copy the name * into the qstr. */ void -nfs_complete_unlink(struct dentry *dentry, struct inode *inode) +nfs_complete_unlink(struct dentry *parent, struct dentry *dentry) { struct nfs_unlinkdata *data = NULL; - spin_lock(&dentry->d_lock); if (dentry->d_flags & DCACHE_NFSFS_RENAMED) { dentry->d_flags &= ~DCACHE_NFSFS_RENAMED; data = dentry->d_fsdata; } - spin_unlock(&dentry->d_lock); - if (data != NULL && (NFS_STALE(inode) || !nfs_call_unlink(dentry, data))) + if (data != NULL && !nfs_call_unlink(parent, dentry, data)) nfs_free_unlinkdata(data); } diff --git a/include/linux/dcache.h b/include/linux/dcache.h index f958c19..a3fd768 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -169,6 +169,7 @@ struct dentry_operations { char *(*d_dname)(struct dentry *, char *, int); struct vfsmount *(*d_automount)(struct path *); int (*d_manage)(struct dentry *, bool, bool); + void (*d_unlink)(struct dentry *, struct dentry *); } ____cacheline_aligned; /* diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 6023efa..11d4b40 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -488,7 +488,7 @@ extern void nfs_release_automount_timer(void); /* * linux/fs/nfs/unlink.c */ -extern void nfs_complete_unlink(struct dentry *dentry, struct inode *); +extern void nfs_complete_unlink(struct dentry *dentry, struct dentry *); extern void nfs_block_sillyrename(struct dentry *dentry); extern void nfs_unblock_sillyrename(struct dentry *dentry); extern int nfs_sillyrename(struct inode *dir, struct dentry *dentry); -- 1.7.3.5 -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... @ 2011-01-26 20:43 ` Trond Myklebust 0 siblings, 0 replies; 16+ messages in thread From: Trond Myklebust @ 2011-01-26 20:43 UTC (permalink / raw) To: J. R. Okajima; +Cc: linux-nfs, Nick Piggin, Linux Filesystem Development On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: > The alternative would be to add a callback that can be called after > dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent > and (negative) dentry as the arguments. > sillyrename doesn't need the inode as an argument, but it definitely > needs the parent dentry so that it can check for races with > ->lookup()... The following (compile tested only!) patch illustrates what I mean. Cheers Trond 8<-------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <1296074585.7127.33.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-26 20:43 ` Trond Myklebust @ 2011-01-26 23:50 ` Nick Piggin -1 siblings, 0 replies; 16+ messages in thread From: Nick Piggin @ 2011-01-26 23:50 UTC (permalink / raw) To: Trond Myklebust Cc: J. R. Okajima, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Linux Filesystem Development On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: >> The alternative would be to add a callback that can be called after >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent >> and (negative) dentry as the arguments. >> sillyrename doesn't need the inode as an argument, but it definitely >> needs the parent dentry so that it can check for races with >> ->lookup()... > > The following (compile tested only!) patch illustrates what I mean. We could do this. CEPH also want a way to get d_parent in the inode unlink path. I think I can actually check for dentry->d_count == 0 rather than dentry->d_parent == NULL here, and avoid clearing d_parent entirely. That might be the better solution for 2.6.38, because other code I've missed might be expecting to use d_parent. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... @ 2011-01-26 23:50 ` Nick Piggin 0 siblings, 0 replies; 16+ messages in thread From: Nick Piggin @ 2011-01-26 23:50 UTC (permalink / raw) To: Trond Myklebust; +Cc: J. R. Okajima, linux-nfs, Linux Filesystem Development On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: >> The alternative would be to add a callback that can be called after >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent >> and (negative) dentry as the arguments. >> sillyrename doesn't need the inode as an argument, but it definitely >> needs the parent dentry so that it can check for races with >> ->lookup()... > > The following (compile tested only!) patch illustrates what I mean. We could do this. CEPH also want a way to get d_parent in the inode unlink path. I think I can actually check for dentry->d_count == 0 rather than dentry->d_parent == NULL here, and avoid clearing d_parent entirely. That might be the better solution for 2.6.38, because other code I've missed might be expecting to use d_parent. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-26 23:50 ` Nick Piggin (?) @ 2011-01-26 23:59 ` Trond Myklebust [not found] ` <1296086349.7127.55.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> -1 siblings, 1 reply; 16+ messages in thread From: Trond Myklebust @ 2011-01-26 23:59 UTC (permalink / raw) To: Nick Piggin; +Cc: J. R. Okajima, linux-nfs, Linux Filesystem Development On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote: > On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust > <Trond.Myklebust@netapp.com> wrote: > > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: > >> The alternative would be to add a callback that can be called after > >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent > >> and (negative) dentry as the arguments. > >> sillyrename doesn't need the inode as an argument, but it definitely > >> needs the parent dentry so that it can check for races with > >> ->lookup()... > > > > The following (compile tested only!) patch illustrates what I mean. > > We could do this. CEPH also want a way to get d_parent in the inode > unlink path. > > I think I can actually check for dentry->d_count == 0 rather than > dentry->d_parent == NULL here, and avoid clearing d_parent > entirely. That might be the better solution for 2.6.38, because other > code I've missed might be expecting to use d_parent. I'm not sure I understand. By the time we hit d_kill() we know that dentry->d_count == 0. The only thing we need here is the ability to unlink the file if it has been sillyrenamed previously. The reason for needing the parent dentry is to allow the filesystem to guard against races with lookup without requiring the vfs to take the parent->d_inode->i_mutex in the dput() path... -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <1296086349.7127.55.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-26 23:59 ` Trond Myklebust @ 2011-01-27 0:44 ` Nick Piggin 0 siblings, 0 replies; 16+ messages in thread From: Nick Piggin @ 2011-01-27 0:44 UTC (permalink / raw) To: Trond Myklebust Cc: J. R. Okajima, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Linux Filesystem Development On Thu, Jan 27, 2011 at 10:59 AM, Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: > On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote: >> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust >> <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: >> > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: >> >> The alternative would be to add a callback that can be called after >> >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent >> >> and (negative) dentry as the arguments. >> >> sillyrename doesn't need the inode as an argument, but it definitely >> >> needs the parent dentry so that it can check for races with >> >> ->lookup()... >> > >> > The following (compile tested only!) patch illustrates what I mean. >> >> We could do this. CEPH also want a way to get d_parent in the inode >> unlink path. >> >> I think I can actually check for dentry->d_count == 0 rather than >> dentry->d_parent == NULL here, and avoid clearing d_parent >> entirely. That might be the better solution for 2.6.38, because other >> code I've missed might be expecting to use d_parent. > > I'm not sure I understand. By the time we hit d_kill() we know that > dentry->d_count == 0. The dcache patch to set d_parent to NULL was done so that when walking the other way up the path we can check that the parent is still valid. But we could just check for d_count != 0 which means the parent has to be valid as well (we have rename protection as well). -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... @ 2011-01-27 0:44 ` Nick Piggin 0 siblings, 0 replies; 16+ messages in thread From: Nick Piggin @ 2011-01-27 0:44 UTC (permalink / raw) To: Trond Myklebust; +Cc: J. R. Okajima, linux-nfs, Linux Filesystem Development On Thu, Jan 27, 2011 at 10:59 AM, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote: >> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust >> <Trond.Myklebust@netapp.com> wrote: >> > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: >> >> The alternative would be to add a callback that can be called after >> >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent >> >> and (negative) dentry as the arguments. >> >> sillyrename doesn't need the inode as an argument, but it definitely >> >> needs the parent dentry so that it can check for races with >> >> ->lookup()... >> > >> > The following (compile tested only!) patch illustrates what I mean. >> >> We could do this. CEPH also want a way to get d_parent in the inode >> unlink path. >> >> I think I can actually check for dentry->d_count == 0 rather than >> dentry->d_parent == NULL here, and avoid clearing d_parent >> entirely. That might be the better solution for 2.6.38, because other >> code I've missed might be expecting to use d_parent. > > I'm not sure I understand. By the time we hit d_kill() we know that > dentry->d_count == 0. The dcache patch to set d_parent to NULL was done so that when walking the other way up the path we can check that the parent is still valid. But we could just check for d_count != 0 which means the parent has to be valid as well (we have rename protection as well). ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <AANLkTim1-dwNjTpyCi5qBjCer5vgPo4qSNfK7Htd_vfL-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-27 0:44 ` Nick Piggin @ 2011-01-27 0:57 ` Trond Myklebust -1 siblings, 0 replies; 16+ messages in thread From: Trond Myklebust @ 2011-01-27 0:57 UTC (permalink / raw) To: Nick Piggin Cc: J. R. Okajima, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Linux Filesystem Development On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote: > On Thu, Jan 27, 2011 at 10:59 AM, Trond Myklebust > <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: > > On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote: > >> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust > >> <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: > >> > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: > >> >> The alternative would be to add a callback that can be called after > >> >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent > >> >> and (negative) dentry as the arguments. > >> >> sillyrename doesn't need the inode as an argument, but it definitely > >> >> needs the parent dentry so that it can check for races with > >> >> ->lookup()... > >> > > >> > The following (compile tested only!) patch illustrates what I mean. > >> > >> We could do this. CEPH also want a way to get d_parent in the inode > >> unlink path. > >> > >> I think I can actually check for dentry->d_count == 0 rather than > >> dentry->d_parent == NULL here, and avoid clearing d_parent > >> entirely. That might be the better solution for 2.6.38, because other > >> code I've missed might be expecting to use d_parent. > > > > I'm not sure I understand. By the time we hit d_kill() we know that > > dentry->d_count == 0. > > The dcache patch to set d_parent to NULL was done so that when > walking the other way up the path we can check that the parent is > still valid. > > But we could just check for d_count != 0 which means the parent has > to be valid as well (we have rename protection as well). That would certainly be less intrusive. Can you please propose a patch? Cheers Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... @ 2011-01-27 0:57 ` Trond Myklebust 0 siblings, 0 replies; 16+ messages in thread From: Trond Myklebust @ 2011-01-27 0:57 UTC (permalink / raw) To: Nick Piggin; +Cc: J. R. Okajima, linux-nfs, Linux Filesystem Development On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote: > On Thu, Jan 27, 2011 at 10:59 AM, Trond Myklebust > <Trond.Myklebust@netapp.com> wrote: > > On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote: > >> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust > >> <Trond.Myklebust@netapp.com> wrote: > >> > On Wed, 2011-01-26 at 15:14 -0500, Trond Myklebust wrote: > >> >> The alternative would be to add a callback that can be called after > >> >> dentry_iput() if DCACHE_NFSFS_RENAMED is true, and that takes the parent > >> >> and (negative) dentry as the arguments. > >> >> sillyrename doesn't need the inode as an argument, but it definitely > >> >> needs the parent dentry so that it can check for races with > >> >> ->lookup()... > >> > > >> > The following (compile tested only!) patch illustrates what I mean. > >> > >> We could do this. CEPH also want a way to get d_parent in the inode > >> unlink path. > >> > >> I think I can actually check for dentry->d_count == 0 rather than > >> dentry->d_parent == NULL here, and avoid clearing d_parent > >> entirely. That might be the better solution for 2.6.38, because other > >> code I've missed might be expecting to use d_parent. > > > > I'm not sure I understand. By the time we hit d_kill() we know that > > dentry->d_count == 0. > > The dcache patch to set d_parent to NULL was done so that when > walking the other way up the path we can check that the parent is > still valid. > > But we could just check for d_count != 0 which means the parent has > to be valid as well (we have rename protection as well). That would certainly be less intrusive. Can you please propose a patch? Cheers Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <1296089830.25999.1.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-27 0:57 ` Trond Myklebust @ 2011-01-27 1:25 ` Nick Piggin -1 siblings, 0 replies; 16+ messages in thread From: Nick Piggin @ 2011-01-27 1:25 UTC (permalink / raw) To: Trond Myklebust Cc: J. R. Okajima, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Linux Filesystem Development On Thu, Jan 27, 2011 at 11:57 AM, Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: > On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote: >> But we could just check for d_count != 0 which means the parent has >> to be valid as well (we have rename protection as well). > > That would certainly be less intrusive. Can you please propose a patch? Yes I'll write something up today. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... @ 2011-01-27 1:25 ` Nick Piggin 0 siblings, 0 replies; 16+ messages in thread From: Nick Piggin @ 2011-01-27 1:25 UTC (permalink / raw) To: Trond Myklebust; +Cc: J. R. Okajima, linux-nfs, Linux Filesystem Development On Thu, Jan 27, 2011 at 11:57 AM, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote: >> But we could just check for d_count != 0 which means the parent has >> to be valid as well (we have rename protection as well). > > That would certainly be less intrusive. Can you please propose a patch? Yes I'll write something up today. ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <AANLkTim_JrZkD_D8HWXNr0qxjQ-=LKaAvzcH_w7A_T42-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: 2.6.38-rc2... NFS sillyrename is broken... 2011-01-27 1:25 ` Nick Piggin @ 2011-03-05 13:49 ` Jeff Layton -1 siblings, 0 replies; 16+ messages in thread From: Jeff Layton @ 2011-03-05 13:49 UTC (permalink / raw) To: Nick Piggin Cc: Trond Myklebust, J. R. Okajima, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Linux Filesystem Development On Thu, 27 Jan 2011 12:25:35 +1100 Nick Piggin <npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > On Thu, Jan 27, 2011 at 11:57 AM, Trond Myklebust > <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote: > > On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote: > > >> But we could just check for d_count != 0 which means the parent has > >> to be valid as well (we have rename protection as well). > > > > That would certainly be less intrusive. Can you please propose a patch? > > Yes I'll write something up today. Hi Nick, I was doing some testing earlier and noticed that NFS sillyrenames are still broken as of -rc7. Did you ever get around to doing a patch to fix this? Thanks, -- Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 2.6.38-rc2... NFS sillyrename is broken... @ 2011-03-05 13:49 ` Jeff Layton 0 siblings, 0 replies; 16+ messages in thread From: Jeff Layton @ 2011-03-05 13:49 UTC (permalink / raw) To: Nick Piggin Cc: Trond Myklebust, J. R. Okajima, linux-nfs, Linux Filesystem Development On Thu, 27 Jan 2011 12:25:35 +1100 Nick Piggin <npiggin@gmail.com> wrote: > On Thu, Jan 27, 2011 at 11:57 AM, Trond Myklebust > <Trond.Myklebust@netapp.com> wrote: > > On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote: > > >> But we could just check for d_count != 0 which means the parent has > >> to be valid as well (we have rename protection as well). > > > > That would certainly be less intrusive. Can you please propose a patch? > > Yes I'll write something up today. Hi Nick, I was doing some testing earlier and noticed that NFS sillyrenames are still broken as of -rc7. Did you ever get around to doing a patch to fix this? Thanks, -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-03-05 13:49 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-25 23:30 2.6.38-rc2... NFS sillyrename is broken Trond Myklebust
2011-01-26 8:20 ` J. R. Okajima
2011-01-26 20:14 ` Trond Myklebust
2011-01-26 20:43 ` Trond Myklebust
2011-01-26 20:43 ` Trond Myklebust
[not found] ` <1296074585.7127.33.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>
2011-01-26 23:50 ` Nick Piggin
2011-01-26 23:50 ` Nick Piggin
2011-01-26 23:59 ` Trond Myklebust
[not found] ` <1296086349.7127.55.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>
2011-01-27 0:44 ` Nick Piggin
2011-01-27 0:44 ` Nick Piggin
[not found] ` <AANLkTim1-dwNjTpyCi5qBjCer5vgPo4qSNfK7Htd_vfL-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-01-27 0:57 ` Trond Myklebust
2011-01-27 0:57 ` Trond Myklebust
[not found] ` <1296089830.25999.1.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>
2011-01-27 1:25 ` Nick Piggin
2011-01-27 1:25 ` Nick Piggin
[not found] ` <AANLkTim_JrZkD_D8HWXNr0qxjQ-=LKaAvzcH_w7A_T42-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-03-05 13:49 ` Jeff Layton
2011-03-05 13:49 ` Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.