[PATCHSET] 2.4.19-pre8-jp12

[PATCHSET] 2.4.19-pre8-jp12

[Jörg Prante]

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 
joerg@infolinux.de 

Re: [PATCHSET] 2.4.19-pre8-jp12

[Pozsar Balazs]

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)



On Wed, 15 May 2002, [iso-8859-15] Jörg Prante wrote:

> 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
> joerg@infolinux.de
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

-- 
Balazs Pozsar

Re: [PATCHSET] 2.4.19-pre8-jp12

[Tomas Szepe]

> 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)

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.

Re: [PATCHSET] 2.4.19-pre8-jp12

[Pozsar Balazs]

> > 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)
>
> 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.

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

-- 
Balazs Pozsar

Re: [PATCHSET] 2.4.19-pre8-jp12

[Robinson Maureira Castillo]

On Thu, 16 May 2002, Tomas Szepe wrote:

> > 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)
> 
> 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'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.

Re: [PATCHSET] 2.4.19-pre8-jp12

[Jörg Prante]

> 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).

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.

> So even tux is unusable :(.

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

Re: [PATCHSET] 2.4.19-pre8-jp12

[Jörg Prante]

> 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)

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

[PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jörg Prante]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]


Hi!

Robinson Maureira Castillo <rmaureira@alumno.inacap.cl> wrote:
> On Thu, 16 May 2002, Tomas Szepe wrote:
> > > 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)
> >
> > 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'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

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

[-- Attachment #2: remove-NFS-close-to-open.patch --]
[-- Type: text/x-diff, Size: 1086 bytes --]

diff -urN linux-2.4.19-pre3/fs/namei.c linux-/fs/namei.c
--- linux-2.4.19-pre3/fs/namei.c	Wed Mar 20 14:03:50 2002
+++ linux/fs/namei.c	Wed Mar 20 14:04:40 2002
@@ -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:

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Alan Cox]

> 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? 

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

[PATCH] fixing supermount for > 2.4.19pre4

[Jörg Prante]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

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

[-- Attachment #2: supermount-2.4.19pre8-fix.patch --]
[-- Type: text/x-diff, Size: 936 bytes --]

--- linux/fs/supermount/dentry_operations.c.orig	Fri May 17 00:36:21 2002
+++ linux/fs/supermount/dentry_operations.c	Fri May 17 01:16:06 2002
@@ -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 = {

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Tomas Szepe]

> > 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? 
> 
> 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

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

T.

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

On Thu, May 16, 2002 at 11:13:01PM +0100, Alan Cox wrote:
> > 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? 
> 
> 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

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

Re: [PATCHSET] 2.4.19-pre8-jp12

[Greg KH]

On Thu, May 16, 2002 at 09:47:14AM +0200, Pozsar Balazs wrote:
> 
> 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/hotplug/pcihpacpi.o

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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

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.

--- 1.19/fs/namei.c	Thu Feb 28 05:57:29 2002
+++ 1.20/fs/namei.c	Tue Mar 12 07:35:02 2002
@@ -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:

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Trond Myklebust]

>>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:

     > 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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

On Thu, Oct 17, 2002 at 11:48:27PM +0200, Trond Myklebust wrote:
> >>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:
>      > 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.

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

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

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.

>      > 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.

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

>      > 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?

/**
 * 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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Trond Myklebust]

>>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:


    >> 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.

     > 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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

On Fri, Oct 18, 2002 at 01:57:17AM +0200, Trond Myklebust wrote:
> 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(".")

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?

> 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.

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

Jan

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Trond Myklebust]

>>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:

     > 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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

On Fri, Oct 18, 2002 at 07:03:27PM +0200, Trond Myklebust wrote:
> 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 '..' ?

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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Trond Myklebust]

>>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:

     > 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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

On Fri, Oct 18, 2002 at 07:41:40PM +0200, Trond Myklebust wrote:
> >>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:
> 
>      > 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.

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
--- linux-2.4.19/fs/namei.c	2002-08-28 01:07:35.000000000 -0400
+++ linux-2.4.19-revalidate/fs/namei.c	2002-10-18 14:21:13.000000000 -0400
@@ -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;
 			}

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Trond Myklebust]

>>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:

     > 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

Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes]

On Fri, Oct 18, 2002 at 09:23:59PM +0200, Trond Myklebust wrote:
>      > 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?

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