RE: [PATCH RFC] vfs: make fstatat retry on ESTALE errors fromgetattr call

[Peter Staubach]

I don't think that the code ends up being all that complex, actually.  I will have to dig out the patches that I had previously to look to see what they did.  I am pretty sure that they handled all of these cases.  I also had some tests which exercised the modified path based system calls and at least, at one point in time, they would run without returning ESTALE to the user level.

Let me see what I can find.  SteveD, you wouldn't have squirrelled away a copy of my stuff from Red Hat, would you?

	Thanx...

		ps


-----Original Message-----
From: Myklebust, Trond [mailto:Trond.Myklebust@xxxxxxxxxx] 
Sent: Tuesday, April 17, 2012 10:09 AM
To: Peter Staubach
Cc: Jeff Layton; Bernd Schubert; Malahal Naineni; linux-nfs@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; miklos@xxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; hch@xxxxxxxxxxxxx; michael.brantley@xxxxxxxxxx; sven.breuner@xxxxxxxxxxxxxxxxxx
Subject: RE: [PATCH RFC] vfs: make fstatat retry on ESTALE errors from getattr call

On Tue, 2012-04-17 at 09:39 -0400, Peter Staubach wrote:
> Hi.
> 
> There does need to be support in the lookup path to handle stale directories.  During a multi-component lookup, it is possible for one component to be looked up, but then become stale before being used.  The support needs to go into more than just the set of system calls, but also into the lookup code.
> 
> I suspect that since the "" case is handled specially anyway, code could be added to do something like issue a GETATTR to verify that the directory is still valid.  Since "" is probably used minimally, the performance impact to the system should be also be minimal.

The problem with that is that even if your current directory is stale, a path of the form "../../../blah" can still be valid. I suspect that any code that tries to loop forever can quickly get very complicated as we start adding all these corner cases.

The thing about stat() is that in 99.99% of cases when the path revalidation code works, then the NFS client won't have to actually issue a GETATTR in the call to vfs_getstat() since the LOOKUP of that final path component will revalidate those attributes anyway.

The only exception to that rule will be the 'noac' mounts, which really can end up looping forever.

Cheers
  Trond

> 		ps
> 
> 
> -----Original Message-----
> From: Jeff Layton [mailto:jlayton@xxxxxxxxxx]
> Sent: Monday, April 16, 2012 7:06 PM
> To: Myklebust, Trond
> Cc: Bernd Schubert; Malahal Naineni; linux-nfs@xxxxxxxxxxxxxxx; 
> linux-fsdevel@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Peter 
> Staubach; miklos@xxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; 
> hch@xxxxxxxxxxxxx; michael.brantley@xxxxxxxxxx; 
> sven.breuner@xxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH RFC] vfs: make fstatat retry on ESTALE errors from 
> getattr call
> 
> On Mon, 16 Apr 2012 20:25:06 +0000
> "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote:
> 
> > On Mon, 2012-04-16 at 15:43 -0400, Jeff Layton wrote:
> > > On Mon, 16 Apr 2012 19:33:05 +0000 "Myklebust, Trond" 
> > > <Trond.Myklebust@xxxxxxxxxx> wrote:
> > > 
> > > > On Mon, 2012-04-16 at 13:46 -0400, Jeff Layton wrote:
> > > > > The question about looping indefinitely really comes down to:
> > > > > 
> > > > > 1) is a persistent ESTALE in conjunction with a successful 
> > > > > lookup a situation that we expect to be temporary. i.e. will 
> > > > > the admin at some point be able to do something about it? If 
> > > > > not, then there's no point in continuing to retry. Again, this 
> > > > > is a situation that *really* should not happen if the filesystem is doing the right thing.
> > > > > 
> > > > > 2) If the admin can't do anything about it, is it reasonable 
> > > > > to expect that users can send a fatal signal to hung 
> > > > > applications if this situation occurs.
> > > > > 
> > > > > We expect that that's ok in other situations to resolve hung 
> > > > > applications, so I'm not sure I understand why it wouldn't be 
> > > > > acceptable here...
> > > > 
> > > > There are definitely potentially persistent pathological 
> > > > situations that the filesystem can't do anything about. If the 
> > > > point of origin for your pathname (for instance your current 
> > > > directory in the case of a relative
> > > > pathname) is stale, then no amount of looping is going to help 
> > > > you to recover.
> > > > 
> > > 
> > > Ok -- Peter pretty much said something similar. Retrying 
> > > indefnitely when the lookup returns ESTALE probably won't help. 
> > > I'm ok with basically letting the VFS continue to do what it does there already.
> > > If it gets an ESTALE, it tries again with LOOKUP_REVAL set and 
> > > then gives up if that doesn't work.
> > > 
> > > If however, the operation itself keeps returning ESTALE, are we OK 
> > > to retry indefinitely assuming that we'll break out of the loop on 
> > > fatal signals?
> > >
> > > For example, something like the v2 patch I sent a little while ago?
> > 
> > 
> > Won't something like fstatat(AT_FDCWD, "", &stat, AT_EMPTY_PATH) 
> > risk looping forever there, or am I missing something?
> > 
> 
> To make sure I understand, that should be "shortcut" for a lookup of the cwd?
> 
> So I guess the concern is that you'd do the above and get a successful lookup since you're just going to get back the cwd. At that point, you'd attempt the getattr and get ESTALE back. Then, you'd redo the lookup with LOOKUP_REVAL set -- but since we're operating on the cwd, we don't have a way to redo the lookup since we don't have a pathname that we can look up again...
> 
> So yeah, I guess if you're sitting in a stale directory, something like that could loop eternally.
> 
> Do you think the proposed check for fatal_signal_pending is enough to mitigate such a problem? Or do we need to limit the number of retries to address those sorts of loops?
> 
> --
> Jeff Layton <jlayton@xxxxxxxxxx>

--
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com

N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i