Hi,
[Previously sent to nfs@sourceforge with no response]
I'm using a frankenstein kernel, 2.4.21-rc3 with some -ac bits,
and 2.5.69 NFS+RPC backported to it. Like the CITI kernel (for krb5),
but a little more aggressive on the bits backported. For the purpose
of this email, I think the code I have questions with is similar or even
identical from 2.4.21->2.5.69. I can reproduce this problem on a RH
2.4.20-9smp kernel.
Consider these two shells running on the same machine:
1 2
cd /nfs cd /nfs
mkdir t
echo foo > t/foo
less t/foo
[less waits for input]
rm -rf t
'v'
[vi tries to access tmp/foo]
At this point, fs/nfs/inode.c:__nfs_refresh_inode() prints the "inode
number mismatch" error. AFAICT, this is just noise, but the noise is
driving me crazy. :-)
Now, if sequence 2 is run on a different machine, there is no error!
So that hints to me that the local cache just needs to be cleared,
perhaps in nfs_rmdir() or maybe in nfs_unlink()/nfs_safe_remove().
I've tried a few things, but I'm not familiar enough with the code
and am making slow progress. I can suppress this error by testing
for 'unlinked but open' in __nfs_refresh_inode:
if (NFS_FILEID(inode) != fattr->fileid) {
if (inode->i_nlink) /* quiet if inode DNE anymore */
printk(...)
}
Do you think this is safe? Some minimal logs:
kernel: NFS: dentry_delete(t/.nfs01c7d70600000001, 2) | renamed file
kernel: NFS: delete_inode(e/29873926) | unlink of renamed foo
kernel: NFS: refresh_inode(e/29873923 ct=1 info=0x6) | accessing t/
kernel: nfs_refresh_inode: inode number mismatch
kernel: expected (0xe/0x1c7d703), got (0xe/0xe63bc2)
kernel: NFS: dentry_delete(fsstress/t, 0)
kernel: NFS: delete_inode(e/29873923)
and then access calls beginning at the root. I apologize for the likely
uselessness of the above logs. I can email some annotated logs if desired,
but the problem is very easy to reproduce, so I'll hold off for now.
This problem only exists for nfsv3. This problem doesn't occur if there
is a third process also holding foo open (note that the directory does
get removed, just no kernel error when trying to access it).
The 2.2 kernel doesn't have this problem, because (apparently) it doesn't
allow you to unlink a .nfsXXX file while it's open (and therefore you
cannot remove the dir).
Which made me look around (2.5.69): In nfs_silly_rename(), the new
dentry (sdentry) gets a d_count of 1. Doesn't this indicate that no
one is holding this file open? (which then tells nfs_unlink() to just
call nfs_safe_remove() rather than nfs_silly_rename()) Is that really
desirable? Even if I set the d_count to match what the previous
dentry->d_count had, and avoid calling dput(sdentry), on the next run
through nfs_unlink() the d_count is 1 and it just goes to nfs_safe_remove().
I think that clearly, I don't understand what the d_move() is for.
(My guess is to avoid nfs_async_unlink() getting passed a dentry which
we are actually about to get rid of, but I haven't wrapped my head around
the dcache yet.)
Then I noticed that the DCACHE_NFSFS_RENAMED seems a little racy.
nfs_async_unlink() sets this and when the call completes,
nfs_complete_unlink() resets it. So while it's being deleted, if an
rm -rf quickly picks up the .nfs name before the async unlink returns,
it won't get removed. But if the nfs call completes first, it does
get removed. Is the intention just to prevent removal of the .nfs
file until the old file is removed on the server? What's the benefit
of this?
So, even with that error message quieted, fsstress reports lots of
inode mismatches. I am in the process of trying to piece together a
simple reproducible sequence of NFS calls.
This is against a netapp server, although I can't see how the server would
matter.
Thanks for any advice, guidance, or hopefully fixes! BTW, I'm interested
to hear what tools folks use to stress the NFS client.
/fc
>>>>> " " == Frank Cusack <[email protected]> writes:
> Hi, [Previously sent to nfs@sourceforge with no response]
> I'm using a frankenstein kernel, 2.4.21-rc3 with some -ac bits,
> and 2.5.69 NFS+RPC backported to it. Like the CITI kernel (for
> krb5), but a little more aggressive on the bits backported.
> For the purpose of this email, I think the code I have
> questions with is similar or even identical from
> 2.4.21->2.5.69. I can reproduce this problem on a RH
> 2.4.20-9smp kernel.
> Consider these two shells running on the same machine:
> 1 2
> cd /nfs cd /nfs mkdir t echo foo > t/foo less t/foo
> [less waits for input]
> rm -rf t
> 'v'
> [vi tries to access tmp/foo]
> At this point, fs/nfs/inode.c:__nfs_refresh_inode() prints the
> "inode number mismatch" error. AFAICT, this is just noise, but
> the noise is driving me crazy. :-)
Inode number mismatch points to either an an obvious server error (it
is not providing unique filehandles) or corruption of the fattr struct
that was passed to nfs_refresh_inode().
Cheers,
Trond
On Wed, Jun 04, 2003 at 04:19:38PM +0200, Trond Myklebust wrote:
> >>>>> " " == Frank Cusack <[email protected]> writes:
> > At this point, fs/nfs/inode.c:__nfs_refresh_inode() prints the
> > "inode number mismatch" error. AFAICT, this is just noise, but
> > the noise is driving me crazy. :-)
>
> Inode number mismatch points to either an an obvious server error (it
> is not providing unique filehandles) or corruption of the fattr struct
> that was passed to nfs_refresh_inode().
Clearly it's not the former. No way a netapp filer is going to have
this problem. I can't imagine *any* nfs server having this problem.
Could you take another look at the specific case I cited? At the time
I try to access the file, the path to it no longer exists. No information
on this file should exist.
/fc
>>>>> " " == Frank Cusack <[email protected]> writes:
> Could you take another look at the specific case I cited? At
> the time I try to access the file, the path to it no longer
> exists. No information on this file should exist.
I cannot duplicate.
Cheers,
Trond
On Wed, 4 Jun 2003 14:20:47 -0700
"Frank Cusack" <[email protected]> wrote:
> On Wed, Jun 04, 2003 at 04:19:38PM +0200, Trond Myklebust wrote:
> > >>>>> " " == Frank Cusack <[email protected]> writes:
> > > At this point, fs/nfs/inode.c:__nfs_refresh_inode() prints
> > > the"inode number mismatch" error. AFAICT, this is just
> > > noise, but the noise is driving me crazy. :-)
> >
> > Inode number mismatch points to either an an obvious server error
> > (it is not providing unique filehandles) or corruption of the fattr
> > struct that was passed to nfs_refresh_inode().
There's a very common cause on embedded boards that don't have
real-time clocks. Without a clock the client uses the same XID on every
run, leading to lots of these messages. Is your clock broken?
- Adrian Cox
http://www.humboldt.co.uk/
On Thu, Jun 05, 2003 at 10:11:20AM +0100, Adrian Cox wrote:
> There's a very common cause on embedded boards that don't have
> real-time clocks. Without a clock the client uses the same XID on every
> run, leading to lots of these messages. Is your clock broken?
BTDT.
If this is the case, you need to ensure that you don't reboot the client
before the servers XID cache times out the XID numbers. For Linux knfsd,
that's around 2 minutes.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
>>>>> " " == Russell King <[email protected]> writes:
> If this is the case, you need to ensure that you don't reboot
> the client before the servers XID cache times out the XID
> numbers. For Linux knfsd, that's around 2 minutes.
Note that older versions of knfsd didn't time out their replay cache
at all...
Cheers,
Trond
OK, I've studied this a lot more and think I've figured it out. I think
calling it a race is incorrect, but I don't know how to succintly describe
the problem otherwise.
Let's look at the first attachment, a test case which shows the problem.
When run against a Solaris NFS server, the readdir()/unlink() loop returns
".", "..", "foo", NULL, and the rmdir fails. I suspect a Linux NFS
server does the same. But when run against a netapp, it returns
".", "..", "foo", ".nfs...", NULL, ie it picks up the sillyrenamed
file immediately, and this allows the rmdir to succeed.
When foo is unlinked, nfs_unlink() does a sillyrename, this puts the
dentry on nfs_delete_queue, and (in the VFS) unhashes it from the dcache.
This causes a problem, because dentry->d_parent->d_inode is now guaranteed
to remain stale. (OK, I'm not really sure about this last part.)
Then readdir() returns the new .nfs file, this creates a NEW dentry
(we just moved the first one to nfs_delete_queue and did not create a
negative dentry) which now has d_count==1 so instead of sillyrename we
just remove it (but note, we actually have this file open). Then rmdir
succeeeds.
Now, we have a dentry on nfs_delete_queue which a) has already been
unlinked and b) whose dentry->d_parent DNE and dentry->d_parent->d_inode
DNE. Of course this will cause confusion later!
Note that if a process does a drive by on the .nfs file (another round
of unlinked-but-open) before we unlink it, we would sillyrename it again.
We'd now have two different dentry's on the delete queue for the same
file. (One of them would just leak, I think--possible local DoS?)
Also note that in vfs_unlink(), we do a d_delete() after the
i_op->unlink(), I think this guarantees that no call to sillyrename will
ever be passed a dentry with the DCACHE_NFSFS_RENAMED flag set (it gets
set in sillyrename, but then unhashed; the next time through vfs_unlink()
we have a new dentry with no flags set). The DCACHE_NFSFS_RENAMED bit
looks to be a holdover from 2.2, where the dentry doesn't get unhashed.
I have 3 proposals for fixes, I've implemented 2 of them and patches
are attached against both 2.4.21-rc7 and 2.5.70.
1) Don't unhash the dentry after silly-renaming. In 2.2, each fs is
responsible for doing a d_delete(), in 2.4 it happens in the VFS and
I think it was just an oversight that the 2.4 VFS doesn't consider
sillyrename (considering the code and comments that are cruft).
This preserves the unlinked-but-open semantic, but breaks rmdir. So
it's not a clear winner from a semantics POV. dentry->d_count is
always correct, which sounds like a plus.
The patch to make this work is utterly simple, which is a big plus.
2a) In nfs_unlink(), if not sillyrenaming, look for the file on
nfs_delete_queue. If present, remove it (since it's now extra).
This has the advantage of preserving the 'rm -rf' semantic, but
breaks unlinked-but-open since the parent dir can go away, and
dentry->d_count is not guaranteed to be correct.
I've implemented but not included this.
2b) Since this is really only a problem when the parent dir goes away,
do the same as above but only scan the queue in nfs_rmdir(), and
mark any entries whose d_parent is "us".
I've included this in favor of (2a) because it's simpler and should
give better performance in the common case.
3) In nfs_complete_unlink(), do a d_lookup() before waking the rpc task.
If an entry is found, go head and schedule the rpc. If no entry is
found, or a negative entry is found, just remove from the queue.
I couldn't get this to work.
With the #2 patch, I've taken the liberty of cleaning up stale comments.
How annoying they were while trying to understand the code. :-( I would
have done the same for the #1 patch, but it's so small that I didn't
want to otherwise "pollute" it. If you decide to go with the #1 patch,
at least do look at the #2 patch for comment fixups.
If you need more data to convince yourself of this bug, I'm definitely
able to provide more data, just tell me what you'd like to see. But I
hope I've adequately described it.
/fc
Forgot to mention, this affects NFSv3 only. Maybe because NFSv2 doesn't
have the cookie verifier and so readdir() always restarts? Something
like that.
/fc
Good job.
On Mon, 9 Jun 2003, Frank Cusack wrote:
>
> 1) Don't unhash the dentry after silly-renaming. In 2.2, each fs is
> responsible for doing a d_delete(), in 2.4 it happens in the VFS and
> I think it was just an oversight that the 2.4 VFS doesn't consider
> sillyrename (considering the code and comments that are cruft).
>
> This preserves the unlinked-but-open semantic, but breaks rmdir. So
> it's not a clear winner from a semantics POV. dentry->d_count is
> always correct, which sounds like a plus.
>
> The patch to make this work is utterly simple, which is a big plus.
I think your #1 is "obviously correct", but the fact that it breaks rmdir
sounds like a bummer. However, since it only breaks rmdir when
silly-renames exist - and since silly-renames should only happen when you
have a file descriptor still open - I'd be inclined to say that this is
the right behaviour.
> 2b) Since this is really only a problem when the parent dir goes away,
> do the same as above but only scan the queue in nfs_rmdir(), and
> mark any entries whose d_parent is "us".
>
> I've included this in favor of (2a) because it's simpler and should
> give better performance in the common case.
This sounds like a hack, even if it happens to work.
I dunno. I'm personally inclined to prefer (1), since that seems to just
fix a bug in the VFS layer and doesn't introduce any conceptual
complexity. But I think I'd let Trond make the final decision, I don't
hate (2b) enough to say "over my dead body!".
Trond?
Linus
>>>>> " " == Linus Torvalds <[email protected]> writes:
> I think your #1 is "obviously correct", but the fact that it
> breaks rmdir sounds like a bummer. However, since it only
> breaks rmdir when silly-renames exist - and since silly-renames
> should only happen when you have a file descriptor still open -
> I'd be inclined to say that this is the right behaviour.
I agree.
If people prefer 'rm -rf' correctness instead of unlinked-but-open,
then we could do that by changing the behaviour of 'unlink' on a
silly-deleted filed. Currently it returns EBUSY, but we could just as
well have it complete the unlink, and mark the inode as being stale...
Cheers,
Trond
On Mon, Jun 09, 2003 at 06:40:43PM +0200, Trond Myklebust wrote:
> If people prefer 'rm -rf' correctness instead of unlinked-but-open,
> then we could do that by changing the behaviour of 'unlink' on a
> silly-deleted filed. Currently it returns EBUSY, but we could just as
> well have it complete the unlink, and mark the inode as being stale...
Actually, *currently* it unlinks. :-) That's the problem.
/fc
>>>>> " " == Frank Cusack <[email protected]> writes:
> On Mon, Jun 09, 2003 at 06:40:43PM +0200, Trond Myklebust
> wrote:
>> If people prefer 'rm -rf' correctness instead of
>> unlinked-but-open, then we could do that by changing the
>> behaviour of 'unlink' on a silly-deleted filed. Currently it
>> returns EBUSY, but we could just as well have it complete the
>> unlink, and mark the inode as being stale...
> Actually, *currently* it unlinks. :-) That's the problem.
No. The problem is that it aliases the dentry, and so it unlinks
incorrectly...
Cheers,
Trond
On Mon, Jun 09, 2003 at 06:51:41AM -0700, Frank Cusack wrote:
> When foo is unlinked, nfs_unlink() does a sillyrename, this puts the
> dentry on nfs_delete_queue, and (in the VFS) unhashes it from the dcache.
> This causes a problem, because dentry->d_parent->d_inode is now guaranteed
> to remain stale. (OK, I'm not really sure about this last part.)
????
What does hashed state have to ->d_parent?
> Then readdir() returns the new .nfs file, this creates a NEW dentry
> (we just moved the first one to nfs_delete_queue and did not create a
> negative dentry) which now has d_count==1 so instead of sillyrename we
> just remove it (but note, we actually have this file open). Then rmdir
> succeeeds.
>
> Now, we have a dentry on nfs_delete_queue which a) has already been
> unlinked and b) whose dentry->d_parent DNE and dentry->d_parent->d_inode
> DNE. Of course this will cause confusion later!
b) is bogus. Unhashing does nothing to ->d_parent.
> Note that if a process does a drive by on the .nfs file (another round
> of unlinked-but-open) before we unlink it, we would sillyrename it again.
> We'd now have two different dentry's on the delete queue for the same
> file. (One of them would just leak, I think--possible local DoS?)
Two different dentries for the same file is obviously not a problem...
> 1) Don't unhash the dentry after silly-renaming. In 2.2, each fs is
> responsible for doing a d_delete(), in 2.4 it happens in the VFS and
> I think it was just an oversight that the 2.4 VFS doesn't consider
> sillyrename (considering the code and comments that are cruft).
>
> This preserves the unlinked-but-open semantic, but breaks rmdir. So
> it's not a clear winner from a semantics POV. dentry->d_count is
> always correct, which sounds like a plus.
>
> The patch to make this work is utterly simple, which is a big plus.
... and AFAICS it opens a huge can of worms with races in NFS unlink/rename.
Sigh... I'll look through that code and try to reconstruct the analysis.
It used to be a very big mess and there was fairly subtle logics around
unhashing/checks for d_count/etc. involved in fixing ;-/ And there was
a lot of changes since then. Oh, well...
On Wed, Jun 11, 2003 at 01:54:25AM +0100, [email protected] wrote:
> On Mon, Jun 09, 2003 at 06:51:41AM -0700, Frank Cusack wrote:
>
> > When foo is unlinked, nfs_unlink() does a sillyrename, this puts the
> > dentry on nfs_delete_queue, and (in the VFS) unhashes it from the dcache.
> > This causes a problem, because dentry->d_parent->d_inode is now guaranteed
> > to remain stale. (OK, I'm not really sure about this last part.)
>
> ????
>
> What does hashed state have to ->d_parent?
Because now d_parent can be d_deleted (no children) and then go away.
Then won't dentry->d_parent be wrong? What happens if d_parent becomes
a negative d_entry v. disappearing entirely.
Like I said, I didn't study this part well enough to be sure even what
I'm talking about, much less the conclusion. :-)
> > Then readdir() returns the new .nfs file, this creates a NEW dentry
> > (we just moved the first one to nfs_delete_queue and did not create a
> > negative dentry) which now has d_count==1 so instead of sillyrename we
> > just remove it (but note, we actually have this file open). Then rmdir
> > succeeeds.
> >
> > Now, we have a dentry on nfs_delete_queue which a) has already been
> > unlinked and b) whose dentry->d_parent DNE and dentry->d_parent->d_inode
> > DNE. Of course this will cause confusion later!
>
> b) is bogus. Unhashing does nothing to ->d_parent.
Except that d_parent can now disappear, right? Because it no longer knows
it has children. Or is that wrong.
> > Note that if a process does a drive by on the .nfs file (another round
> > of unlinked-but-open) before we unlink it, we would sillyrename it again.
> > We'd now have two different dentry's on the delete queue for the same
> > file. (One of them would just leak, I think--possible local DoS?)
>
> Two different dentries for the same file is obviously not a problem...
It is if d_count goes to 0 on one of them and the inode is then unlinked.
But the other dentry remains and again tries to unlink when its d_count
goes to 0. Over NFS the fh includes the generation and so you can't
accidentally delete what only looks like the same file, but what happens
in the local fs?
Also, the real problem is that something goes wrong with d_parent and
NFS tries to refresh an inode that it should really know doesn't exist
anymore. That's why my #2 patch only executes during a rmdir.
I may not have 100% accurately captured the problem, but the effect is
easy to see.
As for the last sentence about the DoS, I can trivially construct
a program which just keeps creating dentries on the nfs_delete_queue
that will never go away. Because the original dentry gets unhashed and
when the file is looked up again it doesn't have the flags, so it gets
sillyrenamed again... and again ... and again. The nfs_delete_queue
processing stops at the first hit.
> > 1) Don't unhash the dentry after silly-renaming. In 2.2, each fs is
> > responsible for doing a d_delete(), in 2.4 it happens in the VFS and
> > I think it was just an oversight that the 2.4 VFS doesn't consider
> > sillyrename (considering the code and comments that are cruft).
> >
> > This preserves the unlinked-but-open semantic, but breaks rmdir. So
> > it's not a clear winner from a semantics POV. dentry->d_count is
> > always correct, which sounds like a plus.
> >
> > The patch to make this work is utterly simple, which is a big plus.
>
> ... and AFAICS it opens a huge can of worms with races in NFS unlink/rename.
AFAICS it solves at least one of them. I actually prefer this #1 patch
to my #2 patch, I just posted the other for completeness. It's obviously
correct to leave the d_entry alone, since you did not actually remove it
from the directory. And d_count and d_flags accurately represents the
state, and nfs_unlink does treat that correctly. I'm new to the kernel,
so please educate me where that is wrong.
> Sigh... I'll look through that code and try to reconstruct the analysis.
> It used to be a very big mess and there was fairly subtle logics around
> unhashing/checks for d_count/etc. involved in fixing ;-/ And there was
> a lot of changes since then. Oh, well...
That would be very cool if you do that. There are other problems besides
the one I described, I haven't figured them out enough to post yet. But
I don't see why you wouldn't think the one liner above isn't correct.
/fc
On Tue, Jun 10, 2003 at 06:28:24PM -0700, Frank Cusack wrote:
> On Wed, Jun 11, 2003 at 01:54:25AM +0100, [email protected] wrote:
> > On Mon, Jun 09, 2003 at 06:51:41AM -0700, Frank Cusack wrote:
> >
> > > When foo is unlinked, nfs_unlink() does a sillyrename, this puts the
> > > dentry on nfs_delete_queue, and (in the VFS) unhashes it from the dcache.
> > > This causes a problem, because dentry->d_parent->d_inode is now guaranteed
> > > to remain stale. (OK, I'm not really sure about this last part.)
> >
> > ????
> >
> > What does hashed state have to ->d_parent?
>
> Because now d_parent can be d_deleted (no children) and then go away.
Why? It still has our dentry refering to it and contributing into its
->d_count.
> Then won't dentry->d_parent be wrong? What happens if d_parent becomes
> a negative d_entry v. disappearing entirely.
->d_parent will not become negative.
> It is if d_count goes to 0 on one of them and the inode is then unlinked.
> But the other dentry remains and again tries to unlink when its d_count
> goes to 0. Over NFS the fh includes the generation and so you can't
> accidentally delete what only looks like the same file, but what happens
> in the local fs?
Please, take a look at the way normal links work.
> Also, the real problem is that something goes wrong with d_parent and
Now, _that_ is interesting. What are you actually seeing there?
Note that unhashing doesn't change _any_ ->d_count - hash chains
are not counted in it and ->d_parent is not changed.
> NFS tries to refresh an inode that it should really know doesn't exist
> anymore. That's why my #2 patch only executes during a rmdir.
>>>>> " " == viro <[email protected]> writes:
> On Mon, Jun 09, 2003 at 06:51:41AM -0700, Frank Cusack wrote:
>> When foo is unlinked, nfs_unlink() does a sillyrename, this
>> puts the dentry on nfs_delete_queue, and (in the VFS) unhashes
>> it from the dcache. This causes a problem, because
>> dentry->d_parent->d_inode is now guaranteed to remain stale.
>> (OK, I'm not really sure about this last part.)
> ????
Al,
AFAICS the problem is the following:
- NFS sillyrenames dentry 1
- Upon return from nfs_unlink(), VFS unhashes dentry 1
- Upon next lookup, VFS+NFS conspire to create aliased dentry 2 to
sillyrenamed file
- Upon last close of files associated with dentry 1, NFS completes
sillyrename. File is unlinked on server.
- Aliased dentry 2 is still around, but it is now pointing to stale
fh.
IOW we just want to prevent VFS from unhashing the dentry in the first
place: dentry aliasing cannot work together with sillyrename.
Cheers,
Trond
On Wed, Jun 11, 2003 at 03:59:10AM +0200, Trond Myklebust wrote:
> AFAICS the problem is the following:
>
> - NFS sillyrenames dentry 1
> - Upon return from nfs_unlink(), VFS unhashes dentry 1
>
> - Upon next lookup, VFS+NFS conspire to create aliased dentry 2 to
> sillyrenamed file
> - Upon last close of files associated with dentry 1, NFS completes
> sillyrename. File is unlinked on server.
> - Aliased dentry 2 is still around, but it is now pointing to stale
> fh.
>
> IOW we just want to prevent VFS from unhashing the dentry in the first
> place: dentry aliasing cannot work together with sillyrename.
Aliasing could be dealt with. They would have the same inode, so it's
easy to detect. The real problem is different: what happens if I take
silly-renamed file and rename it away? You suddenly get ->dir and ->dentry
if your nfs_unlinkdata having nothing to do with each other.
_If_ we want to be able to work with silly-renamed dentry, we need much
more careful async unlink. Your current code assumes that these dentries
won't go anywhere. AFAICS, dcache will not get into inconsistent state,
but it will have very little to do with state of server...
On Wed, Jun 11, 2003 at 02:47:17AM +0100, [email protected] wrote:
> Now, _that_ is interesting. What are you actually seeing there?
> Note that unhashing doesn't change _any_ ->d_count - hash chains
> are not counted in it and ->d_parent is not changed.
ahh, ok, thanks. (Thanks also to Trond for nudging me in the correct
direction!) I will look into that some more, then.
But anyway you do agree that the one line fix is correct? You may have
gotten thrown off track because of my somewhat broken description.
/fc
On Tue, Jun 10, 2003 at 07:32:43PM -0700, Frank Cusack wrote:
> But anyway you do agree that the one line fix is correct? You may have
> gotten thrown off track because of my somewhat broken description.
As far as I can see, it doesn't solve the real problem. E.g. it allows
you to create all sorts of fun for async-unlink code in NFS - the thing
pretty much assumes that victim dentry is not going anywhere.
On Wed, Jun 11, 2003 at 03:27:54AM +0100, [email protected] wrote:
> On Wed, Jun 11, 2003 at 03:59:10AM +0200, Trond Myklebust wrote:
> > IOW we just want to prevent VFS from unhashing the dentry in the first
> > place: dentry aliasing cannot work together with sillyrename.
>
> Aliasing could be dealt with. They would have the same inode, so it's
> easy to detect.
dentry only contains the inode, not the fh. On the server, the inode
can go away and come back as a new fh, but with the same inode. Is
that detectable (would comparison hooks have to be added?)? Although,
I guess the inode is enough; you can do an NFS_I(inode)->fh to get
the fh, but I wouldn't guess you'd want that in the VFS. Bah, here
I go again ... forgive me if that's nonsense.
> The real problem is different: what happens if I take
> silly-renamed file and rename it away? You suddenly get ->dir and ->dentry
> if your nfs_unlinkdata having nothing to do with each other.
You could disallow rename if DCACHE_NFSFS_RENAMED is set. That would
be easy and makes sense as an "ok" thing to do. I mean, if you're not
going to allow unlinking of a sillyrenamed file, you may as well disallow
rename as well.
If that's not desirable (again, seems ok to me ... speaking as just a user)
then hey, in rename you just need to check the nfs_delete_queue.
> _If_ we want to be able to work with silly-renamed dentry, we need much
> more careful async unlink. Your current code assumes that these dentries
> won't go anywhere. AFAICS, dcache will not get into inconsistent state,
> but it will have very little to do with state of server...
OK, where else besides rename would the dentry change?
/fc
On Tue, Jun 10, 2003 at 07:43:33PM -0700, Frank Cusack wrote:
> On Wed, Jun 11, 2003 at 03:27:54AM +0100, [email protected] wrote:
> > The real problem is different: what happens if I take
> > silly-renamed file and rename it away? You suddenly get ->dir and
> > ->dentry if your nfs_unlinkdata having nothing to do with each other.
Wow, it's clear to me now :-) that this is another place I'm seeing NFS
problems.
> You could disallow rename if DCACHE_NFSFS_RENAMED is set. That would
...
> OK, where else besides rename would the dentry change?
I can answer this myself: link. (Is that correct?) Anywhere else?
/fc
>>>>> " " == viro <[email protected]> writes:
> Aliasing could be dealt with. They would have the same inode,
> so it's easy to detect.
I suppose we could... Is the procedure that nfs_lookup() should first
rehash the dropped dentry, then return it instead of NULL?
> The real problem is different: what happens if I take
> silly-renamed file and rename it away? You suddenly get ->dir
> and ->dentry if your nfs_unlinkdata having nothing to do with
> each other.
->dir is the important one here, as it provides the filehandle. We
only use ->dentry in order to give us a name/string for the
NFSPROC_REMOVE call.
> AFAICS, dcache will not get into inconsistent state, but it
> will have very little to do with state of server...
But that's the best we can do in any scenario. NFS does not ever give
you a guarantee that someone won't screw things up on the server, nor
does it give you any failsafe mechanism for detecting it.
That's why operation atomicity is such an issue with the current
kernel, and is why I'm so eager to push the intent patches into 2.6.
Sure sillyrename fails miserably in the atomicity department. There's
bugger all we can do about that: if you are suggesting we should rely
on some sort of revalidation before we unlink, then that's just as
race prone as what we have now.
Cheers,
Trond
On Tue, Jun 10, 2003 at 07:43:33PM -0700, Frank Cusack wrote:
> On Wed, Jun 11, 2003 at 03:27:54AM +0100, [email protected] wrote:
> > On Wed, Jun 11, 2003 at 03:59:10AM +0200, Trond Myklebust wrote:
> > > IOW we just want to prevent VFS from unhashing the dentry in the first
> > > place: dentry aliasing cannot work together with sillyrename.
> >
> > Aliasing could be dealt with. They would have the same inode, so it's
> > easy to detect.
>
> dentry only contains the inode, not the fh. On the server, the inode
> can go away and come back as a new fh, but with the same inode. Is
> that detectable (would comparison hooks have to be added?)? Although,
> I guess the inode is enough; you can do an NFS_I(inode)->fh to get
> the fh, but I wouldn't guess you'd want that in the VFS. Bah, here
> I go again ... forgive me if that's nonsense.
You wouldn't need to do that anywhere near VFS - all relevant code is
inside NFS and yes, there we _are_ allowed to look at ->fh ;-)
FWIW, we could probably simply do the following: have nfs_lookup()
return ERR_PTR(-EINVAL) if it notices that it's about to give us
such alias. IOW, no access to such guys at all - if it's going
to die, we refuse to do anything with it. I'll try to do that
variant when I get some sleep - I'd rather not mess with anything
in that area until I'm completely awake...
On Wed, 11 Jun 2003 [email protected] wrote:
>
> Two different dentries for the same file is obviously not a problem...
It _is_ a problem. It does the wrong thing on any subsequent directory
operation (move or unlink).
Multiple aliased dentries have never been ok, unless the filesystem
explicitly handles them and invalidates them (ie ntfs/fat kind of things).
Linus
On Jun 10, 2003 22:30 -0700, Linus Torvalds wrote:
> On Wed, 11 Jun 2003 [email protected] wrote:
> > Two different dentries for the same file is obviously not a problem...
>
> It _is_ a problem. It does the wrong thing on any subsequent directory
> operation (move or unlink).
>
> Multiple aliased dentries have never been ok, unless the filesystem
> explicitly handles them and invalidates them (ie ntfs/fat kind of things).
Amusingly, we hit _exactly_ this same problem a couple of days ago
in Lustre because we were trying to use RW locks instead of the single
directory i_sem to improve large (10M files) directory lookup performance,
and it is hard to hit and even harder to detect.
We've gone back to using i_sem to protect the actual dentry instantiation
in lookup for now (we have a separate DLM for doing RW locking of the
directory for create/rename/unlink), but will be looking at ways to allow
this in the filesystem again at some time in the future. Allowing RW
locking for local filesystem lookups would probably also be a win.
My 5-minute theory to fix this is to re-check the inode dentry alias list
when you are doing d_instantiate() (with dcache lock held) for another
dentry with the same hash (and further the same name if the hashes match),
but that would behave poorly when there are large numbers of links to a
single file. It _might_ be safe to check the first N entries on the
i_dentry list, where N ~= num_cpus, assuming that we could only have N
racy dentry lookups going at one time. Even so, the boost of allowing
multiple parallel lookups for huge directories probably beats out the
extra searching slowdown for the uncommon many-links-to-inode case.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/
On Wed, Jun 11, 2003 at 04:00:41AM +0100, [email protected] wrote:
> FWIW, we could probably simply do the following: have nfs_lookup()
> return ERR_PTR(-EINVAL) if it notices that it's about to give us
> such alias. IOW, no access to such guys at all - if it's going
> to die, we refuse to do anything with it. I'll try to do that
> variant when I get some sleep - I'd rather not mess with anything
> in that area until I'm completely awake...
Sounds ok to me, except that Linus says
On Tue, Jun 10, 2003 at 10:30:10PM -0700, Linus Torvalds wrote:
>
> On Wed, 11 Jun 2003 [email protected] wrote:
> >
> > Two different dentries for the same file is obviously not a problem...
>
> It _is_ a problem. It does the wrong thing on any subsequent directory
> operation (move or unlink).
>
> Multiple aliased dentries have never been ok, unless the filesystem
> explicitly handles them and invalidates them (ie ntfs/fat kind of things).
so anyway, please find attached a 2.4.21-rc7 and 2.5.70 patch which
prevents removal or rename of unlinked-but-open files. You can see
the rename bug by doing something like
mkdir d1 d2
hold a file open in d1 and rm it; it gets sillyrenamed
move sillyrenamed file to d2
rmdir d1
close file => "inode number mismatch" (data->dir isn't "live", ie it
doesn't follow the rename, and d1 is gone)
/fc
On Mer, 2003-06-11 at 06:30, Linus Torvalds wrote:
> Multiple aliased dentries have never been ok, unless the filesystem
> explicitly handles them and invalidates them (ie ntfs/fat kind of things).
For vfat at least its all broken.
cd foo
mv ../file .
more file
ESTALE.
On 11 Jun 2003, Alan Cox wrote:
>
> For vfat at least its all broken.
Looks like a different issue, not dentry aliasing per se.
> cd foo
> mv ../file .
> more file
>
> ESTALE.
Yes, VFAT ends up encoding the parent directory in the FH, so renaming
will invalidate the old file handle, and if you cache inodes (and thus
filehandles) over a directory move, badness happens.
Arguably it's a NFS client problem - the path revalidate at open time
should have caught the ESTALE and forced a new inode lookup. But I think
you can also argue that VFAT over NFS is just non-unixy enough that it
just isn't really "supported".
I think it's more of a "you can NFS-export strange filesystems for some
limited file sharing, but if things break, you get to keep both pieces".
Linus
On Mer, 2003-06-11 at 16:08, Linus Torvalds wrote:
> > cd foo
> > mv ../file .
> > more file
> >
> > ESTALE.
>
> Yes, VFAT ends up encoding the parent directory in the FH, so renaming
> will invalidate the old file handle, and if you cache inodes (and thus
> filehandles) over a directory move, badness happens.
>
> Arguably it's a NFS client problem
No no - this happens on LOCAL disk. No NFS needed at all.
On 11 Jun 2003, Alan Cox wrote:
>
> No no - this happens on LOCAL disk. No NFS needed at all.
How do you make a local filesystem return ESTALE? A quick grep shows it in
fs/fat/inode.c, but it should only be in the "export" functions used to
export it for NFS.
Curious. What am I missing?
Linus
On Mer, 2003-06-11 at 17:11, Linus Torvalds wrote:
> On 11 Jun 2003, Alan Cox wrote:
> >
> > No no - this happens on LOCAL disk. No NFS needed at all.
>
> How do you make a local filesystem return ESTALE? A quick grep shows it in
> fs/fat/inode.c, but it should only be in the "export" functions used to
> export it for NFS.
>
> Curious. What am I missing?
fs/vfat - d_revalidate: vfat_revalidate
On 11 Jun 2003, Alan Cox wrote:
>
> fs/vfat - d_revalidate: vfat_revalidate
That still shouldn't cause ESTALE, it should just force a dropping of the
dentry, and a re-lookup (and that, in turn, should either get the right
thing, or should return ENOENT).
Or are you talking about 2.4.x and that is doing something strange these
days?
[ You have entered the twilight zone: "Tee-dee tee-dee.." ]
Linus
On Wed, Jun 11, 2003 at 09:31:32AM -0700, Linus Torvalds wrote:
>
> On 11 Jun 2003, Alan Cox wrote:
> >
> > fs/vfat - d_revalidate: vfat_revalidate
>
> That still shouldn't cause ESTALE, it should just force a dropping of the
> dentry, and a re-lookup (and that, in turn, should either get the right
> thing, or should return ENOENT).
.... and vfat lookup will pick what can be picked. We do not get hashed
aliases there.
On Mer, 2003-06-11 at 17:31, Linus Torvalds wrote:
> On 11 Jun 2003, Alan Cox wrote:
> >
> > fs/vfat - d_revalidate: vfat_revalidate
>
> That still shouldn't cause ESTALE, it should just force a dropping of the
> dentry, and a re-lookup (and that, in turn, should either get the right
> thing, or should return ENOENT).
>
> Or are you talking about 2.4.x and that is doing something strange these
> days?
>
> [ You have entered the twilight zone: "Tee-dee tee-dee.." ]
I've seen it on early 2.5 and on 2.4, current 2.5.x seems to be ok from
a quick test.
>>>>> " " == Alan Cox <[email protected]> writes:
> I've seen it on early 2.5 and on 2.4, current 2.5.x seems to be
> ok from a quick test.
2.4 has the 'return ESTALE if current dir fails d_revalidate()'
test. Looks like the vfat stuff has the same problem that
Sigh... Can we perhaps add FS_ALWAYS_REVAL in order to flag what kind
of behaviour filesystems expect in link_path_walk()? NFS needs
revalidation on all open() calls (including opendir(".")), so removing
the ESTALE code is not an option.
Cheers,
Trond
>>>>> " " == Trond Myklebust <[email protected]> writes:
> 2.4 has the 'return ESTALE if current dir fails d_revalidate()'
> test. Looks like the vfat stuff has the same problem that
I should learn to complete my own sentences before sending... The
above should read:
Looks like the vfat stuff has the same problem that Coda did. It is
unintentionally triggering the ESTALE code, as it assumes that
d_revalidate() is advisory only.
Cheers,
Trond
On Wed, Jun 11, 2003 at 08:26:52PM +0200, Trond Myklebust wrote:
> diff -u --recursive --new-file linux-2.4.21-rc6/fs/namei.c linux/fs/namei.c
> --- linux-2.4.21-rc6/fs/namei.c 2002-12-30 09:39:54.000000000 -0800
> +++ linux/fs/namei.c 2003-06-11 11:16:49.000000000 -0700
> @@ -633,7 +633,8 @@
> * Check the cached dentry for staleness.
> */
> dentry = nd->dentry;
> - if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
Doesn't the above simply always revalidate?
> + if (dentry && dentry->d_sb
> + && (dentry->d_sb->s_type->fs_flags & FS_ALWAYS_REVAL)) {
> err = -ESTALE;
> if (!dentry->d_op->d_revalidate(dentry, 0)) {
> d_invalidate(dentry);
/fc
>>>>> " " == Frank Cusack <[email protected]> writes:
> On Wed, Jun 11, 2003 at 08:26:52PM +0200, Trond Myklebust
> wrote:
>> diff -u --recursive --new-file linux-2.4.21-rc6/fs/namei.c
>> linux/fs/namei.c
>> --- linux-2.4.21-rc6/fs/namei.c 2002-12-30 09:39:54.000000000
>> -0800
>> +++ linux/fs/namei.c 2003-06-11 11:16:49.000000000 -0700
>> @@ -633,7 +633,8 @@
>> * Check the cached dentry for staleness.
>> */ dentry = nd->dentry;
>> - if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
> Doesn't the above simply always revalidate?
That's the whole problem in a nutshell. Read the thread...
Cheers,
Trond
On Thu, Jun 12, 2003 at 01:16:15AM +0200, Trond Myklebust wrote:
> >>>>> " " == Frank Cusack <[email protected]> writes:
> > Doesn't the above simply always revalidate?
>
> That's the whole problem in a nutshell. Read the thread...
Oh, I thought you were fixing an NFS problem with that patch.
/fc
On Wed, Jun 11, 2003 at 07:47:24PM +0200, Trond Myklebust wrote:
> >>>>> " " == Trond Myklebust <[email protected]> writes:
>
> > 2.4 has the 'return ESTALE if current dir fails d_revalidate()'
> > test. Looks like the vfat stuff has the same problem that
>
> I should learn to complete my own sentences before sending... The
> above should read:
>
> Looks like the vfat stuff has the same problem that Coda did. It is
> unintentionally triggering the ESTALE code, as it assumes that
> d_revalidate() is advisory only.
Coda still has the problem with 2.4. The only thing I have been telling
people that hit the problem is to take the revalidate patch out.
btw. The sheer number of problem cases is already reduced significantly
by the following patch which avoids calling revalidate on every name
that happens to start with a '.'.
Jan
diff -urN --exclude-from=dontdiff linux-2.4.21-rc2/fs/namei.c linux-2.4.21-rc2-coda/fs/namei.c
--- linux-2.4.21-rc2/fs/namei.c 2003-05-09 02:20:44.000000000 -0400
+++ linux-2.4.21-rc2-coda/fs/namei.c 2003-05-14 02:23:07.000000000 -0400
@@ -627,6 +627,8 @@
nd->last_type = LAST_DOT;
else if (this.len == 2 && this.name[1] == '.')
nd->last_type = LAST_DOTDOT;
+ else
+ goto return_base;
return_reval:
/*
* We bypassed the ordinary revalidation routines.
On Wed, Jun 11, 2003 at 12:22:26AM -0700, Frank Cusack wrote:
> so anyway, please find attached a 2.4.21-rc7 and 2.5.70 patch which
> prevents removal or rename of unlinked-but-open files.
Minor adjustment. It's functionally the same but prevents a spurious
printk (fs/nfs/dir.c:1199 in 2.5.70).
Works for me.
/fc