vfs-scale, keep the errno

vfs-scale, keep the errno

[J. R. Okajima]

When open(2) without O_DIRECTORY opens an existing dir, it should return
EISDIR. In do_last(), the variable 'error' is initialized EISDIR, but it
is changed by d_revalidate() which returns any positive to represent
'the target dir is valid.'
Should we keep and return the initialized 'error' in this case.


J. R. Okajima

diff --git a/fs/namei.c b/fs/namei.c
index 5bb7588..26fa823 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2102,17 +2118,20 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	struct file *filp;
 	int error = -EISDIR;
 
 	switch (nd->last_type) {
 	case LAST_DOTDOT:
 		follow_dotdot(nd);
 		dir = nd->path.dentry;
 	case LAST_DOT:
 		if (need_reval_dot(dir)) {
-			error = d_revalidate(nd->path.dentry, nd);
-			if (!error)
+			int e;
+			e = d_revalidate(nd->path.dentry, nd);
+			if (!e)
 				error = -ESTALE;
-			if (error < 0)
+			if (e < 0) {
+				error = e;
 				goto exit;
+			}
 		}
 		/* fallthrough */
 	case LAST_ROOT:

Re: vfs-scale, keep the errno

[Nick Piggin]

On Fri, Jan 14, 2011 at 2:38 PM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>
> When open(2) without O_DIRECTORY opens an existing dir, it should return
> EISDIR. In do_last(), the variable 'error' is initialized EISDIR, but it
> is changed by d_revalidate() which returns any positive to represent
> 'the target dir is valid.'
> Should we keep and return the initialized 'error' in this case.

Great, thank you very much. I just changed a few variable names but
applied it. Would you please add you Signed-off-by: on patches from
now on?

Good reviewing work. Are you finding these by review or testing? I
think ltptests on an nfs mount should find this kind of bug... which I
should do.

Thanks,
Nick

Re: vfs-scale, keep the errno

[J. R. Okajima]

Nick Piggin:
> Great, thank you very much. I just changed a few variable names but
> applied it. Would you please add you Signed-off-by: on patches from
> now on?

Yes, of course.
I am just not confident, and guessed you may take another approach.


> Good reviewing work. Are you finding these by review or testing? I
> think ltptests on an nfs mount should find this kind of bug... which I
> should do.

First, review. And then modify/test the filesystem which I am developing
(FS_REVAL_DOT is used).
Some of my mails are from the reviewing and modifying phase.
I am in my testing phase. Someday my fs ->d_revalidate will be gone, I
hope.

Anyway your vfs-scale work is great which you definetly spent big time
and effort I believe.


J. R. Okajima

Re: vfs-scale, keep the errno

[Hans-Peter Jansen]

On Friday 14 January 2011, 05:01:37 Nick Piggin wrote:
> On Fri, Jan 14, 2011 at 2:38 PM, J. R. Okajima <hooanon05@yahoo.co.jp> 
wrote:
> > When open(2) without O_DIRECTORY opens an existing dir, it should
> > return EISDIR. In do_last(), the variable 'error' is initialized
> > EISDIR, but it is changed by d_revalidate() which returns any
> > positive to represent 'the target dir is valid.'
> > Should we keep and return the initialized 'error' in this case.
>
> Great, thank you very much. I just changed a few variable names but
> applied it. Would you please add you Signed-off-by: on patches from
> now on?
>
> Good reviewing work. Are you finding these by review or testing? I
> think ltptests on an nfs mount should find this kind of bug... which
> I should do.

Nick, Junjiro is well known as a very smart (and|but) humble guy in the 
kernel niche of layered filesystems. Be assured, that almost every live 
distribution (one, that is runnable from RO media directly) _relies_ on 
his work: aufs (http://aufs.sourceforge.net/) to get the job done. Also 
many diskless setups (as do mine) relies on aufs.

It's a real pity, that all other approaches to layered filesystems (or 
filesystem unification, overlay filesystems), that are done by others, 
are either
 - technically inferior
 - restricted in usage scenarios
 - simply unfinished

but get more attention in the kernel guild. All, that Junjiro's approach 
takes is exporting a handful of symbols in order to be able to build it 
as a module also (and the number decreases as other lately merged 
subsystems do need them as well, i.e. apparmor). That's all. 

Layered filesystems are a killer feature, that could have been in the 
kernel for years, but is still missing for hard to follow (e.g. 
political, emotional) reasons. There are many interesting things, that 
can be done with layered filesystems, hard to archive up to impossible 
to be done otherwise). 

Sadly,
Pete

Re: vfs-scale, keep the errno

[Nick Piggin]

On Sat, Jan 15, 2011 at 12:17 AM, Hans-Peter Jansen <hpj@urpla.net> wrote:
> On Friday 14 January 2011, 05:01:37 Nick Piggin wrote:
>> On Fri, Jan 14, 2011 at 2:38 PM, J. R. Okajima <hooanon05@yahoo.co.jp>
> wrote:
>> > When open(2) without O_DIRECTORY opens an existing dir, it should
>> > return EISDIR. In do_last(), the variable 'error' is initialized
>> > EISDIR, but it is changed by d_revalidate() which returns any
>> > positive to represent 'the target dir is valid.'
>> > Should we keep and return the initialized 'error' in this case.
>>
>> Great, thank you very much. I just changed a few variable names but
>> applied it. Would you please add you Signed-off-by: on patches from
>> now on?
>>
>> Good reviewing work. Are you finding these by review or testing? I
>> think ltptests on an nfs mount should find this kind of bug... which
>> I should do.
>
> Nick, Junjiro is well known as a very smart (and|but) humble guy in the

I agree, based on the thoughtful review comments and patches :)

Thanks,
Nick