This series fixes races between PipeFS mount/umount notification calls and
SUNRPC clients creation and destruction.
https://bugzilla.redhat.com/show_bug.cgi?id=924649
v3: fixes first (for easy backport to stable)
v2: Fixed few silly locking bugs.
The following series implements...
---
Stanislav Kinsbursky (4):
SUNRPC: fix races on PipeFS MOUNT notifications
SUNRPC: fix races on PipeFS UMOUNT notifications
SUNRPC: split client creation routine into setup and registration
SUNRPC: PipeFS MOUNT notification optimization for dying clients
net/sunrpc/clnt.c | 71 ++++++++++++++++++++++++++++++-------------------
net/sunrpc/rpc_pipe.c | 5 +++
2 files changed, 48 insertions(+), 28 deletions(-)
CPU#0 CPU#1
----------------------------- -----------------------------
rpc_kill_sb
sn->pipefs_sb = NULL rpc_release_client
(UMOUNT_EVENT) rpc_free_auth
rpc_pipefs_event
rpc_get_client_for_event
!atomic_inc_not_zero(cl_count)
<skip the client>
atomic_inc(cl_count)
rpc_free_client
rpc_clnt_remove_pipedir
<skip client dir removing>
To fix this, this patch does the following:
1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
2) Removes SUNRPC client from the list AFTER pipes destroying.
3) Doesn't hold RPC client on notification: if client in the list, then it
can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.
Signed-off-by: Stanislav Kinsbursky <[email protected]>
Cc: [email protected]
---
net/sunrpc/clnt.c | 5 +----
net/sunrpc/rpc_pipe.c | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b827a4b..41f180c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -236,8 +236,6 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
continue;
if (rpc_clnt_skip_event(clnt, event))
continue;
- if (atomic_inc_not_zero(&clnt->cl_count) == 0)
- continue;
spin_unlock(&sn->rpc_client_lock);
return clnt;
}
@@ -254,7 +252,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
while ((clnt = rpc_get_client_for_event(sb->s_fs_info, event))) {
error = __rpc_pipefs_event(clnt, event, sb);
- rpc_release_client(clnt);
if (error)
break;
}
@@ -641,8 +638,8 @@ rpc_free_client(struct rpc_clnt *clnt)
rcu_dereference(clnt->cl_xprt)->servername);
if (clnt->cl_parent != clnt)
rpc_release_client(clnt->cl_parent);
- rpc_unregister_client(clnt);
rpc_clnt_remove_pipedir(clnt);
+ rpc_unregister_client(clnt);
rpc_free_iostats(clnt->cl_metrics);
kfree(clnt->cl_principal);
clnt->cl_metrics = NULL;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index c512448..efca2f7 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1165,7 +1165,6 @@ static void rpc_kill_sb(struct super_block *sb)
goto out;
}
sn->pipefs_sb = NULL;
- mutex_unlock(&sn->pipefs_sb_lock);
dprintk("RPC: sending pipefs UMOUNT notification for net %p%s\n",
net, NET_NAME(net));
blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
@@ -1173,6 +1172,7 @@ static void rpc_kill_sb(struct super_block *sb)
sb);
put_net(net);
out:
+ mutex_unlock(&sn->pipefs_sb_lock);
kill_litter_super(sb);
}
This helper moves all "registration" code to the new rpc_client_register()
helper.
This helper will be used later in the series to synchronize against PipeFS
MOUNT/UMOUNT events.
Signed-off-by: Stanislav Kinsbursky <[email protected]>
---
net/sunrpc/clnt.c | 64 ++++++++++++++++++++++++++++++++---------------------
1 files changed, 39 insertions(+), 25 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 41f180c..b4f1711 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -281,14 +281,47 @@ static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename)
memcpy(clnt->cl_nodename, nodename, clnt->cl_nodelen);
}
+static int rpc_client_register(const struct rpc_create_args *args,
+ struct rpc_clnt *clnt)
+{
+ const struct rpc_program *program = args->program;
+ struct rpc_auth *auth;
+ struct net *net = rpc_net_ns(clnt);
+ struct super_block *pipefs_sb;
+ int err = 0;
+
+ pipefs_sb = rpc_get_sb_net(net);
+ if (pipefs_sb) {
+ err = rpc_setup_pipedir(clnt, program->pipe_dir_name, pipefs_sb);
+ if (err)
+ goto out;
+ }
+
+ auth = rpcauth_create(args->authflavor, clnt);
+ if (IS_ERR(auth)) {
+ dprintk("RPC: Couldn't create auth handle (flavor %u)\n",
+ args->authflavor);
+ err = PTR_ERR(auth);
+ goto err_auth;
+ }
+
+ rpc_register_client(clnt);
+out:
+ if (pipefs_sb)
+ rpc_put_sb_net(net);
+ return err;
+
+err_auth:
+ __rpc_clnt_remove_pipedir(clnt);
+ goto out;
+}
+
static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, struct rpc_xprt *xprt)
{
const struct rpc_program *program = args->program;
const struct rpc_version *version;
struct rpc_clnt *clnt = NULL;
- struct rpc_auth *auth;
int err;
- struct super_block *pipefs_sb;
/* sanity check the name before trying to print it */
dprintk("RPC: creating %s client for %s (xprt %p)\n",
@@ -347,34 +380,15 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
atomic_set(&clnt->cl_count, 1);
- pipefs_sb = rpc_get_sb_net(rpc_net_ns(clnt));
- if (pipefs_sb) {
- err = rpc_setup_pipedir(clnt, program->pipe_dir_name, pipefs_sb);
- if (err)
- goto out_no_path;
- }
-
- auth = rpcauth_create(args->authflavor, clnt);
- if (IS_ERR(auth)) {
- dprintk("RPC: Couldn't create auth handle (flavor %u)\n",
- args->authflavor);
- err = PTR_ERR(auth);
- goto out_no_auth;
- }
-
/* save the nodename */
rpc_clnt_set_nodename(clnt, utsname()->nodename);
- rpc_register_client(clnt);
- if (pipefs_sb)
- rpc_put_sb_net(rpc_net_ns(clnt));
+
+ err = rpc_client_register(args, clnt);
+ if (err)
+ goto out_no_path;
return clnt;
-out_no_auth:
- if (pipefs_sb)
- __rpc_clnt_remove_pipedir(clnt);
out_no_path:
- if (pipefs_sb)
- rpc_put_sb_net(rpc_net_ns(clnt));
kfree(clnt->cl_principal);
out_no_principal:
rpc_free_iostats(clnt->cl_metrics);
Not need to create pipes for dying client. So just skip them.
Note: we can safely dereference the client structure, because notification
caller is holding sn->pipefs_sb_lock.
Signed-off-by: Stanislav Kinsbursky <[email protected]>
Cc: [email protected]
---
net/sunrpc/clnt.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b4f1711..f0339ae 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -177,6 +177,8 @@ static inline int rpc_clnt_skip_event(struct rpc_clnt *clnt, unsigned long event
if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
return 1;
+ if ((event == RPC_PIPEFS_MOUNT) && atomic_read(&clnt->cl_count) == 0)
+ return 1;
return 0;
}
Below are races, when RPC client can be created without PiepFS dentries
CPU#0 CPU#1
----------------------------- -----------------------------
rpc_new_client rpc_fill_super
rpc_setup_pipedir
mutex_lock(&sn->pipefs_sb_lock)
rpc_get_sb_net == NULL
(no per-net PipeFS superblock)
sn->pipefs_sb = sb;
notifier_call_chain(MOUNT)
(client is not in the list)
rpc_register_client
(client without pipes dentries)
To fix this patch:
1) makes PipeFS mount notification call with pipefs_sb_lock being held.
2) releases pipefs_sb_lock on new SUNRPC client creation only after
registration.
Signed-off-by: Stanislav Kinsbursky <[email protected]>
Cc: [email protected]
---
net/sunrpc/clnt.c | 26 +++++++++++++++-----------
net/sunrpc/rpc_pipe.c | 3 +++
2 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 5a750b9..b827a4b 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -157,20 +157,15 @@ static struct dentry *rpc_setup_pipedir_sb(struct super_block *sb,
}
static int
-rpc_setup_pipedir(struct rpc_clnt *clnt, const char *dir_name)
+rpc_setup_pipedir(struct rpc_clnt *clnt, const char *dir_name,
+ struct super_block *pipefs_sb)
{
- struct net *net = rpc_net_ns(clnt);
- struct super_block *pipefs_sb;
struct dentry *dentry;
clnt->cl_dentry = NULL;
if (dir_name == NULL)
return 0;
- pipefs_sb = rpc_get_sb_net(net);
- if (!pipefs_sb)
- return 0;
dentry = rpc_setup_pipedir_sb(pipefs_sb, clnt, dir_name);
- rpc_put_sb_net(net);
if (IS_ERR(dentry))
return PTR_ERR(dentry);
clnt->cl_dentry = dentry;
@@ -296,6 +291,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
struct rpc_clnt *clnt = NULL;
struct rpc_auth *auth;
int err;
+ struct super_block *pipefs_sb;
/* sanity check the name before trying to print it */
dprintk("RPC: creating %s client for %s (xprt %p)\n",
@@ -354,9 +350,12 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
atomic_set(&clnt->cl_count, 1);
- err = rpc_setup_pipedir(clnt, program->pipe_dir_name);
- if (err < 0)
- goto out_no_path;
+ pipefs_sb = rpc_get_sb_net(rpc_net_ns(clnt));
+ if (pipefs_sb) {
+ err = rpc_setup_pipedir(clnt, program->pipe_dir_name, pipefs_sb);
+ if (err)
+ goto out_no_path;
+ }
auth = rpcauth_create(args->authflavor, clnt);
if (IS_ERR(auth)) {
@@ -369,11 +368,16 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
/* save the nodename */
rpc_clnt_set_nodename(clnt, utsname()->nodename);
rpc_register_client(clnt);
+ if (pipefs_sb)
+ rpc_put_sb_net(rpc_net_ns(clnt));
return clnt;
out_no_auth:
- rpc_clnt_remove_pipedir(clnt);
+ if (pipefs_sb)
+ __rpc_clnt_remove_pipedir(clnt);
out_no_path:
+ if (pipefs_sb)
+ rpc_put_sb_net(rpc_net_ns(clnt));
kfree(clnt->cl_principal);
out_no_principal:
rpc_free_iostats(clnt->cl_metrics);
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index e7ce4b3..c512448 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1126,6 +1126,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
return -ENOMEM;
dprintk("RPC: sending pipefs MOUNT notification for net %p%s\n",
net, NET_NAME(net));
+ mutex_lock(&sn->pipefs_sb_lock);
sn->pipefs_sb = sb;
err = blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
RPC_PIPEFS_MOUNT,
@@ -1133,6 +1134,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
if (err)
goto err_depopulate;
sb->s_fs_info = get_net(net);
+ mutex_unlock(&sn->pipefs_sb_lock);
return 0;
err_depopulate:
@@ -1141,6 +1143,7 @@ err_depopulate:
sb);
sn->pipefs_sb = NULL;
__rpc_depopulate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF);
+ mutex_unlock(&sn->pipefs_sb_lock);
return err;
}
On Mon, 2013-06-24 at 11:52 +0400, Stanislav Kinsbursky wrote:
> CPU#0 CPU#1
> ----------------------------- -----------------------------
> rpc_kill_sb
> sn->pipefs_sb = NULL rpc_release_client
> (UMOUNT_EVENT) rpc_free_auth
> rpc_pipefs_event
> rpc_get_client_for_event
> !atomic_inc_not_zero(cl_count)
> <skip the client>
> atomic_inc(cl_count)
> rpc_free_client
> rpc_clnt_remove_pipedir
> <skip client dir removing>
>
> To fix this, this patch does the following:
>
> 1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
> 2) Removes SUNRPC client from the list AFTER pipes destroying.
> 3) Doesn't hold RPC client on notification: if client in the list, then it
> can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.
>
> Signed-off-by: Stanislav Kinsbursky <[email protected]>
> Cc: [email protected]
> ---
> net/sunrpc/clnt.c | 5 +----
> net/sunrpc/rpc_pipe.c | 2 +-
> 2 files changed, 2 insertions(+), 5 deletions(-)
<snip>
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index c512448..efca2f7 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -1165,7 +1165,6 @@ static void rpc_kill_sb(struct super_block *sb)
> goto out;
> }
> sn->pipefs_sb = NULL;
> - mutex_unlock(&sn->pipefs_sb_lock);
> dprintk("RPC: sending pipefs UMOUNT notification for net %p%s\n",
> net, NET_NAME(net));
> blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
> @@ -1173,6 +1172,7 @@ static void rpc_kill_sb(struct super_block *sb)
> sb);
> put_net(net);
> out:
> + mutex_unlock(&sn->pipefs_sb_lock);
Is this safe to do after the put_net()?
> kill_litter_super(sb);
> }
>
>
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
25.06.2013 20:13, Myklebust, Trond пишет:
> On Mon, 2013-06-24 at 11:52 +0400, Stanislav Kinsbursky wrote:
>> CPU#0 CPU#1
>> ----------------------------- -----------------------------
>> rpc_kill_sb
>> sn->pipefs_sb = NULL rpc_release_client
>> (UMOUNT_EVENT) rpc_free_auth
>> rpc_pipefs_event
>> rpc_get_client_for_event
>> !atomic_inc_not_zero(cl_count)
>> <skip the client>
>> atomic_inc(cl_count)
>> rpc_free_client
>> rpc_clnt_remove_pipedir
>> <skip client dir removing>
>>
>> To fix this, this patch does the following:
>>
>> 1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
>> 2) Removes SUNRPC client from the list AFTER pipes destroying.
>> 3) Doesn't hold RPC client on notification: if client in the list, then it
>> can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.
>>
>> Signed-off-by: Stanislav Kinsbursky <[email protected]>
>> Cc: [email protected]
>> ---
>> net/sunrpc/clnt.c | 5 +----
>> net/sunrpc/rpc_pipe.c | 2 +-
>> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> <snip>
>
>> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
>> index c512448..efca2f7 100644
>> --- a/net/sunrpc/rpc_pipe.c
>> +++ b/net/sunrpc/rpc_pipe.c
>> @@ -1165,7 +1165,6 @@ static void rpc_kill_sb(struct super_block *sb)
>> goto out;
>> }
>> sn->pipefs_sb = NULL;
>> - mutex_unlock(&sn->pipefs_sb_lock);
>> dprintk("RPC: sending pipefs UMOUNT notification for net %p%s\n",
>> net, NET_NAME(net));
>> blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
>> @@ -1173,6 +1172,7 @@ static void rpc_kill_sb(struct super_block *sb)
>> sb);
>> put_net(net);
>> out:
>> + mutex_unlock(&sn->pipefs_sb_lock);
>
> Is this safe to do after the put_net()?
>
Sure it's not. Sorry.
Will send the patch once more.
>> kill_litter_super(sb);
>> }
>>
>>
>
>
--
Best regards,
Stanislav Kinsbursky
CPU#0 CPU#1
----------------------------- -----------------------------
rpc_kill_sb
sn->pipefs_sb = NULL rpc_release_client
(UMOUNT_EVENT) rpc_free_auth
rpc_pipefs_event
rpc_get_client_for_event
!atomic_inc_not_zero(cl_count)
<skip the client>
atomic_inc(cl_count)
rpc_free_client
rpc_clnt_remove_pipedir
<skip client dir removing>
To fix this, this patch does the following:
1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
2) Removes SUNRPC client from the list AFTER pipes destroying.
3) Doesn't hold RPC client on notification: if client in the list, then it
can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.
Signed-off-by: Stanislav Kinsbursky <[email protected]>
Cc: [email protected]
---
net/sunrpc/clnt.c | 5 +----
net/sunrpc/rpc_pipe.c | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b827a4b..41f180c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -236,8 +236,6 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
continue;
if (rpc_clnt_skip_event(clnt, event))
continue;
- if (atomic_inc_not_zero(&clnt->cl_count) == 0)
- continue;
spin_unlock(&sn->rpc_client_lock);
return clnt;
}
@@ -254,7 +252,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
while ((clnt = rpc_get_client_for_event(sb->s_fs_info, event))) {
error = __rpc_pipefs_event(clnt, event, sb);
- rpc_release_client(clnt);
if (error)
break;
}
@@ -641,8 +638,8 @@ rpc_free_client(struct rpc_clnt *clnt)
rcu_dereference(clnt->cl_xprt)->servername);
if (clnt->cl_parent != clnt)
rpc_release_client(clnt->cl_parent);
- rpc_unregister_client(clnt);
rpc_clnt_remove_pipedir(clnt);
+ rpc_unregister_client(clnt);
rpc_free_iostats(clnt->cl_metrics);
kfree(clnt->cl_principal);
clnt->cl_metrics = NULL;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index c512448..9b4ec5b 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1165,12 +1165,12 @@ static void rpc_kill_sb(struct super_block *sb)
goto out;
}
sn->pipefs_sb = NULL;
- mutex_unlock(&sn->pipefs_sb_lock);
dprintk("RPC: sending pipefs UMOUNT notification for net %p%s\n",
net, NET_NAME(net));
blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
RPC_PIPEFS_UMOUNT,
sb);
+ mutex_unlock(&sn->pipefs_sb_lock);
put_net(net);
out:
kill_litter_super(sb);
On Wed, Jun 26, 2013 at 8:15 AM, Stanislav Kinsbursky
<[email protected]> wrote:
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index b827a4b..41f180c 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -236,8 +236,6 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
> continue;
> if (rpc_clnt_skip_event(clnt, event))
> continue;
> - if (atomic_inc_not_zero(&clnt->cl_count) == 0)
> - continue;
> spin_unlock(&sn->rpc_client_lock);
> return clnt;
> }
> @@ -254,7 +252,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
>
> while ((clnt = rpc_get_client_for_event(sb->s_fs_info, event))) {
> error = __rpc_pipefs_event(clnt, event, sb);
> - rpc_release_client(clnt);
> if (error)
> break;
> }
> @@ -641,8 +638,8 @@ rpc_free_client(struct rpc_clnt *clnt)
> rcu_dereference(clnt->cl_xprt)->servername);
> if (clnt->cl_parent != clnt)
> rpc_release_client(clnt->cl_parent);
> - rpc_unregister_client(clnt);
> rpc_clnt_remove_pipedir(clnt);
> + rpc_unregister_client(clnt);
> rpc_free_iostats(clnt->cl_metrics);
> kfree(clnt->cl_principal);
> clnt->cl_metrics = NULL;
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index c512448..9b4ec5b 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -1165,12 +1165,12 @@ static void rpc_kill_sb(struct super_block *sb)
> goto out;
> }
> sn->pipefs_sb = NULL;
> - mutex_unlock(&sn->pipefs_sb_lock);
> dprintk("RPC: sending pipefs UMOUNT notification for net %p%s\n",
> net, NET_NAME(net));
> blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
> RPC_PIPEFS_UMOUNT,
> sb);
> + mutex_unlock(&sn->pipefs_sb_lock);
> put_net(net);
> out:
> kill_litter_super(sb);
are these patches validated?
any chance to see them in tree soon? I wasn't able to find them on
http://git.linux-nfs.org/?p=trondmy/linux-nfs.git
Thanks,
--
William