[PATCH 0/3] SUNRPC: three bugfixes for PipeFS event handling

[PATCH 0/3] SUNRPC: three bugfixes for PipeFS event handling

[Stanislav Kinsbursky]

These bugfixes were caught (or noticed) due to the following simple script:

======================================================================

#!/bin/bash
service rpcbind stop
service rpcidmapd stop
killall -TERM rpc.statd
umount /var/lib/nfs/rpc_pipefs
rmmod nfs
service rpcbind start
service nfs start
mount -t rpc_pipefs sunrpc /var/lib/nfs/rpc_pipefs

=======================================================================

The following series consists of:

---

Stanislav Kinsbursky (3):
      SUNRPC: skip clients with program without PipeFS entries
      SUNRPC: traverse clients tree on PipeFS event
      SUNRPC: set per-net PipeFS superblock before notification


 net/sunrpc/clnt.c     |   34 ++++++++++++++++++++++++++++------
 net/sunrpc/rpc_pipe.c |    3 ++-
 2 files changed, 30 insertions(+), 7 deletions(-)

[PATCH 1/3] SUNRPC: skip clients with program without PipeFS entries

[Stanislav Kinsbursky]

1) This is sane.
2) Otherwise there will be soft lockup:

do {
	rpc_get_client_for_event (clnt->cl_dentry == NULL ==> choose)
	__rpc_pipefs_event (clnt->cl_program->pipe_dir_name == NULL ==> return)
} while (1)


Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/clnt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d10ebc4..8a19849 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -184,8 +184,6 @@ static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
 
 	switch (event) {
 	case RPC_PIPEFS_MOUNT:
-		if (clnt->cl_program->pipe_dir_name == NULL)
-			break;
 		dentry = rpc_setup_pipedir_sb(sb, clnt,
 					      clnt->cl_program->pipe_dir_name);
 		BUG_ON(dentry == NULL);
@@ -215,6 +213,8 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 
 	spin_lock(&sn->rpc_client_lock);
 	list_for_each_entry(clnt, &sn->all_clients, cl_clients) {
+		if (clnt->cl_program->pipe_dir_name == NULL)
+			break;
 		if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
 		    ((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
 			continue;

[PATCH 2/3] SUNRPC: traverse clients tree on PipeFS event

[Stanislav Kinsbursky]

If client is a clone, then it's parent can not be in the list.
But parent's PipeFS dentries have to be created and destroyed as well.

Note: event skip helper for clients introduced

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/clnt.c |   30 ++++++++++++++++++++++++++----
 1 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8a19849..80b59f1 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -176,8 +176,16 @@ rpc_setup_pipedir(struct rpc_clnt *clnt, const char *dir_name)
 	return 0;
 }
 
-static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
-				struct super_block *sb)
+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;
+	return 0;
+}
+
+static int __rpc_clnt_handle_event(struct rpc_clnt *clnt, unsigned long event,
+				   struct super_block *sb)
 {
 	struct dentry *dentry;
 	int err = 0;
@@ -206,6 +214,21 @@ static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
 	return err;
 }
 
+static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
+				struct super_block *sb)
+{
+	int error;
+
+	if (!rpc_clnt_skip_event(clnt, event)) {
+		error = __rpc_clnt_handle_event(clnt, event, sb);
+		if (error)
+			return error;
+	}
+	if (clnt != clnt->cl_parent)
+		return __rpc_pipefs_event(clnt->cl_parent, event, sb);
+	return 0;
+}
+
 static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 {
 	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
@@ -215,8 +238,7 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 	list_for_each_entry(clnt, &sn->all_clients, cl_clients) {
 		if (clnt->cl_program->pipe_dir_name == NULL)
 			break;
-		if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
-		    ((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
+		if (rpc_clnt_skip_event(clnt, event))
 			continue;
 		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
 			continue;

Re: [PATCH 2/3] SUNRPC: traverse clients tree on PipeFS event

[Myklebust, Trond]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 980 bytes --]

On Fri, 2012-04-20 at 18:19 +0400, Stanislav Kinsbursky wrote:

> +static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
> +				struct super_block *sb)
> +{
> +	int error;
> +
> +	if (!rpc_clnt_skip_event(clnt, event)) {
> +		error = __rpc_clnt_handle_event(clnt, event, sb);
> +		if (error)
> +			return error;
> +	}
> +	if (clnt != clnt->cl_parent)
> +		return __rpc_pipefs_event(clnt->cl_parent, event, sb);
> +	return 0;
> +}

Hi Stanislav,

Recursion in the kernel is generally frowned upon due to the stack size
limits. Could you please rewrite the above into a simple loop. Something
along the lines of:

	for(;;) {
		...

		if (clnt == clnt->cl_parent)
			break;
		clnt = clnt->cl_parent;
	}

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/3] SUNRPC: traverse clients tree on PipeFS event

[Myklebust, Trond]

T24gRnJpLCAyMDEyLTA0LTIwIGF0IDE4OjE5ICswNDAwLCBTdGFuaXNsYXYgS2luc2J1cnNreSB3
cm90ZToNCg0KPiArc3RhdGljIGludCBfX3JwY19waXBlZnNfZXZlbnQoc3RydWN0IHJwY19jbG50
ICpjbG50LCB1bnNpZ25lZCBsb25nIGV2ZW50LA0KPiArCQkJCXN0cnVjdCBzdXBlcl9ibG9jayAq
c2IpDQo+ICt7DQo+ICsJaW50IGVycm9yOw0KPiArDQo+ICsJaWYgKCFycGNfY2xudF9za2lwX2V2
ZW50KGNsbnQsIGV2ZW50KSkgew0KPiArCQllcnJvciA9IF9fcnBjX2NsbnRfaGFuZGxlX2V2ZW50
KGNsbnQsIGV2ZW50LCBzYik7DQo+ICsJCWlmIChlcnJvcikNCj4gKwkJCXJldHVybiBlcnJvcjsN
Cj4gKwl9DQo+ICsJaWYgKGNsbnQgIT0gY2xudC0+Y2xfcGFyZW50KQ0KPiArCQlyZXR1cm4gX19y
cGNfcGlwZWZzX2V2ZW50KGNsbnQtPmNsX3BhcmVudCwgZXZlbnQsIHNiKTsNCj4gKwlyZXR1cm4g
MDsNCj4gK30NCg0KSGkgU3RhbmlzbGF2LA0KDQpSZWN1cnNpb24gaW4gdGhlIGtlcm5lbCBpcyBn
ZW5lcmFsbHkgZnJvd25lZCB1cG9uIGR1ZSB0byB0aGUgc3RhY2sgc2l6ZQ0KbGltaXRzLiBDb3Vs
ZCB5b3UgcGxlYXNlIHJld3JpdGUgdGhlIGFib3ZlIGludG8gYSBzaW1wbGUgbG9vcC4gU29tZXRo
aW5nDQphbG9uZyB0aGUgbGluZXMgb2Y6DQoNCglmb3IoOzspIHsNCgkJLi4uDQoNCgkJaWYgKGNs
bnQgPT0gY2xudC0+Y2xfcGFyZW50KQ0KCQkJYnJlYWs7DQoJCWNsbnQgPSBjbG50LT5jbF9wYXJl
bnQ7DQoJfQ0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFp
bmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29t
DQoNCg==

Re: [PATCH 2/3] SUNRPC: traverse clients tree on PipeFS event

[Stanislav Kinsbursky]

26.04.2012 22:11, Myklebust, Trond написал:
> On Fri, 2012-04-20 at 18:19 +0400, Stanislav Kinsbursky wrote:
>
>> +static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
>> +				struct super_block *sb)
>> +{
>> +	int error;
>> +
>> +	if (!rpc_clnt_skip_event(clnt, event)) {
>> +		error = __rpc_clnt_handle_event(clnt, event, sb);
>> +		if (error)
>> +			return error;
>> +	}
>> +	if (clnt != clnt->cl_parent)
>> +		return __rpc_pipefs_event(clnt->cl_parent, event, sb);
>> +	return 0;
>> +}
> Hi Stanislav,
>
> Recursion in the kernel is generally frowned upon due to the stack size
> limits. Could you please rewrite the above into a simple loop. Something
> along the lines of:
>
> 	for(;;) {
> 		...
>
> 		if (clnt == clnt->cl_parent)
> 			break;
> 		clnt = clnt->cl_parent;
> 	}
>

Hi, Trond.
Yes, sure, I can do this.

[PATCH v2 2/3] SUNRPC: traverse clients tree on PipeFS event

[Stanislav Kinsbursky]

v2: recursion was replaced by loop

If client is a clone, then it's parent can not be in the list.
But parent's Pipefs dentries have to be created and destroyed.

Note: event skip helper for clients introduced

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/clnt.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8a19849..d127bd7 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -176,8 +176,16 @@ rpc_setup_pipedir(struct rpc_clnt *clnt, const char *dir_name)
 	return 0;
 }
 
-static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
-				struct super_block *sb)
+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;
+	return 0;
+}
+
+static int __rpc_clnt_handle_event(struct rpc_clnt *clnt, unsigned long event,
+				   struct super_block *sb)
 {
 	struct dentry *dentry;
 	int err = 0;
@@ -206,6 +214,20 @@ static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
 	return err;
 }
 
+static int __rpc_pipefs_event(struct rpc_clnt *clnt, unsigned long event,
+				struct super_block *sb)
+{
+	int error = 0;
+
+	for (;; clnt = clnt->cl_parent) {
+		if (!rpc_clnt_skip_event(clnt, event))
+			error = __rpc_clnt_handle_event(clnt, event, sb);
+		if (error || clnt == clnt->cl_parent)
+			break;
+	}
+	return error;
+}
+
 static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 {
 	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
@@ -215,8 +237,7 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 	list_for_each_entry(clnt, &sn->all_clients, cl_clients) {
 		if (clnt->cl_program->pipe_dir_name == NULL)
 			break;
-		if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
-		    ((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
+		if (rpc_clnt_skip_event(clnt, event))
 			continue;
 		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
 			continue;

[PATCH 3/3] SUNRPC: set per-net PipeFS superblock before notification

[Stanislav Kinsbursky]

There can be a case, when on MOUNT event RPC client (after it's dentries were
created) is not longer hold by anyone except notification callback.
I.e. on release this client will be destoroyed. And it's dentries have to be
destroyed as well. Which in turn requires per-net PipeFS superblock to be set.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/rpc_pipe.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 0af37fc..3b62cf2 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1126,19 +1126,20 @@ 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));
+	sn->pipefs_sb = sb;
 	err = blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
 					   RPC_PIPEFS_MOUNT,
 					   sb);
 	if (err)
 		goto err_depopulate;
 	sb->s_fs_info = get_net(net);
-	sn->pipefs_sb = sb;
 	return 0;
 
 err_depopulate:
 	blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
 					   RPC_PIPEFS_UMOUNT,
 					   sb);
+	sn->pipefs_sb = NULL;
 	__rpc_depopulate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF);
 	return err;
 }

Re: [PATCH 0/3] SUNRPC: three bugfixes for PipeFS event handling

[J. Bruce Fields]

On Fri, Apr 20, 2012 at 06:18:59PM +0400, Stanislav Kinsbursky wrote:
> These bugfixes were caught (or noticed) due to the following simple script:

I'm assuming Trond should take these (possibly for 3.4?).

--b.

> 
> ======================================================================
> 
> #!/bin/bash
> service rpcbind stop
> service rpcidmapd stop
> killall -TERM rpc.statd
> umount /var/lib/nfs/rpc_pipefs
> rmmod nfs
> service rpcbind start
> service nfs start
> mount -t rpc_pipefs sunrpc /var/lib/nfs/rpc_pipefs
> 
> =======================================================================
> 
> The following series consists of:
> 
> ---
> 
> Stanislav Kinsbursky (3):
>       SUNRPC: skip clients with program without PipeFS entries
>       SUNRPC: traverse clients tree on PipeFS event
>       SUNRPC: set per-net PipeFS superblock before notification
> 
> 
>  net/sunrpc/clnt.c     |   34 ++++++++++++++++++++++++++++------
>  net/sunrpc/rpc_pipe.c |    3 ++-
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 

Re: [PATCH 0/3] SUNRPC: three bugfixes for PipeFS event handling

[Stanislav Kinsbursky]

25.04.2012 22:24, J. Bruce Fields написал:
> On Fri, Apr 20, 2012 at 06:18:59PM +0400, Stanislav Kinsbursky wrote:
>> These bugfixes were caught (or noticed) due to the following simple script:
> I'm assuming Trond should take these (possibly for 3.4?).

I think so.
But would be great if you'll take them as well (since I'm mostly working 
with your tree).
Of course, only if its not a big deal for you.
But if there will be any conflicts - then don't worry. I can just keep 
these patches in my repo.

> --b.
>
>> ======================================================================
>>
>> #!/bin/bash
>> service rpcbind stop
>> service rpcidmapd stop
>> killall -TERM rpc.statd
>> umount /var/lib/nfs/rpc_pipefs
>> rmmod nfs
>> service rpcbind start
>> service nfs start
>> mount -t rpc_pipefs sunrpc /var/lib/nfs/rpc_pipefs
>>
>> =======================================================================
>>
>> The following series consists of:
>>
>> ---
>>
>> Stanislav Kinsbursky (3):
>>        SUNRPC: skip clients with program without PipeFS entries
>>        SUNRPC: traverse clients tree on PipeFS event
>>        SUNRPC: set per-net PipeFS superblock before notification
>>
>>
>>   net/sunrpc/clnt.c     |   34 ++++++++++++++++++++++++++++------
>>   net/sunrpc/rpc_pipe.c |    3 ++-
>>   2 files changed, 30 insertions(+), 7 deletions(-)
>>

Re: [PATCH 0/3] SUNRPC: three bugfixes for PipeFS event handling

[J. Bruce Fields]

On Thu, Apr 26, 2012 at 12:59:07AM +0400, Stanislav Kinsbursky wrote:
> 25.04.2012 22:24, J. Bruce Fields написал:
> >On Fri, Apr 20, 2012 at 06:18:59PM +0400, Stanislav Kinsbursky wrote:
> >>These bugfixes were caught (or noticed) due to the following simple script:
> >I'm assuming Trond should take these (possibly for 3.4?).
> 
> I think so.
> But would be great if you'll take them as well (since I'm mostly
> working with your tree).
> Of course, only if its not a big deal for you.
> But if there will be any conflicts - then don't worry. I can just
> keep these patches in my repo.

If it's a bugfix that should go to 3.4, then best might be for Trond to
get it merged, then I'll pull it into my tree after that.

Or I'm happy to do it some other way if someone prefers.

--b.