grace: only add lock_manager to grace_list if it's not already there

Commit Message

Vasily Averin Nov. 1, 2017, 10:10 a.m. UTC

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 ?


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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

J. Bruce Fields Nov. 9, 2017, 3:44 p.m. UTC | #1

On Wed, Nov 01, 2017 at 01:10:36PM +0300, Vasily Averin wrote:
> 
> 
> 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)

Jeff, were you planning to resend this?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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