2.4.19-pre8-jp12

Linux kernel patch set 2.4.19pre8-jp12

This is the twelvth release of the -jp patch set.

http://infolinux.de/jp12/

Status: 15 May 2002 00:45 CEST

Changes from jp11 to jp12

Updates: XFS update to CVS as of 13 May 2002, new IDE convert all 10, Alsa compile fix, IDE CD DMA upgrade. New patches included in this
version: Momchil Velikov/Christoph Hellwig's radix tree pagecache (adapted to work with rmap/preempt/lockbreak and XFS), Jens Axboe's IDE tagged command queue support, Robert Love's latest additional miscellanous scheduler patches.

Known Issues

The miroSound PCM20 radio audio driver depends on OSS sound include file and is no longer compileable. IDE CD should be fixed. I did not find the bug but it could be in the IDE patch, or some interaction with it I did not understand. Anyway, cdfs does not work, it will mount, but oops when stat()ing a directory. The drive will be locked until reboot. This is caused by an interaction between the new code in IDE, supermount, and cdfs and must be investigated.

What is it?

The -jp kernels are development kernels for testing purpose only. They will appear regularly two or three times a month. Their purpose is to provide a service for developers who can't keep up to date with the latest kernel and patch versions, but want to test new features and evaluate enhancements that are not to be expected for inclusion into the mainstream 2.4 kernel.

Download

http://infolinux.de/jp12/patch-2.4.19-pre8-jp12.tar.bz2

Patch set overview

01_kernel-sound-remove-0-pre8  34_llseek
01_kernel-sound-remove-1-pre7  35_mount
01_kernel-sound-remove-2-pre6  36_device
01_kernel-sound-remove-3-pre5  37_supermount
01_kernel-sound-remove-4-pre4  38_xfs-kdb-from-cvs-04May2002
01_kernel-sound-remove-5-pre3  39_xfs-13May2002
01_kernel-sound-remove-6-pre2  40_xfs-kdb-adapt
01_kernel-sound-remove-7-core  41_xfs-kdb-fixups
02_alsa-sound-0.9.0rc1         42_jfs-1.0.14-0
03_alsa-adapt                  43_jfs-1.0.14-15
04_TIOCGDEV                    44_jfs-1.0.15-16
05_boot-time-ioremap           45_jfs-1.0.16-17
06_via-northbridge-fixup       46_jfs-adapt
07_jiffies-for-i386            47_ftpfs-0.6.2
08_rmap-13                     48_cdfs-0.5b
09_rmap13a                     49_cdfs-0.5bc
10_sched-O1-K3                 50_patch-int-2.4.18.2
11_up-apic-fix                 51_loop-jari
12_remove-wake-up-sync         52_grsecurity-1.9.5-pre3
13_need-resched-abstraction    53_grsecurity-adapt
14_frozen-lock                 54_i2c-2.6.3
15_sched-yield                 55_lmsensors-2.6.3
16_more-sched-yield            56_freeswan-1.97
17_need-resched-check          57_ide-cd-dma-4
18_maxrtprio-1                 58_lowlatency-mini
19_maxrtprio                   59_lowlatency-fixes-5
20_migration-thread            60_mmx-init
21_updated-migration-init      61_p4-xeon
22_misc-stuff                  62_x86-fast-pte
23_preempt-2.4.19pre8          63_acpi-20020503
24_lockbreak                   64_acpi-pciirq-17
25_ide-all-convert-10          65_remove-khttpd
26_md-locks                    66_tux2-final-A3
27_raid-split                  67_sis-740-961
28_md-part                     68_radix-tree-pagecache-2.4.19pre5ac3
29_mdp-major                   69_radix-tree-pagecache-xfs-fix
30_autofs4                     70_block-tag-2.4.19pre8
31_isrdonly                    71_ide-tag-2.4.19pre8
32_new-stat                    98_tkparse-4096
33_mediactl                    99_VERSION

More info available at

http://infolinux.de/jp12

Jörg Prante
[email protected]

I tried it. First I had a lot of compile problems (everything as module), I had to disable at least 5 drivers (sorry I don't know exactly which were them).
After compiling, depmod -a says:

unresolved symbols in
/drivers/char/drm/sis.o
/drivers/hotplug/pcihpacpi.o
/net/tux/tux.o

So even tux is unusable :(.

But the worst problem for is supermount:

# mount -t supermount -o dev=/dev/cdrom none /mnt/cdrom
# ls -l /mnt/cdrom
ls: .: Stale NFS handle                (~or something similar)
[...]                                  (and it lists the file)

But:

# cd /mnt/cdrom
# ls -l
ls: .: Stale NFS handle
#                                      (and it doesn't even list the files)

Balazs Pozsar

Hi Pozsar, can you please give me some hints what the module names are. I could add a note to the homepage, or even fix the issue.

No it's not unusuable. You have to compile it into the kernel. But thank you for the info. I try to reproduce the building weaknesses.

Jörg

Hi Pozsar, this is a known supermount bug. I got this as well. Supermount is a patch for 2.4.18pre8 kernel, not for 2.4.19preX. See also

http://www.mandrake.com/en/archives/cooker/2002-04/msg00086.php

I will try my best to figure out what is happening. The open(".",...) call to supermount returns -1 and this gives "stale NFS handle", but open("/mnt/cdrom", ...) gives "3" or a positive number. I think the text of the message is misleading, it has nothing to do with NFS.

Jörg

Hmmm.. I've been seeing this problem in the latest -ac kernels too.

Basically, a while after mounting the CD a ls on any subdir of the mount will complain about a 'stale NFS handle' and the device has to be remounted.

T.

I thougth it wasn't Jorg's fault :). Unfortunately I immediately get this message :(.

Balazs Pozsar

I'm getting the same with ftpfs, both in jp11 and jp12. Remounting doesn't change a thing, it just shows me the root tree, I can cd into directories if I know the name, but all I got is those nice 'stale NFS handle' as a response from ls

Regards

Rob.

Hi!

I traced it down. The trouble exists since 2.4.19-pre4. Trond Myklebust touched the VFS in order to send dentry revalidation events to NFS.

http://www.geocrawler.com/archives/3/789/2002/3/100/8078826/

But Trond's patch conflicts with almost all non-standard virtual or remote mount file systems (supermount, cdfs, ftpfs, and maybe autofs). I don't know if the cdfs oops I observed now for a while is related.

Is it possible to leave the VFS layer untouched? Or restrict the dentry revalidation to NFS and let other remote file systems coexist, i.e. without revalidation calls?

Or is it recommended to write fixes for the file systems stated above, because they now have wrong assumptions about the VFS behavior?

Anyway, while these questions arise, I request to remove Trond's patch since the patch changes too much for 2.4 stable kernel series.

Attached is a revert patch against 2.4.19-pre8 for Marcelo.

Thanks

Jörg

diff -urN linux-2.4.19-pre3/fs/namei.c linux-/fs/namei.c
@@ -457,7 +457,7 @@
 	while (*name=='/')
 		name++;
 	if (!*name)
+		goto return_base;
-		goto return_reval;
 
 	inode = nd->dentry->d_inode;
 	if (current->link_count)

@@ -576,7 +576,7 @@
 				inode = nd->dentry->d_inode;
 				/* fallthrough */
 			case 1:
+				goto return_base;
-				goto return_reval;
 		}
 		if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
 			err = nd->dentry->d_op->d_hash(nd->dentry, &this);

@@ -627,19 +627,6 @@
 			nd->last_type = LAST_DOT;
 		else if (this.len == 2 && this.name[1] == '.')
 			nd->last_type = LAST_DOTDOT;
-return_reval:
-		/*
-		 * We bypassed the ordinary revalidation routines.
-		 * Check the cached dentry for staleness.
-		 */
-		dentry = nd->dentry;
-		if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
-			err = -ESTALE;
-			if (!dentry->d_op->d_revalidate(dentry, 0)) {
-				d_invalidate(dentry);
-				break;
-			}
-		}
 return_base:
 		return 0;
 out_dput:

Really the other file systems want fixing - that revalidation is a real bug fix and the situation could occur for other network file systems too

Hi,

in 2.4.19pre4, the dentry revalidation has been patched, and the VFS behavior has changed due to an NFS bugfix. All non-standard remote file systems (like supermount, ftpfs, etc.) are in conflict with the patch and may not operate correctly.

Alan Cox pointed out that non-standard remote file systems should get fixed for 2.4.19. So, here is my quick hack for supermount 0.7 that changes the return policy in the d_revalidate operation. Supermount should work now in 2.4.19pre4 kernels and higher.

Other file systems with similar revalidate policy can get fixed accordingly.

Cheers,

Jörg

@@ -25,8 +25,9 @@
 
 	dump_dentry(dentry);
 
-	if (!subfs_go_online(dentry->d_sb))
-		goto bad_dentry;
+	if (!subfs_go_online(dentry->d_sb)) {
+		goto out;
+    }
 	spin_lock(&dcache_lock);
 	if (dentry->d_inode && is_inode_obsolete(dentry->d_inode)) {
 		supermount_debug("found old dentry: **%s**",

@@ -36,20 +37,21 @@
 	}
 	spin_unlock(&dcache_lock);
 	subd = get_subfs_dentry(dentry);
-	if (IS_ERR(subd))
+	if (IS_ERR(subd)) {
 		goto bad_dentry;
+	}
 
 	if (subd->d_op && subd->d_op->d_revalidate) {
 		supermount_debug("lowlevel revalidate");
 		rc = subd->d_op->d_revalidate(subd, flags);
 	}
 	dput(subd);
+	goto out;
+bad_dentry:
+	rc = 0;
 out:
 	subfs_go_inactive(dentry->d_sb);
 	return rc;
-bad_dentry:
-	rc = 0;
-	goto out;
 }
 
 struct dentry_operations supermount_dir_dops = {

I was able to reproduce the bug on a reiserfs root on one of my machines today.

T.

And I thought I broke something with my latest changes in Coda. This 'bugfix' is hitting us hard. In some cases we hand down quite volatile objects, files that are involved in a conflict, fake expanded directory trees during the repair/examination of such conflicts.

These object are passed down with a 'no-cache' flag, which uses dentry_revalidate to skip the cached lookup from the dcache but forces all lookups to be passed through to the Coda filesystem. This bugfix causes breakage, instead of forcing a new filesystem lookup, the VFS simply returns ESTALE.

AFAIK, before the fix, failing dentry revalidate meant 'the lookup path in the dcache is probably invalid, please double check with the filesystem'. And now it means, 'the lookup path is invalid, return failure'.

Jan

Hi,

I'm pulling up this old thread again, because there are some serious issues with a patch that was merged in 2.4.19-pre3.

Basically it boils down to this, NFS had problems with '.' and '..' lookups as they were bypassing the dcache revalidation/lookup. However, the patch that got merged in 2.4.19-pre3 to fix this is causing some significant problems, it changed the semantics of d_revalidate.

Originally, returning a error from d_revalidate in cached_lookup would force a subsequent real_lookup. After the patch, returning an error from d_revalidate propagates (in some cases) ESTALE back up to userspace.

NFS seems to do it's own replacement for real_lookup in dentry_revalidate, but most other filesystems (like Samba) 'fixed' the problem by simply revalidating the cached attributes of the object with the server. This ofcourse breaks when an object is simply renamed, as the revalidation will reinstantiate the old and incorrect lookup path for the object in the dcache.

Now either every filesystem will have to follow NFS's lead and implement some form of real_lookup inside of the dentry_revalidate function which is not the prettiest solution. Or the VFS patch should be reverted/fixed to actually walk the tree for '.' and '..' lookups.

btw. the patch also leaks dentries when a 'stale dentry' happens to have children because it doesn't properly check and handle the d_invalidate returncode.

Jan

For reference here is the thread from last May when Joerg Prante tracked down his problems in supermount to the NFS patch.

    http://marc.theaimsgroup.com/?l=linux-kernel&m=102158542315351&w=2

Here is the 'controversial patch', cut-n-pasted out from linux.bkbits.net.

@@ -457,7 +457,7 @@
    while (*name=='/')

	name++;
    if (!*name)

- goto return_base;
+ goto return_reval;

    inode = nd->dentry->d_inode;
    if (current->link_count)
@@ -576,7 +576,7 @@
		inode = nd->dentry->d_inode;
		/* fallthrough */
	    case 1:
-				goto return_base;
+				goto return_reval;
	}
	if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
	    err = nd->dentry->d_op->d_hash(nd->dentry, &this);
@@ -627,6 +627,19 @@
	    nd->last_type = LAST_DOT;
	else if (this.len == 2 && this.name[1] == '.')
	    nd->last_type = LAST_DOTDOT;

+return_reval:
+ /*
+ * We bypassed the ordinary revalidation routines.
+ * Check the cached dentry for staleness.
+ */
+ dentry = nd->dentry;
+ if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+ err = -ESTALE;
+ if (!dentry->d_op->d_revalidate(dentry, 0)) {
+ d_invalidate(dentry);
+ break;
+ }
+ }
return_base:

	return 0;

out_dput:

> Originally, returning a error from d_revalidate in cached_lookup would > force a subsequent real_lookup. After the patch, returning an error from > d_revalidate propagates (in some cases) ESTALE back up to
> userspace.

Which is the whole point of the patch. If you are trying to read or modify a directory that is invalid, you need to be notified of that.

> Now either every filesystem will have to follow NFS's lead and
^^^^^
The vast majority of filesystems are coping fine as they are.

> implement some form of real_lookup inside of the
> dentry_revalidate function which is not the prettiest > solution. Or the VFS patch should be reverted/fixed to actually > walk the tree for '.' and '..' lookups.

The whole problem was that we did *not* walk the tree for '.' and '.. 'lookups. Reverting the VFS patch simply results in us stupidly reusing current->fs->pwd, dentry->d_parent and friends without checking if it is safe to do so.

> btw. the patch also leaks dentries when a 'stale dentry' > happens to have children because it doesn't properly check and > handle the d_invalidate returncode.

Why? d_invalidate() doesn't dget() anything, and if you are doing 'open(".")' then it will return -EBUSY anyway. What are you going to do about it?

Cheers,
Trond

Yes, by failing in real_lookup, not randomly crapping out with ESTALE.

Right, well only network filesystems could possibly be affected as on-disk filesystems don't have any reason why the cached path would suddenly become invalid, so let's look at network filesytems,

- Coda definitely doesn't cope just fine.
- smbfs, calls into revalidate_inode instead of retrying the lookup. Broken.
- ncpfs, dentry->d_parent->d_inode. Hmm, probably breaks when revalidating the root of the mounted fs?
- intermezzo, revalidates data and attributes of the object, same broken behaviour as smbfs.
- NFS works!

Were there any other network filesystems in the kernel? So we've already got 4 out of 5 broken.

Now let's look at out of kernel network and virtual filesystems,
- OpenAFS, guess they're broken.
- Arla, guess what :)

- Supermount, well that is the guy who traced where the strange ESTALE problems came from, he worked on a patch that probably ended up working because I guess nothing can suddenly get renamed from underneath him.
- CDFS, same deal.

Sorry it should have been 'reverted, or fixed to actually revalidate all entries on the cached tree leading up to '.' and '..'.

/**
* d_invalidate - invalidate a dentry
* @dentry: dentry to invalidate
*
* Try to invalidate the dentry if it turns out to be
* possible. If there are other dentries that can be
* reached through this one we can't delete it and we
* return -EBUSY. On success we return 0.
*
* no dcache lock.
*/

It's the missing 'd_put' that is the problem. The code should probably look like all other places where d_invalidate is called, i.e.

    if (!dentry->d_op->d_revalidate(dentry, flags)) {

- d_invalidate(dentry);
+ if (!d_invalidate(dentry)) {
+ dput(dentry);
+ dentry = NULL;
+ }

	break;
    }

But why would I submit a patch to fix something that I believe is fundamentally broken in the first place. I agree about the problem, I just disagree about the fix that was applied.

And I actually thought it had already been reverted last May.

Jan

> Yes, by failing in real_lookup, not randomly crapping out with > ESTALE.

You are completely missing the point: open("."); never did call 'real_lookup', and POSIX compatibility say that it shouldn't (see example below).

> Sorry it should have been 'reverted, or fixed to actually > revalidate all entries on the cached tree leading up to '.' and > '..'.

No.

Client Server
------ --------
cd foo
mv foo bar
open(".")

You are basically saying that you believe that the above scenario must always fail and that the VFS should enforce a violation of POSIX rules. The current code has the possibility to recover from the above sort of thing: this will not be the case if you have to look up 'foo' on the server in order to do open(".")

     > /**
     > * d_invalidate - invalidate a dentry
     > * @dentry: dentry to invalidate
     >  *
     > * Try to invalidate the dentry if it turns out to be
     > * possible. If there are other dentries that can be
     > * reached through this one we can't delete it and we
     > * return -EBUSY. On success we return 0.
     >  *
     > * no dcache lock.
     >  */

     > It's the missing 'd_put' that is the problem. The code should
     > probably look like all other places where d_invalidate is
     > called, i.e.

     >     if (!dentry->d_op->d_revalidate(dentry, flags)) {
     > - d_invalidate(dentry);
     > + if (!d_invalidate(dentry)) {
     > + dput(dentry);
     > + dentry = NULL;
     > + }
     > 	break;
     >     }

So? That's just because those lines usually lie just after a cached_lookup(), which bumps the count. Read the code in question: it is not leaking dentries.

Cheers,
Trond

I must be blind, because nfs_lookup_revalidate does,

    dir = dentry->d_parent->d_inode;
    ...
    error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
    if (error)
	goto out_bad;

So how is this recovering any better from the scenario you just gave?

Ah, got confused by the dput in the out_dput path, must have missed the return 0; right above it.

Jan

     > I must be blind, because nfs_lookup_revalidate does,

     >     dir = dentry->d_parent->d_inode; ...  error =
     >     NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle,
     >     &fattr); if (error)
     > 	goto out_bad;

     > So how is this recovering any better from the scenario you just
     > gave?

I agree that is a bug (and thanks for pointing it out). I do not, however, see it as an argument for turning off revalidation altogether.
Perhaps the right thing to do then is to pass down a flag (LOOKUP_DOT?) that states that we are in fact doing a revalidation of '.' and/or '..' ?

Cheers,
Trond

That would help me a lot as I can then reliably recognize when d_revalidate is called from the new 'context' and try to appropriately handle this case.

Let me get the POSIX stuff right, In the case of open('.') in your example, I guess d_revalidate would check the inode to see if the object was removed. And possibly retry the lookup, but if that fails only unhash the dentry, not return a 'validation failure'.

So, we're not really revalidating the dcache entry at all. Maybe the code really wants to revalidate the inode.

Jan

> So, we're not really revalidating the dcache entry at > all. Maybe the code really wants to revalidate the inode.

That is indeed the correct thing to do here according to POSIX, and is really all I want to do for NFS too. For that reason, I originally proposed to use i_op->revalidate(), but that was vetoed as you may recall.

Cheers,
Trond

It was? I really hope I wasn't one of the veto-ers because looking at this problem it does seem like the correct thing to do as we're not trying to see whether the path is correct, but only the object.

Ok, it will cost Coda a whole upcall/context switch, but as it is '.' we're talking about it should already be locally cached.

btw. you can easily drop the lookup in nfs_lookup_revalidate. It is a bug for this case, and not necessary in all other places d_revalidate is called as the VFS already uses real_lookup in those cases.

Jan

Haven't tried to compile it yet, but I guess it would look something like this?

diff -urN linux-2.4.19/fs/namei.c linux-2.4.19-revalidate/fs/namei.c
@@ -633,9 +633,12 @@
 		 * Check the cached dentry for staleness.
 		 */
 		dentry = nd->dentry;
-		if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+		if (dentry) {
 			err = -ESTALE;
-			if (!dentry->d_op->d_revalidate(dentry, 0)) {
+			if (!dentry->d_inode || is_bad_inode(dentry->d_inode) ||
+			    (dentry->d_inode->i_op && 
+			     dentry->d_inode->i_op->revalidate &&
+			    !dentry->d_inode->i_op->revalidate(dentry))) {
 				d_invalidate(dentry);
 				break;
 			}

> It was? I really hope I wasn't one of the veto-ers because > looking at this problem it does seem like the correct thing to > do as we're not trying to see whether the path is correct, but > only the object.

Al was the main vetoer. His objection to i_op->revalidate() was that it is interpreted differently for NFS, smbfs, coda,... For some it just works like getattr(), for others it actually does an inode revalidation...

> Ok, it will cost Coda a whole upcall/context switch, but as it > is '.' we're talking about it should already be locally > cached.

But if all you want to do is read the cache, you'd be better off with d_revalidate(dentry,LOOKUP_DOT) / d_revalidate(dentry, LOOKUP_DOTDOT). Wouldn't that allow you to drop the upcall+context switch too?

> btw. you can easily drop the lookup in nfs_lookup_revalidate. > It is a bug for this case, and not necessary in all other > places d_revalidate is called as the VFS already uses > real_lookup in those cases.

True, but that would be at the expense of increased dentry aliasing.

Normally that is not a problem (and so we do tolerate it if operations on the server are the cause), but under NFS we have to play tricks to get around the problem of somebody doing 'unlink("file")' while we are keeping the same file open for read()/write(). Basically we rename the file to '.nfsxxxxx', and keep it until the last process to call close() can do the actual unlink() rpc call to the server. The resulting 'sillyrename()' feature (which is a recommended implementation for all NFS clients) is intolerant of dentry/filename aliasing.

Cheers,
Trond

Actually, as long as Coda hasn't received a callback from userspace indicating that the object has changed, it doesn't even make the getattr upcall. In fact I would probably end up with coda_dentry_revalidate calling coda_inode_revalidate in either of these cases. So why not just use iops->revalidate in the first place.

As far as I can see, d_revalidate should just be used to check whether a cached directory path might have been changed (i.e. forces a real_lookup). While iops->revalidate checks the validity of the object.

It also looks like 2.5 doesn't have this patch, so I'm assuming a change like this will be made to that tree at some point. And I'd rather see a variant that makes sense and doesn't change existing semantics, even when it might be less efficient for some filesystems. As long as it remains definitely correct.

Especially the changed semantics of d_revalidate is what troubles me,

- If I do the test on the object, I might 'reinstantiate' a possibly bad lookup path in the dcache because we revalidate the 'path' based on the object's state.
- When I test the path, we return ESTALE whenever the path has changed due to rename, or on 'volatile objects' (Coda specific, mountpoints, objects under repair, etc.).

So neither test ends up being correct. But if we use dentry_revalidate and inode_revalidate in the correct places it is immediately clear _what_ we're testing, the path leading up to the object or the object itself.

Jan

What is the unresolved symbol here? acpi_walk_something_or_other?

Do you really want to use this driver (i.e. do you have a machine that has a ACPI PCI Hotplug controller?)

thanks,

greg k-h