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
[email protected]
http://www.netapp.com
On Thu, Jan 27, 2011 at 11:57 AM, Trond Myklebust
<[email protected]> 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.
On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust
<[email protected]> 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.
On Thu, Jan 27, 2011 at 10:59 AM, Trond Myklebust
<[email protected]> wrote:
> On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote:
>> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust
>> <[email protected]> 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).
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
[email protected]
http://www.netapp.com
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<--------------------------------------------------------------------------
On Thu, 2011-01-27 at 11:44 +1100, Nick Piggin wrote:
> On Thu, Jan 27, 2011 at 10:59 AM, Trond Myklebust
> <[email protected]> wrote:
> > On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote:
> >> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust
> >> <[email protected]> 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
[email protected]
http://www.netapp.com
(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
On Thu, 2011-01-27 at 10:50 +1100, Nick Piggin wrote:
> On Thu, Jan 27, 2011 at 7:43 AM, Trond Myklebust
> <[email protected]> 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
[email protected]
http://www.netapp.com
On Thu, 27 Jan 2011 12:25:35 +1100
Nick Piggin <[email protected]> wrote:
> On Thu, Jan 27, 2011 at 11:57 AM, Trond Myklebust
> <[email protected]> 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 <[email protected]>