Re: [PATCH] grace: only add lock_manager to grace_list if it's not already there

[Vasily Averin <vvs@virtuozzo.com>]

On 2017-11-01 00:18, J. Bruce Fields wrote:
> On Tue, Oct 31, 2017 at 10:31:35AM +0300, Vasily Averin wrote:
>> On 2017-10-30 21:29, Jeff Layton wrote:
>>> Vasily, would this patch fix the panic you're seeing? We may still
>>> need to do something saner here, but this seems like it should at
>>> least prevent a double list_add.
>>
>> Jeff, I think your patch is wrong.
>>
>> Double list_add is not a real problem, it is just a marker of its presence.
>> It's great that such marker exist, it allows to detect the problems.

Jeff, what do you about about following hunk ?

--- a/fs/nfs_common/grace.c
+++ b/fs/nfs_common/grace.c
@@ -30,7 +30,11 @@ locks_start_grace(struct net *net, struct lock_manager *lm)
        struct list_head *grace_list = net_generic(net, grace_net_id);
 
        spin_lock(&grace_lock);
-       list_add(&lm->list, grace_list);
+       if (list_empty(&lm->list))
+               list_add(&lm->list, grace_list);
+       else
+               WARN(1, "double list_add attempt detected in %s %p\n",
+                    (net == &init_net) ? "init_net" : "net", net);
        spin_unlock(&grace_lock);
 }
 EXPORT_SYMBOL_GPL(locks_start_grace);

It allows to avoid list corruption and panic
but will report about detected problem

[  344.722040] double list_add attempt detected in init_net ffffffffa4fd9800
[  344.722108] ------------[ cut here ]------------
[  344.722142] WARNING: CPU: 3 PID: 1505 at fs/nfs_common/grace.c:37 locks_start_grace+0xa2/0xb0 [grace]


I think by same way we can also detect and disarm lost delayed_work on stop of network namespace.
(in lockd_exit_net)

>> 2) restart_grace() works with init_net only.
>> It can call set_grace_period() for init_net when it was not required
>> (i.e. when nfsd in init_net was stopped) and cause double list_add on its start.
>>
>> However main problem here is that it does nothing for any other net namespaces.
>> This part of lockd was not namespace-ified, and I would like to clarify how it's better to complete this task.
> 
> I'm not sure.  The original idea was that the grace period is global to
> the host (hypervisor), so as long as a server in any network namespace
> needs a grace period, normal locking should be blocked across all
> namespaces.  (This is really only necessarily if we know that a
> filesystem is exported from all namespaces; but since we don't keep
> track of that, we assume the worst.)
> 
> The signal interface to lockd I think of as a legacy interface.  As you
> say it might be risky to just rip it out completely.  But I'd be fine
> with it being limited to legacy use cases.  So if it doesn't play well
> with network namespaces, that's OK (as long as it doesn't crash).

Dear Bruce, thank you very much for explanation.