Re: [PATCH RFC v3] vfs: make fstatat retry once on ESTALE errors from getattr call

[Ric Wheeler <rwheeler@redhat.com>]

On 04/20/2012 08:10 PM, Jeff Layton wrote:
> On Wed, 18 Apr 2012 07:52:07 -0400
> Jeff Layton<jlayton@redhat.com>  wrote:
>
>> ESTALE errors are a source of pain for many users of NFS. Usually they
>> occur when a file is removed from the server after a successful lookup
>> against it.
>>
>> Luckily, the remedy in these cases is usually simple. We should just
>> redo the lookup, forcing revalidations all the way in and then retry the
>> call. We of course cannot do this for syscalls that do not involve a
>> path, but for path-based syscalls we can and should attempt to recover
>> from an ESTALE.
>>
>> This patch implements this by having the VFS reattempt the lookup (with
>> LOOKUP_REVAL set) and call exactly once when it would ordinarily return
>> ESTALE. This should catch the bulk of these cases under normal usage,
>> without unduly inconveniencing other filesystems that return ESTALE on
>> path-based syscalls.
>>
>> Note that it's possible to hit this race more than once, but a single
>> retry should catch the bulk of these cases under normal circumstances.
>>
>> This patch is just an example. We'll alter most path-based syscalls in a
>> similar fashion to fix this correctly. At this point, I'm just trying to
>> ensure that the retry semantics are acceptable before I being that work.
>>
>> Does anyone have strong objections to this patch? I'm aware that the
>> retry mechanism is not as robust as many (e.g. Peter) would like, but it
>> should at least improve the current situation.
>>
>> If no one has a strong objection, then I'll start going through and
>> adding similar code to the other syscalls. And we can hopefully we can
>> get at least some of them in for 3.5.
>>
>> Signed-off-by: Jeff Layton<jlayton@redhat.com>
>> ---
>>   fs/stat.c |    9 ++++++++-
>>   1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/stat.c b/fs/stat.c
>> index c733dc5..0ee9cb4 100644
>> --- a/fs/stat.c
>> +++ b/fs/stat.c
>> @@ -73,7 +73,8 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
>>   {
>>   	struct path path;
>>   	int error = -EINVAL;
>> -	int lookup_flags = 0;
>> +	bool retried = false;
>> +	unsigned int lookup_flags = 0;
>>
>>   	if ((flag&  ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
>>   		      AT_EMPTY_PATH)) != 0)
>> @@ -84,12 +85,18 @@ int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
>>   	if (flag&  AT_EMPTY_PATH)
>>   		lookup_flags |= LOOKUP_EMPTY;
>>
>> +retry:
>>   	error = user_path_at(dfd, filename, lookup_flags,&path);
>>   	if (error)
>>   		goto out;
>>
>>   	error = vfs_getattr(path.mnt, path.dentry, stat);
>>   	path_put(&path);
>> +	if (error == -ESTALE&&  !retried) {
>> +		retried = true;
>> +		lookup_flags |= LOOKUP_REVAL;
>> +		goto retry;
>> +	}
>>   out:
>>   	return error;
>>   }
> Apologies for replying to myself here. Just to beat on the deceased
> equine a little longer, I should note that the above approach does
> *not* fix Peter's reproducer in his original email. It fails rather
> quickly when run simultaneously on the client and server.
>
> At least one of the tests in it creates and removes a hierarchy of
> directories in a loop. During that, the lookup from the client can
> easily fail more than once with ESTALE. Since we give up after a single
> retry, that causes the call to return ESTALE.
>
> While testing this approach with mkdir and fstatat, I modified the
> patch to retry multiple times, also retry when the lookup thows ESTALE
> and to throw a printk when the number of retries was>  1 with the
> number of retries that the call did and the eventual error code.
>
> With Peter's (admittedly synthetic) test, we can get an answer of sorts
> to Trond's question from earlier in the thread as to how many retries
> is "enough":
>
> [   45.023665] sys_mkdirat: retries=33 error=-2
> [   47.889300] sys_mkdirat: retries=51 error=-2
> [   49.172746] sys_mkdirat: retries=27 error=-2
> [   52.325723] sys_mkdirat: retries=10 error=-2
> [   58.082576] sys_mkdirat: retries=33 error=-2
> [   59.810461] sys_mkdirat: retries=5 error=-2
> [   63.387914] sys_mkdirat: retries=14 error=-2
> [   63.630785] sys_mkdirat: retries=4 error=-2
> [   68.268903] sys_mkdirat: retries=6 error=-2
> [   71.124173] sys_mkdirat: retries=99 error=-2
> [   75.657649] sys_mkdirat: retries=123 error=-2
> [   76.903814] sys_mkdirat: retries=26 error=-2
> [   82.009463] sys_mkdirat: retries=30 error=-2
> [   84.807731] sys_mkdirat: retries=67 error=-2
> [   89.825529] sys_mkdirat: retries=166 error=-2
> [   91.599104] sys_mkdirat: retries=8 error=-2
> [   95.621855] sys_mkdirat: retries=44 error=-2
> [   98.164588] sys_mkdirat: retries=61 error=-2
> [   99.783347] sys_mkdirat: retries=11 error=-2
> [  105.593980] sys_mkdirat: retries=104 error=-2
> [  110.348861] sys_mkdirat: retries=8 error=-2
> [  112.087966] sys_mkdirat: retries=46 error=-2
> [  117.613316] sys_mkdirat: retries=90 error=-2
> [  120.117550] sys_mkdirat: retries=2 error=-2
> [  122.624330] sys_mkdirat: retries=15 error=-2
>
> So, now I'm having buyers remorse of sorts about proposing to just
> retry once as that may not be strong enough to fix some of the cases
> we're interested in.
>
> I guess the questions at this point is:
>
> 1) How representative is Peter's mkdir_test() of a real-world workload?
>
> 2) if we assume that it is fairly representative of one, how can we
> achieve retrying indefinitely with NFS, or at least some large finite
> amount?
>
> I have my doubts as to whether it would really be as big a problem for
> other filesystems as Miklos and others have asserted, but I'll take
> their word for it at the moment. What's the best way to contain this
> behavior to just those filesystems that want to retry indefinitely when
> they get an ESTALE? Would we need to go with an entirely new
> ESTALERETRY after all?
>

Maybe we should have a default of a single loop and a tunable to allow clients 
to crank it up?

Ric