pnfs: Send layoutreturn, if Return-On-Close is set

Every thing was ready, in pnfs_roc(). The segments released and the LO state blocked til after the close is done. All that is needed is to send the actual layoutreturn synchronously.

RFC1: It looks like the close that comes afterwords is also sync
So I assumed it would be always safe to wait here. Is that true?

RFC2: The ideal is if we could send the layoutreturn together
with the close, in the same compound. The only little problem with that is that we will need, at error handling, to analyse if the error came from the layoutreturn part of the compound, re-encode the close with out the layoutreturn and resend.
I mean, What stupid Server returns an error on layoutreturn?! really? why is it useful information to the client? what can he do differently. Any way ...

So I'm more lazy to do the right thing, right now. I promise to put it on my TODO, and do it soon.

TODO:
mark_lseg_invalid() should receive and collect the union *range* of all the segments that where ROC. And only send a layoutreturn on that collected range. (If lseg_list is empty then all-file return)

---
 fs/nfs/nfs4proc.c       |   10 ++++++++-
 fs/nfs/pnfs.c           |   52 +++++++++++++++++++++++++++++-----------------
 include/linux/nfs_xdr.h |    1 +
 3 files changed, 43 insertions(+), 20 deletions(-)

index 5734009..f32cc46 100644
@@ -5745,6 +5745,7 @@ int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp)
 		.rpc_message = &msg,
 		.callback_ops = &nfs4_layoutreturn_call_ops,
 		.callback_data = lrp,
+		.flags = RPC_TASK_ASYNC,
 	};
 	int status;
 

@@ -5753,8 +5754,15 @@ int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp)
 	if (IS_ERR(task))
 		return PTR_ERR(task);
 	status = task->tk_status;
-	dprintk("<-- %s status=%d\n", __func__, status);
+
+	if (!status && lrp->args.sync) {
+		status = nfs4_wait_for_completion_rpc_task(task);
+		if (status == 0)
+			status = task->tk_status;
+	}
+
 	rpc_put_task(task);
+	dprintk("<-- %s status=%d\n", __func__, status);
 	return status;
 }
 

index df99102..00b391a 100644
@@ -272,6 +272,7 @@ struct nfs4_layoutcommit_data {
 };
 
 struct nfs4_layoutreturn_args {
+	bool	sync;
 	__u32   reclaim;
 	__u32   layout_type;
 	struct inode *inode;

1.7.2.3

Why would we want to do this?

Return-on-close was initially considered useful only for debugging.

At the interim IETF meeting in Sunnyvale, we also discussed the case where the forgetful client has forgotten the layout: in this case the server may decide to forget the layout too. There is no controversy in doing this, since both the client and the server know that any outstanding layout is supposed to be returned (and if there is a problem, then the server always has the option of sending a CB_LAYOUTRECALL).

Adding a synchronous call to close is in any case a bug since close can on occasion be sent in situations where we don't allow sleeping.

Cheers
Trond

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

What ??

OK I didn't know that. So what you are saying is that if the server see a final close he can go and provocative free all segments marked with ROC?

If so then someone should fix the Linux server. Because currently it never frees them. On a modest machine like the UML I use. Few 10s of "git checkout linux" crash the machine with oom. Today they are only freed on client umount.

This is done only on the final close. Isn't the very final call sync?

Ok re-inspecting the code I can see that nfs4_do_close also takes a wait flag. I thought that the last close should always be waiting for all operations to end before proceeding with the close. That's how it is at the VFS level but I guess life is hard. So the only possible solution is within the same compound as the close. (not that we need it as you say)

Thanks
Boaz

That would be a bug. The server must either free or recall in this situation.

The problem is that LAYOUTRETURN may have to be sent with a _different_ credential than the CLOSE itself.

See the description of the EXCHGID4_FLAG_BIND_PRINC_STATEID exchangeid flag. Although we don't set that flag in our exchangeid requests, the _server_ may still set it in its reply, in which case we are supposed to obey it.
This is also a reason why sending OPEN and LAYOUTGET in the same compound can be problematic.

Cheers
Trond

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

BTW: We have yet to implement support for BIND_PRINC_STATEID in the client. One of the more "interesting" issues, I think, is that we probably want to handle EKEYEXPIRED differently: instead of just retrying, we should really clear out the layouts (forgetfully - since we don't have a credential) and reget them.

Note that SP4_MACH_CRED and SP4_SSV might help here, but we're not guaranteed that the server supports those even if it requires BIND_PRINC_STATEID.

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com