[PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[andros]

From: Andy Adamson <andros@netapp.com>

Taking a reference on the nfs_open_context to signal that a layoutcommit is
needed is problematic because the last reference to the context triggers a
close (nfs_release).  But, if the layout holds a reference on the
nfs_open_context, then close will not be triggered.

Since we only use the rpc credential from the layoutcommit_ctx, replace the
layoutcommit_ctx with the rpc_cred.

Hold a reference on the rpc_cred until the layoutcommit rpc returns. Note that
the rpc layer (rpcauth_bind) also references the rpc_cred.

If the layoutdriver fails to setup the layoutcommit, clear the layoutcommit
properties and put the credential.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/inode.c            |    4 ++--
 fs/nfs/nfs4state.c        |    2 +-
 fs/nfs/pnfs.c             |   42 +++++++++++++++++-------------------------
 fs/nfs/write.c            |    2 +-
 include/linux/nfs4_pnfs.h |    6 ++++++
 include/linux/nfs_fs.h    |    3 +--
 include/linux/pnfs_xdr.h  |    1 -
 7 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7fd33d9..2b8e8e6 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	/*
 	 * file needs layout commit, server attributes may be stale
 	 */
-	if (nfsi->layoutcommit_ctx && nfsi->change_attr >= fattr->change_attr) {
+	if (do_layoutcommit(nfsi) && nfsi->change_attr >= fattr->change_attr) {
 		dprintk("NFS: %s: layoutcommit is needed for file %s/%ld\n",
 			__func__, inode->i_sb->s_id, inode->i_ino);
 		return 0;
@@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
 	nfsi->pnfs_layout_state = 0;
 	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
 	nfsi->layout.roc_iomode = 0;
-	nfsi->layoutcommit_ctx = NULL;
+	nfsi->lo_cred = NULL;
 	nfsi->pnfs_write_begin_pos = 0;
 	nfsi->pnfs_write_end_pos = 0;
 #endif /* CONFIG_NFS_V4_1 */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index d145de1..bf03fde 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
 #ifdef CONFIG_NFS_V4_1
 		struct nfs_inode *nfsi = NFS_I(state->inode);
 
-		if (nfsi->layoutcommit_ctx)
+		if (do_layoutcommit(nfsi))
 			pnfs_layoutcommit_inode(state->inode, wait);
 		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
 			struct nfs4_pnfs_layout_segment range;
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 20285bc..cb8ff06 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) {
 	return 0;
 }
 
-/* Set context to indicate we require a layoutcommit
+/* Set lo_cred to indicate we require a layoutcommit
  * If we don't even have a layout, we don't need to commit it.
  */
 void
 pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
 {
-	dprintk("%s: has_layout=%d layoutcommit_ctx=%p ctx=%p\n", __func__,
-		has_layout(nfsi), nfsi->layoutcommit_ctx, ctx);
+	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
 	spin_lock(&nfsi->lo_lock);
-	if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
-		nfsi->layoutcommit_ctx = get_nfs_open_context(ctx);
+	if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
+		nfsi->lo_cred = get_rpccred(ctx->state->owner->so_cred);
 		nfsi->change_attr++;
 		spin_unlock(&nfsi->lo_lock);
-		dprintk("%s: Set layoutcommit_ctx=%p\n", __func__,
-			nfsi->layoutcommit_ctx);
+		dprintk("%s: Set layoutcommit\n", __func__);
 		return;
 	}
 	spin_unlock(&nfsi->lo_lock);
@@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 				!pnfs_return_layout_barrier(nfsi, &arg));
 		}
 
-		if (nfsi->layoutcommit_ctx) {
+		if (do_layoutcommit(nfsi)) {
 			status = pnfs_layoutcommit_inode(ino, wait);
 			if (status) {
 				dprintk("%s: layoutcommit failed, status=%d. "
@@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
 
 	dprintk("%s: (status %d)\n", __func__, data->status);
 
-	/* TODO: For now, set an error in the open context (just like
-	 * if a commit failed) We may want to do more, much more, like
-	 * replay all writes through the NFSv4
-	 * server, or something.
-	 */
-	if (data->status < 0) {
+	if (data->status < 0)
 		printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
 		       __func__, data->status);
-		data->ctx->error = data->status;
-	}
 
 	/* TODO: Maybe we should avoid this by allowing the layout driver
 	 * to directly xdr its layout on the wire.
@@ -1929,9 +1920,6 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
 							&nfsi->layout,
 							&data->args,
 							data->status);
-
-	/* release the open_context acquired in pnfs_writeback_done */
-	put_nfs_open_context(data->ctx);
 }
 
 /*
@@ -1995,30 +1983,34 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
 		return -ENOMEM;
 
 	spin_lock(&nfsi->lo_lock);
-	if (!nfsi->layoutcommit_ctx)
+	if (!do_layoutcommit(nfsi))
 		goto out_unlock;
 
 	data->args.inode = inode;
-	data->cred  = nfsi->layoutcommit_ctx->cred;
-	data->ctx = nfsi->layoutcommit_ctx;
+	data->cred = nfsi->lo_cred;
 
 	/* Set up layout commit args*/
 	status = pnfs_layoutcommit_setup(data, sync);
-	if (status)
-		goto out_unlock;
 
 	/* Clear layoutcommit properties in the inode so
 	 * new lc info can be generated
 	 */
 	nfsi->pnfs_write_begin_pos = 0;
 	nfsi->pnfs_write_end_pos = 0;
-	nfsi->layoutcommit_ctx = NULL;
+	nfsi->lo_cred = NULL;
+
+	if (status) {
+		/* The layout driver failed to setup the layoutcommit */
+		put_rpccred(data->cred);
+		goto out_unlock;
+	}
 
 	/* release lock on pnfs layoutcommit attrs */
 	spin_unlock(&nfsi->lo_lock);
 
 	data->is_sync = sync;
 	status = pnfs4_proc_layoutcommit(data);
+	put_rpccred(data->cred);
 out:
 	dprintk("%s end (err:%d)\n", __func__, status);
 	return status;
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index a4c95a0..2c1918e 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode, int how)
 					nfs_wait_bit_killable,
 					TASK_KILLABLE);
 #ifdef CONFIG_NFS_V4_1
-		if (may_wait && NFS_I(inode)->layoutcommit_ctx) {
+		if (may_wait && do_layoutcommit(NFS_I(inode))) {
 			error = pnfs_layoutcommit_inode(inode, 1);
 			if (error < 0)
 				return error;
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index 4d47b48..c9ef43e 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
 	return nfsi->layout.ld_data != NULL;
 }
 
+static inline bool
+do_layoutcommit(struct nfs_inode *nfsi)
+{
+	return nfsi->lo_cred != NULL;
+}
+
 #endif /* CONFIG_NFS_V4_1 */
 
 struct pnfs_layout_segment {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 98a8dc0..478b00c 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -203,11 +203,10 @@ struct nfs_inode {
 #define NFS_INO_RW_LAYOUT_FAILED 1 	/* get rw layout failed stop trying */
 #define NFS_INO_LAYOUT_ALLOC     2	/* bit lock for layout allocation */
 	time_t pnfs_layout_suspend;
+	struct rpc_cred		*lo_cred; /* layoutcommit credential */
 	wait_queue_head_t lo_waitq;
 	spinlock_t lo_lock;
 	struct pnfs_layout_type layout;
-	/* use rpc_creds in this open_context to send LAYOUTCOMMIT to MDS */
-	struct nfs_open_context *layoutcommit_ctx;
 	/* DH: These vars keep track of the maximum write range
 	 * so the values can be used for layoutcommit.
 	 */
diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
index a0bf341..154b04e 100644
--- a/include/linux/pnfs_xdr.h
+++ b/include/linux/pnfs_xdr.h
@@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
 	bool is_sync;
 	struct rpc_cred *cred;
 	struct nfs_fattr fattr;
-	struct nfs_open_context *ctx;
 	struct pnfs_layoutcommit_arg args;
 	struct pnfs_layoutcommit_res res;
 	int status;
-- 
1.6.6

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Tao Guo]

On Fri, May 21, 2010 at 12:04 AM,  <andros@netapp.com> wrote:
> From: Andy Adamson <andros@netapp.com>
>
> Taking a reference on the nfs_open_context to signal that a layoutcom=
mit is
> needed is problematic because the last reference to the context trigg=
ers a
> close (nfs_release). =C2=A0But, if the layout holds a reference on th=
e
> nfs_open_context, then close will not be triggered.
>
> Since we only use the rpc credential from the layoutcommit_ctx, repla=
ce the
> layoutcommit_ctx with the rpc_cred.
>
> Hold a reference on the rpc_cred until the layoutcommit rpc returns. =
Note that
> the rpc layer (rpcauth_bind) also references the rpc_cred.
>
> If the layoutdriver fails to setup the layoutcommit, clear the layout=
commit
> properties and put the credential.
>
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
> =C2=A0fs/nfs/inode.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0=
 =C2=A04 ++--
> =C2=A0fs/nfs/nfs4state.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A02 =
+-
> =C2=A0fs/nfs/pnfs.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0=
 42 +++++++++++++++++-------------------------
> =C2=A0fs/nfs/write.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0=
 =C2=A02 +-
> =C2=A0include/linux/nfs4_pnfs.h | =C2=A0 =C2=A06 ++++++
> =C2=A0include/linux/nfs_fs.h =C2=A0 =C2=A0| =C2=A0 =C2=A03 +--
> =C2=A0include/linux/pnfs_xdr.h =C2=A0| =C2=A0 =C2=A01 -
> =C2=A07 files changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 7fd33d9..2b8e8e6 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode=
, struct nfs_fattr *fattr)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 * file needs layout commit, server attrib=
utes may be stale
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> - =C2=A0 =C2=A0 =C2=A0 if (nfsi->layoutcommit_ctx && nfsi->change_att=
r >=3D fattr->change_attr) {
> + =C2=A0 =C2=A0 =C2=A0 if (do_layoutcommit(nfsi) && nfsi->change_attr=
 >=3D fattr->change_attr) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("NFS: =
%s: layoutcommit is needed for file %s/%ld\n",
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0__func__, inode->i_sb->s_id, inode->i_ino);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
> @@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_in=
ode *nfsi)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_layout_state =3D 0;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0memset(&nfsi->layout.stateid, 0, NFS4_STAT=
EID_SIZE);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->layout.roc_iomode =3D 0;
> - =C2=A0 =C2=A0 =C2=A0 nfsi->layoutcommit_ctx =3D NULL;
> + =C2=A0 =C2=A0 =C2=A0 nfsi->lo_cred =3D NULL;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_write_begin_pos =3D 0;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_write_end_pos =3D 0;
> =C2=A0#endif /* CONFIG_NFS_V4_1 */
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index d145de1..bf03fde 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struc=
t nfs4_state *state, fmode_t fm
> =C2=A0#ifdef CONFIG_NFS_V4_1
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct nfs_ino=
de *nfsi =3D NFS_I(state->inode);
>
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (nfsi->layoutco=
mmit_ctx)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (do_layoutcommi=
t(nfsi))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0pnfs_layoutcommit_inode(state->inode, wait);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (has_layout=
(nfsi) && nfsi->layout.roc_iomode) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0struct nfs4_pnfs_layout_segment range;
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 20285bc..cb8ff06 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) =
{
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
> =C2=A0}
>
> -/* Set context to indicate we require a layoutcommit
> +/* Set lo_cred to indicate we require a layoutcommit
> =C2=A0* If we don't even have a layout, we don't need to commit it.
> =C2=A0*/
> =C2=A0void
> =C2=A0pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_=
context *ctx)
> =C2=A0{
> - =C2=A0 =C2=A0 =C2=A0 dprintk("%s: has_layout=3D%d layoutcommit_ctx=3D=
%p ctx=3D%p\n", __func__,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 has_layout(nfsi), =
nfsi->layoutcommit_ctx, ctx);
> + =C2=A0 =C2=A0 =C2=A0 dprintk("%s: has_layout=3D%d ctx=3D%p\n", __fu=
nc__, has_layout(nfsi), ctx);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock(&nfsi->lo_lock);
> - =C2=A0 =C2=A0 =C2=A0 if (has_layout(nfsi) && !nfsi->layoutcommit_ct=
x) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nfsi->layoutcommit=
_ctx =3D get_nfs_open_context(ctx);
> + =C2=A0 =C2=A0 =C2=A0 if (has_layout(nfsi) && !do_layoutcommit(nfsi)=
) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nfsi->lo_cred =3D =
get_rpccred(ctx->state->owner->so_cred);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->change_a=
ttr++;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock(&n=
fsi->lo_lock);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dprintk("%s: Set l=
ayoutcommit_ctx=3D%p\n", __func__,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 nfsi->layoutcommit_ctx);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dprintk("%s: Set l=
ayoutcommit\n", __func__);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock(&nfsi->lo_lock);
> @@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs=
4_pnfs_layout_segment *range,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0!pnfs_return_layout_barrier(n=
fsi, &arg));
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (nfsi->layoutco=
mmit_ctx) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (do_layoutcommi=
t(nfsi)) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0status =3D pnfs_layoutcommit_inode(ino, wait);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0if (status) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("%s: layoutcommit fai=
led, status=3D%d. "
> @@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommi=
t_data *data)
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("%s: (status %d)\n", __func__, dat=
a->status);
>
> - =C2=A0 =C2=A0 =C2=A0 /* TODO: For now, set an error in the open con=
text (just like
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0* if a commit failed) We may want to do =
more, much more, like
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0* replay all writes through the NFSv4
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0* server, or something.
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> - =C2=A0 =C2=A0 =C2=A0 if (data->status < 0) {
> + =C2=A0 =C2=A0 =C2=A0 if (data->status < 0)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printk(KERN_ER=
R "%s, Layoutcommit Failed! =3D %d\n",
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 __func__, data->status);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 data->ctx->error =3D=
 data->status;
> - =C2=A0 =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* TODO: Maybe we should avoid this by all=
owing the layout driver
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 * to directly xdr its layout on the wire.
> @@ -1929,9 +1920,6 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit=
_data *data)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&nfsi->layout,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&data->args,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0data->status);
> -
> - =C2=A0 =C2=A0 =C2=A0 /* release the open_context acquired in pnfs_w=
riteback_done */
> - =C2=A0 =C2=A0 =C2=A0 put_nfs_open_context(data->ctx);
> =C2=A0}
>
> =C2=A0/*
> @@ -1995,30 +1983,34 @@ pnfs_layoutcommit_inode(struct inode *inode, =
int sync)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM=
;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock(&nfsi->lo_lock);
> - =C2=A0 =C2=A0 =C2=A0 if (!nfsi->layoutcommit_ctx)
> + =C2=A0 =C2=A0 =C2=A0 if (!do_layoutcommit(nfsi))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out_unloc=
k;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->args.inode =3D inode;
> - =C2=A0 =C2=A0 =C2=A0 data->cred =C2=A0=3D nfsi->layoutcommit_ctx->c=
red;
> - =C2=A0 =C2=A0 =C2=A0 data->ctx =3D nfsi->layoutcommit_ctx;
> + =C2=A0 =C2=A0 =C2=A0 data->cred =3D nfsi->lo_cred;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Set up layout commit args*/
> =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D pnfs_layoutcommit_setup(data, s=
ync);
> - =C2=A0 =C2=A0 =C2=A0 if (status)
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out_unlock;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Clear layoutcommit properties in the in=
ode so
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 * new lc info can be generated
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_write_begin_pos =3D 0;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_write_end_pos =3D 0;
> - =C2=A0 =C2=A0 =C2=A0 nfsi->layoutcommit_ctx =3D NULL;
> + =C2=A0 =C2=A0 =C2=A0 nfsi->lo_cred =3D NULL;
> +
> + =C2=A0 =C2=A0 =C2=A0 if (status) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* The layout driv=
er failed to setup the layoutcommit */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 put_rpccred(data->=
cred);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out_unlock;
> + =C2=A0 =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* release lock on pnfs layoutcommit attrs=
 */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock(&nfsi->lo_lock);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->is_sync =3D sync;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D pnfs4_proc_layoutcommit(data);
> + =C2=A0 =C2=A0 =C2=A0 put_rpccred(data->cred);
Is this OK to put_rpccred here if sync =3D=3D 0? why not move it into
pnfs_layoutcommit_done just as the code did?
--tao

> =C2=A0out:
> =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("%s end (err:%d)\n", __func__, sta=
tus);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return status;
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index a4c95a0..2c1918e 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode=
, int how)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0n=
fs_wait_bit_killable,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0T=
ASK_KILLABLE);
> =C2=A0#ifdef CONFIG_NFS_V4_1
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (may_wait && NF=
S_I(inode)->layoutcommit_ctx) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (may_wait && do=
_layoutcommit(NFS_I(inode))) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0error =3D pnfs_layoutcommit_inode(inode, 1);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0if (error < 0)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return error;
> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
> index 4d47b48..c9ef43e 100644
> --- a/include/linux/nfs4_pnfs.h
> +++ b/include/linux/nfs4_pnfs.h
> @@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return nfsi->layout.ld_data !=3D NULL;
> =C2=A0}
>
> +static inline bool
> +do_layoutcommit(struct nfs_inode *nfsi)
> +{
> + =C2=A0 =C2=A0 =C2=A0 return nfsi->lo_cred !=3D NULL;
> +}
> +
> =C2=A0#endif /* CONFIG_NFS_V4_1 */
>
> =C2=A0struct pnfs_layout_segment {
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 98a8dc0..478b00c 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -203,11 +203,10 @@ struct nfs_inode {
> =C2=A0#define NFS_INO_RW_LAYOUT_FAILED 1 =C2=A0 =C2=A0 /* get rw layo=
ut failed stop trying */
> =C2=A0#define NFS_INO_LAYOUT_ALLOC =C2=A0 =C2=A0 2 =C2=A0 =C2=A0 /* b=
it lock for layout allocation */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0time_t pnfs_layout_suspend;
> + =C2=A0 =C2=A0 =C2=A0 struct rpc_cred =C2=A0 =C2=A0 =C2=A0 =C2=A0 *l=
o_cred; /* layoutcommit credential */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0wait_queue_head_t lo_waitq;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0spinlock_t lo_lock;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct pnfs_layout_type layout;
> - =C2=A0 =C2=A0 =C2=A0 /* use rpc_creds in this open_context to send =
LAYOUTCOMMIT to MDS */
> - =C2=A0 =C2=A0 =C2=A0 struct nfs_open_context *layoutcommit_ctx;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* DH: These vars keep track of the maximu=
m write range
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 * so the values can be used for layoutcom=
mit.
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
> index a0bf341..154b04e 100644
> --- a/include/linux/pnfs_xdr.h
> +++ b/include/linux/pnfs_xdr.h
> @@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0bool is_sync;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct rpc_cred *cred;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct nfs_fattr fattr;
> - =C2=A0 =C2=A0 =C2=A0 struct nfs_open_context *ctx;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct pnfs_layoutcommit_arg args;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct pnfs_layoutcommit_res res;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0int status;
> --
> 1.6.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.ht=
ml
>



--=20
tao.

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[William A. (Andy) Adamson]

On Thu, May 20, 2010 at 10:50 PM, Tao Guo <glorioustao@gmail.com> wrote=
:
> On Fri, May 21, 2010 at 12:04 AM, =A0<andros@netapp.com> wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Taking a reference on the nfs_open_context to signal that a layoutco=
mmit is
>> needed is problematic because the last reference to the context trig=
gers a
>> close (nfs_release). =A0But, if the layout holds a reference on the
>> nfs_open_context, then close will not be triggered.
>>
>> Since we only use the rpc credential from the layoutcommit_ctx, repl=
ace the
>> layoutcommit_ctx with the rpc_cred.
>>
>> Hold a reference on the rpc_cred until the layoutcommit rpc returns.=
 Note that
>> the rpc layer (rpcauth_bind) also references the rpc_cred.
>>
>> If the layoutdriver fails to setup the layoutcommit, clear the layou=
tcommit
>> properties and put the credential.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> =A0fs/nfs/inode.c =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A04 ++--
>> =A0fs/nfs/nfs4state.c =A0 =A0 =A0 =A0| =A0 =A02 +-
>> =A0fs/nfs/pnfs.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 42 +++++++++++++++++-=
------------------------
>> =A0fs/nfs/write.c =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A02 +-
>> =A0include/linux/nfs4_pnfs.h | =A0 =A06 ++++++
>> =A0include/linux/nfs_fs.h =A0 =A0| =A0 =A03 +--
>> =A0include/linux/pnfs_xdr.h =A0| =A0 =A01 -
>> =A07 files changed, 28 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 7fd33d9..2b8e8e6 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inod=
e, struct nfs_fattr *fattr)
>> =A0 =A0 =A0 =A0/*
>> =A0 =A0 =A0 =A0 * file needs layout commit, server attributes may be=
 stale
>> =A0 =A0 =A0 =A0 */
>> - =A0 =A0 =A0 if (nfsi->layoutcommit_ctx && nfsi->change_attr >=3D f=
attr->change_attr) {
>> + =A0 =A0 =A0 if (do_layoutcommit(nfsi) && nfsi->change_attr >=3D fa=
ttr->change_attr) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dprintk("NFS: %s: layoutcommit is nee=
ded for file %s/%ld\n",
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__func__, inode->i_sb=
->s_id, inode->i_ino);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0;
>> @@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_i=
node *nfsi)
>> =A0 =A0 =A0 =A0nfsi->pnfs_layout_state =3D 0;
>> =A0 =A0 =A0 =A0memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>> =A0 =A0 =A0 =A0nfsi->layout.roc_iomode =3D 0;
>> - =A0 =A0 =A0 nfsi->layoutcommit_ctx =3D NULL;
>> + =A0 =A0 =A0 nfsi->lo_cred =3D NULL;
>> =A0 =A0 =A0 =A0nfsi->pnfs_write_begin_pos =3D 0;
>> =A0 =A0 =A0 =A0nfsi->pnfs_write_end_pos =3D 0;
>> =A0#endif /* CONFIG_NFS_V4_1 */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index d145de1..bf03fde 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, stru=
ct nfs4_state *state, fmode_t fm
>> =A0#ifdef CONFIG_NFS_V4_1
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct nfs_inode *nfsi =3D NFS_I(stat=
e->inode);
>>
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (nfsi->layoutcommit_ctx)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (do_layoutcommit(nfsi))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pnfs_layoutcommit_ino=
de(state->inode, wait);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (has_layout(nfsi) && nfsi->layout.=
roc_iomode) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct nfs4_pnfs_layo=
ut_segment range;
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 20285bc..cb8ff06 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module)=
 {
>> =A0 =A0 =A0 =A0return 0;
>> =A0}
>>
>> -/* Set context to indicate we require a layoutcommit
>> +/* Set lo_cred to indicate we require a layoutcommit
>> =A0* If we don't even have a layout, we don't need to commit it.
>> =A0*/
>> =A0void
>> =A0pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_co=
ntext *ctx)
>> =A0{
>> - =A0 =A0 =A0 dprintk("%s: has_layout=3D%d layoutcommit_ctx=3D%p ctx=
=3D%p\n", __func__,
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 has_layout(nfsi), nfsi->layoutcommit_c=
tx, ctx);
>> + =A0 =A0 =A0 dprintk("%s: has_layout=3D%d ctx=3D%p\n", __func__, ha=
s_layout(nfsi), ctx);
>> =A0 =A0 =A0 =A0spin_lock(&nfsi->lo_lock);
>> - =A0 =A0 =A0 if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 nfsi->layoutcommit_ctx =3D get_nfs_ope=
n_context(ctx);
>> + =A0 =A0 =A0 if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 nfsi->lo_cred =3D get_rpccred(ctx->sta=
te->owner->so_cred);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0nfsi->change_attr++;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spin_unlock(&nfsi->lo_lock);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dprintk("%s: Set layoutcommit_ctx=3D%p=
\n", __func__,
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 nfsi->layoutcommit_ctx=
);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dprintk("%s: Set layoutcommit\n", __fu=
nc__);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
>> =A0 =A0 =A0 =A0}
>> =A0 =A0 =A0 =A0spin_unlock(&nfsi->lo_lock);
>> @@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nf=
s4_pnfs_layout_segment *range,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0!pnfs=
_return_layout_barrier(nfsi, &arg));
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>>
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (nfsi->layoutcommit_ctx) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (do_layoutcommit(nfsi)) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0status =3D pnfs_layou=
tcommit_inode(ino, wait);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (status) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dprin=
tk("%s: layoutcommit failed, status=3D%d. "
>> @@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcomm=
it_data *data)
>>
>> =A0 =A0 =A0 =A0dprintk("%s: (status %d)\n", __func__, data->status);
>>
>> - =A0 =A0 =A0 /* TODO: For now, set an error in the open context (ju=
st like
>> - =A0 =A0 =A0 =A0* if a commit failed) We may want to do more, much =
more, like
>> - =A0 =A0 =A0 =A0* replay all writes through the NFSv4
>> - =A0 =A0 =A0 =A0* server, or something.
>> - =A0 =A0 =A0 =A0*/
>> - =A0 =A0 =A0 if (data->status < 0) {
>> + =A0 =A0 =A0 if (data->status < 0)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0printk(KERN_ERR "%s, Layoutcommit Fai=
led! =3D %d\n",
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 __func__, data->status);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 data->ctx->error =3D data->status;
>> - =A0 =A0 =A0 }
>>
>> =A0 =A0 =A0 =A0/* TODO: Maybe we should avoid this by allowing the l=
ayout driver
>> =A0 =A0 =A0 =A0 * to directly xdr its layout on the wire.
>> @@ -1929,9 +1920,6 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommi=
t_data *data)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&nfsi->layout,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&data->args,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0data->status);
>> -
>> - =A0 =A0 =A0 /* release the open_context acquired in pnfs_writeback=
_done */
>> - =A0 =A0 =A0 put_nfs_open_context(data->ctx);
>> =A0}
>>
>> =A0/*
>> @@ -1995,30 +1983,34 @@ pnfs_layoutcommit_inode(struct inode *inode,=
 int sync)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENOMEM;
>>
>> =A0 =A0 =A0 =A0spin_lock(&nfsi->lo_lock);
>> - =A0 =A0 =A0 if (!nfsi->layoutcommit_ctx)
>> + =A0 =A0 =A0 if (!do_layoutcommit(nfsi))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out_unlock;
>>
>> =A0 =A0 =A0 =A0data->args.inode =3D inode;
>> - =A0 =A0 =A0 data->cred =A0=3D nfsi->layoutcommit_ctx->cred;
>> - =A0 =A0 =A0 data->ctx =3D nfsi->layoutcommit_ctx;
>> + =A0 =A0 =A0 data->cred =3D nfsi->lo_cred;
>>
>> =A0 =A0 =A0 =A0/* Set up layout commit args*/
>> =A0 =A0 =A0 =A0status =3D pnfs_layoutcommit_setup(data, sync);
>> - =A0 =A0 =A0 if (status)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out_unlock;
>>
>> =A0 =A0 =A0 =A0/* Clear layoutcommit properties in the inode so
>> =A0 =A0 =A0 =A0 * new lc info can be generated
>> =A0 =A0 =A0 =A0 */
>> =A0 =A0 =A0 =A0nfsi->pnfs_write_begin_pos =3D 0;
>> =A0 =A0 =A0 =A0nfsi->pnfs_write_end_pos =3D 0;
>> - =A0 =A0 =A0 nfsi->layoutcommit_ctx =3D NULL;
>> + =A0 =A0 =A0 nfsi->lo_cred =3D NULL;
>> +
>> + =A0 =A0 =A0 if (status) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* The layout driver failed to setup t=
he layoutcommit */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 put_rpccred(data->cred);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out_unlock;
>> + =A0 =A0 =A0 }
>>
>> =A0 =A0 =A0 =A0/* release lock on pnfs layoutcommit attrs */
>> =A0 =A0 =A0 =A0spin_unlock(&nfsi->lo_lock);
>>
>> =A0 =A0 =A0 =A0data->is_sync =3D sync;
>> =A0 =A0 =A0 =A0status =3D pnfs4_proc_layoutcommit(data);
>> + =A0 =A0 =A0 put_rpccred(data->cred);
> Is this OK to put_rpccred here if sync =3D=3D 0? why not move it into
> pnfs_layoutcommit_done just as the code did?

As I said in the patch comments, the RPC layer takes a reference on the=
 cred,

-->Andy

> --tao
>
>> =A0out:
>> =A0 =A0 =A0 =A0dprintk("%s end (err:%d)\n", __func__, status);
>> =A0 =A0 =A0 =A0return status;
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index a4c95a0..2c1918e 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inod=
e, int how)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0nfs_wait_bit_killable,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0TASK_KILLABLE);
>> =A0#ifdef CONFIG_NFS_V4_1
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (may_wait && NFS_I(inode)->layoutco=
mmit_ctx) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (may_wait && do_layoutcommit(NFS_I(=
inode))) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D pnfs_layout=
commit_inode(inode, 1);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (error < 0)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0retur=
n error;
>> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
>> index 4d47b48..c9ef43e 100644
>> --- a/include/linux/nfs4_pnfs.h
>> +++ b/include/linux/nfs4_pnfs.h
>> @@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
>> =A0 =A0 =A0 =A0return nfsi->layout.ld_data !=3D NULL;
>> =A0}
>>
>> +static inline bool
>> +do_layoutcommit(struct nfs_inode *nfsi)
>> +{
>> + =A0 =A0 =A0 return nfsi->lo_cred !=3D NULL;
>> +}
>> +
>> =A0#endif /* CONFIG_NFS_V4_1 */
>>
>> =A0struct pnfs_layout_segment {
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index 98a8dc0..478b00c 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -203,11 +203,10 @@ struct nfs_inode {
>> =A0#define NFS_INO_RW_LAYOUT_FAILED 1 =A0 =A0 /* get rw layout faile=
d stop trying */
>> =A0#define NFS_INO_LAYOUT_ALLOC =A0 =A0 2 =A0 =A0 /* bit lock for la=
yout allocation */
>> =A0 =A0 =A0 =A0time_t pnfs_layout_suspend;
>> + =A0 =A0 =A0 struct rpc_cred =A0 =A0 =A0 =A0 *lo_cred; /* layoutcom=
mit credential */
>> =A0 =A0 =A0 =A0wait_queue_head_t lo_waitq;
>> =A0 =A0 =A0 =A0spinlock_t lo_lock;
>> =A0 =A0 =A0 =A0struct pnfs_layout_type layout;
>> - =A0 =A0 =A0 /* use rpc_creds in this open_context to send LAYOUTCO=
MMIT to MDS */
>> - =A0 =A0 =A0 struct nfs_open_context *layoutcommit_ctx;
>> =A0 =A0 =A0 =A0/* DH: These vars keep track of the maximum write ran=
ge
>> =A0 =A0 =A0 =A0 * so the values can be used for layoutcommit.
>> =A0 =A0 =A0 =A0 */
>> diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
>> index a0bf341..154b04e 100644
>> --- a/include/linux/pnfs_xdr.h
>> +++ b/include/linux/pnfs_xdr.h
>> @@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
>> =A0 =A0 =A0 =A0bool is_sync;
>> =A0 =A0 =A0 =A0struct rpc_cred *cred;
>> =A0 =A0 =A0 =A0struct nfs_fattr fattr;
>> - =A0 =A0 =A0 struct nfs_open_context *ctx;
>> =A0 =A0 =A0 =A0struct pnfs_layoutcommit_arg args;
>> =A0 =A0 =A0 =A0struct pnfs_layoutcommit_res res;
>> =A0 =A0 =A0 =A0int status;
>> --
>> 1.6.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs"=
 in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> tao.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Tao Guo]

I see... But what if sync == 1? At that time, data must have already
been freed, so you should not use data->cred anymore.

-- 
tao.

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[William A. (Andy) Adamson]

On Mon, May 24, 2010 at 2:37 AM, Tao Guo <glorioustao@gmail.com> wrote:
> I see... But what if sync == 1? At that time, data must have already
> been freed, so you should not use data->cred anymore.

Agreed.

Since LAYOUTCOMMIT is not called after close, we can get rid of taking
the reference all together since a reference is taken on OPEN and
dropped on CLOSE.

-->Andy
>
> --
> tao.
>

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Tao Guo]

On Mon, May 24, 2010 at 9:43 PM, William A. (Andy) Adamson
<androsadamson@gmail.com> wrote:
> On Mon, May 24, 2010 at 2:37 AM, Tao Guo <glorioustao@gmail.com> wrote:
>> I see... But what if sync == 1? At that time, data must have already
>> been freed, so you should not use data->cred anymore.
>
> Agreed.
>
> Since LAYOUTCOMMIT is not called after close, we can get rid of taking
> the reference all together since a reference is taken on OPEN and
> dropped on CLOSE.
>
> -->Andy
I agree. However that will change several functions' interface I guess.

I still have a question: why some procedures need to set rpc_cred
whereas some needn't ? what is the connection between ctx's rpc_cred
and nfs4_state_owner's so_cred? Any guidance will be appreciated...

>>
>> --
>> tao.
>>
>



-- 
tao.

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[William A. (Andy) Adamson]

On Mon, May 24, 2010 at 12:02 PM, Tao Guo <glorioustao@gmail.com> wrote:
> On Mon, May 24, 2010 at 9:43 PM, William A. (Andy) Adamson
> <androsadamson@gmail.com> wrote:
>> On Mon, May 24, 2010 at 2:37 AM, Tao Guo <glorioustao@gmail.com> wrote:
>>> I see... But what if sync == 1? At that time, data must have already
>>> been freed, so you should not use data->cred anymore.
>>
>> Agreed.
>>
>> Since LAYOUTCOMMIT is not called after close, we can get rid of taking
>> the reference all together since a reference is taken on OPEN and
>> dropped on CLOSE.

I forgot memory mapped files where writes can occur after the close,
and so layoutcommit can also occur after the close. So, I will keep
the get_rpccred in pnfs_need_layout commit and resend this patch with
the put_rpccred in the layoutcommit done routine.

>>
>> -->Andy
> I agree. However that will change several functions' interface I guess.
>
> I still have a question: why some procedures need to set rpc_cred
> whereas some needn't ? what is the connection between ctx's rpc_cred
> and nfs4_state_owner's so_cred? Any guidance will be appreciated...

Here's my take on an answer...

If an rpc_cred is not specified in the rpc_msg then the fsuid, fsgid,
and group info toe the  current_cred is used to lookup an rpc_cred.
RPC's can be sent from several tasks - rpciod vrs the application task
for example. OPEN, CLOSE, LOCK, UNLOCK, READ, WRITE (and memmapped
I/O) should all be sent with the credential that opened the file. So,
in order to ensure this the nfs4_state_owner->so_cred is used for
these compounds even if the task is rpciod with root fsuid/fsgid. The
nfs_open_context rpc_cred is the nfs4_state_owner so_cred. The
nfs_open_context can exist longer than the nfs4_state_owner.

I think LAYOUTCOMMIT also wants to use the credential that did the I/O
- not for the file layout driver which doesn't care, but for the
object and block layout drivers that use the layout as a lock and
where a permissions check (?) is done on LAYOUTCOMMIT.

Currently, LAYOUTGET and LAYOUTRETURN don't set the rpc_cred. This is
because the layout can last past opens and closes. If we ever want to
support EXCHGID4_FLAG_BIND_PRINC_STATEID we will need to remember the
cred and set it for these calls.

-->Andy



>
>>>
>>> --
>>> tao.
>>>
>>
>
>
>
> --
> tao.
>

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Boaz Harrosh]

On 05/24/2010 08:25 PM, William A. (Andy) Adamson wrote:
> On Mon, May 24, 2010 at 12:02 PM, Tao Guo <glorioustao@gmail.com> wrote:
>> On Mon, May 24, 2010 at 9:43 PM, William A. (Andy) Adamson
>> <androsadamson@gmail.com> wrote:
>>> On Mon, May 24, 2010 at 2:37 AM, Tao Guo <glorioustao@gmail.com> wrote:
>>>> I see... But what if sync == 1? At that time, data must have already
>>>> been freed, so you should not use data->cred anymore.
>>>
>>> Agreed.
>>>
>>> Since LAYOUTCOMMIT is not called after close, we can get rid of taking
>>> the reference all together since a reference is taken on OPEN and
>>> dropped on CLOSE.
> 
> I forgot memory mapped files where writes can occur after the close,
> and so layoutcommit can also occur after the close. So, I will keep
> the get_rpccred in pnfs_need_layout commit and resend this patch with
> the put_rpccred in the layoutcommit done routine.
> 

Andy I get hangs on:
       data->is_sync = sync;
        status = pnfs4_proc_layoutcommit(data);
 +       put_rpccred(data->cred);

with sync==1 like Tao Guo said

could you send the updated patch?

Boaz

>>>
>>> -->Andy
>> I agree. However that will change several functions' interface I guess.
>>
>> I still have a question: why some procedures need to set rpc_cred
>> whereas some needn't ? what is the connection between ctx's rpc_cred
>> and nfs4_state_owner's so_cred? Any guidance will be appreciated...
> 
> Here's my take on an answer...
> 
> If an rpc_cred is not specified in the rpc_msg then the fsuid, fsgid,
> and group info toe the  current_cred is used to lookup an rpc_cred.
> RPC's can be sent from several tasks - rpciod vrs the application task
> for example. OPEN, CLOSE, LOCK, UNLOCK, READ, WRITE (and memmapped
> I/O) should all be sent with the credential that opened the file. So,
> in order to ensure this the nfs4_state_owner->so_cred is used for
> these compounds even if the task is rpciod with root fsuid/fsgid. The
> nfs_open_context rpc_cred is the nfs4_state_owner so_cred. The
> nfs_open_context can exist longer than the nfs4_state_owner.
> 
> I think LAYOUTCOMMIT also wants to use the credential that did the I/O
> - not for the file layout driver which doesn't care, but for the
> object and block layout drivers that use the layout as a lock and
> where a permissions check (?) is done on LAYOUTCOMMIT.
> 
> Currently, LAYOUTGET and LAYOUTRETURN don't set the rpc_cred. This is
> because the layout can last past opens and closes. If we ever want to
> support EXCHGID4_FLAG_BIND_PRINC_STATEID we will need to remember the
> cred and set it for these calls.
> 
> -->Andy
> 
> 
> 
>>
>>>>
>>>> --
>>>> tao.
>>>>
>>>
>>
>>
>>
>> --
>> tao.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Andy Adamson]

On May 24, 2010, at 2:20 PM, Boaz Harrosh wrote:

> On 05/24/2010 08:25 PM, William A. (Andy) Adamson wrote:
>> On Mon, May 24, 2010 at 12:02 PM, Tao Guo <glorioustao@gmail.com>  
>> wrote:
>>> On Mon, May 24, 2010 at 9:43 PM, William A. (Andy) Adamson
>>> <androsadamson@gmail.com> wrote:
>>>> On Mon, May 24, 2010 at 2:37 AM, Tao Guo <glorioustao@gmail.com>  
>>>> wrote:
>>>>> I see... But what if sync == 1? At that time, data must have  
>>>>> already
>>>>> been freed, so you should not use data->cred anymore.
>>>>
>>>> Agreed.
>>>>
>>>> Since LAYOUTCOMMIT is not called after close, we can get rid of  
>>>> taking
>>>> the reference all together since a reference is taken on OPEN and
>>>> dropped on CLOSE.
>>
>> I forgot memory mapped files where writes can occur after the close,
>> and so layoutcommit can also occur after the close. So, I will keep
>> the get_rpccred in pnfs_need_layout commit and resend this patch with
>> the put_rpccred in the layoutcommit done routine.
>>
>
> Andy I get hangs on:
>       data->is_sync = sync;
>        status = pnfs4_proc_layoutcommit(data);
> +       put_rpccred(data->cred);
>
> with sync==1 like Tao Guo said
>
> could you send the updated patch?

On it's way
>
> Boaz
>
>>>>
>>>> -->Andy
>>> I agree. However that will change several functions' interface I  
>>> guess.
>>>
>>> I still have a question: why some procedures need to set rpc_cred
>>> whereas some needn't ? what is the connection between ctx's rpc_cred
>>> and nfs4_state_owner's so_cred? Any guidance will be appreciated...
>>
>> Here's my take on an answer...
>>
>> If an rpc_cred is not specified in the rpc_msg then the fsuid, fsgid,
>> and group info toe the  current_cred is used to lookup an rpc_cred.
>> RPC's can be sent from several tasks - rpciod vrs the application  
>> task
>> for example. OPEN, CLOSE, LOCK, UNLOCK, READ, WRITE (and memmapped
>> I/O) should all be sent with the credential that opened the file. So,
>> in order to ensure this the nfs4_state_owner->so_cred is used for
>> these compounds even if the task is rpciod with root fsuid/fsgid. The
>> nfs_open_context rpc_cred is the nfs4_state_owner so_cred. The
>> nfs_open_context can exist longer than the nfs4_state_owner.
>>
>> I think LAYOUTCOMMIT also wants to use the credential that did the  
>> I/O
>> - not for the file layout driver which doesn't care, but for the
>> object and block layout drivers that use the layout as a lock and
>> where a permissions check (?) is done on LAYOUTCOMMIT.
>>
>> Currently, LAYOUTGET and LAYOUTRETURN don't set the rpc_cred. This is
>> because the layout can last past opens and closes. If we ever want to
>> support EXCHGID4_FLAG_BIND_PRINC_STATEID we will need to remember the
>> cred and set it for these calls.
>>
>> -->Andy
>>
>>
>>
>>>
>>>>>
>>>>> --
>>>>> tao.
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> tao.
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"  
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[andros]

From: Andy Adamson <andros@netapp.com>

Taking a reference on the nfs_open_context to signal that a layoutcommit is
needed is problematic because the last reference to the context triggers a
close (nfs_release).  But, if the layout holds a reference on the
nfs_open_context, then close will not be triggered.

Since we only use the rpc credential from the layoutcommit_ctx, replace the
layoutcommit_ctx with the rpc_cred.

Hold a reference on the rpc_cred until the layoutcommit rpc returns.

If the layoutdriver fails to setup the layoutcommit, clear the layoutcommit
properties and put the credential.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/inode.c            |    4 ++--
 fs/nfs/nfs4state.c        |    2 +-
 fs/nfs/pnfs.c             |   42 +++++++++++++++++-------------------------
 fs/nfs/write.c            |    2 +-
 include/linux/nfs4_pnfs.h |    6 ++++++
 include/linux/nfs_fs.h    |    3 +--
 include/linux/pnfs_xdr.h  |    1 -
 7 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7fd33d9..2b8e8e6 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	/*
 	 * file needs layout commit, server attributes may be stale
 	 */
-	if (nfsi->layoutcommit_ctx && nfsi->change_attr >= fattr->change_attr) {
+	if (do_layoutcommit(nfsi) && nfsi->change_attr >= fattr->change_attr) {
 		dprintk("NFS: %s: layoutcommit is needed for file %s/%ld\n",
 			__func__, inode->i_sb->s_id, inode->i_ino);
 		return 0;
@@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
 	nfsi->pnfs_layout_state = 0;
 	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
 	nfsi->layout.roc_iomode = 0;
-	nfsi->layoutcommit_ctx = NULL;
+	nfsi->lo_cred = NULL;
 	nfsi->pnfs_write_begin_pos = 0;
 	nfsi->pnfs_write_end_pos = 0;
 #endif /* CONFIG_NFS_V4_1 */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index d145de1..bf03fde 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
 #ifdef CONFIG_NFS_V4_1
 		struct nfs_inode *nfsi = NFS_I(state->inode);
 
-		if (nfsi->layoutcommit_ctx)
+		if (do_layoutcommit(nfsi))
 			pnfs_layoutcommit_inode(state->inode, wait);
 		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
 			struct nfs4_pnfs_layout_segment range;
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 20285bc..ee530c8 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) {
 	return 0;
 }
 
-/* Set context to indicate we require a layoutcommit
+/* Set lo_cred to indicate we require a layoutcommit
  * If we don't even have a layout, we don't need to commit it.
  */
 void
 pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
 {
-	dprintk("%s: has_layout=%d layoutcommit_ctx=%p ctx=%p\n", __func__,
-		has_layout(nfsi), nfsi->layoutcommit_ctx, ctx);
+	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
 	spin_lock(&nfsi->lo_lock);
-	if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
-		nfsi->layoutcommit_ctx = get_nfs_open_context(ctx);
+	if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
+		nfsi->lo_cred = get_rpccred(ctx->state->owner->so_cred);
 		nfsi->change_attr++;
 		spin_unlock(&nfsi->lo_lock);
-		dprintk("%s: Set layoutcommit_ctx=%p\n", __func__,
-			nfsi->layoutcommit_ctx);
+		dprintk("%s: Set layoutcommit\n", __func__);
 		return;
 	}
 	spin_unlock(&nfsi->lo_lock);
@@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 				!pnfs_return_layout_barrier(nfsi, &arg));
 		}
 
-		if (nfsi->layoutcommit_ctx) {
+		if (do_layoutcommit(nfsi)) {
 			status = pnfs_layoutcommit_inode(ino, wait);
 			if (status) {
 				dprintk("%s: layoutcommit failed, status=%d. "
@@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
 
 	dprintk("%s: (status %d)\n", __func__, data->status);
 
-	/* TODO: For now, set an error in the open context (just like
-	 * if a commit failed) We may want to do more, much more, like
-	 * replay all writes through the NFSv4
-	 * server, or something.
-	 */
-	if (data->status < 0) {
+	if (data->status < 0)
 		printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
 		       __func__, data->status);
-		data->ctx->error = data->status;
-	}
 
 	/* TODO: Maybe we should avoid this by allowing the layout driver
 	 * to directly xdr its layout on the wire.
@@ -1929,9 +1920,7 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
 							&nfsi->layout,
 							&data->args,
 							data->status);
-
-	/* release the open_context acquired in pnfs_writeback_done */
-	put_nfs_open_context(data->ctx);
+	put_rpccred(data->cred);
 }
 
 /*
@@ -1995,24 +1984,27 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
 		return -ENOMEM;
 
 	spin_lock(&nfsi->lo_lock);
-	if (!nfsi->layoutcommit_ctx)
+	if (!do_layoutcommit(nfsi))
 		goto out_unlock;
 
 	data->args.inode = inode;
-	data->cred  = nfsi->layoutcommit_ctx->cred;
-	data->ctx = nfsi->layoutcommit_ctx;
+	data->cred = nfsi->lo_cred;
 
 	/* Set up layout commit args*/
 	status = pnfs_layoutcommit_setup(data, sync);
-	if (status)
-		goto out_unlock;
 
 	/* Clear layoutcommit properties in the inode so
 	 * new lc info can be generated
 	 */
 	nfsi->pnfs_write_begin_pos = 0;
 	nfsi->pnfs_write_end_pos = 0;
-	nfsi->layoutcommit_ctx = NULL;
+	nfsi->lo_cred = NULL;
+
+	if (status) {
+		/* The layout driver failed to setup the layoutcommit */
+		put_rpccred(data->cred);
+		goto out_unlock;
+	}
 
 	/* release lock on pnfs layoutcommit attrs */
 	spin_unlock(&nfsi->lo_lock);
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index a4c95a0..2c1918e 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode, int how)
 					nfs_wait_bit_killable,
 					TASK_KILLABLE);
 #ifdef CONFIG_NFS_V4_1
-		if (may_wait && NFS_I(inode)->layoutcommit_ctx) {
+		if (may_wait && do_layoutcommit(NFS_I(inode))) {
 			error = pnfs_layoutcommit_inode(inode, 1);
 			if (error < 0)
 				return error;
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index 4d47b48..c9ef43e 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
 	return nfsi->layout.ld_data != NULL;
 }
 
+static inline bool
+do_layoutcommit(struct nfs_inode *nfsi)
+{
+	return nfsi->lo_cred != NULL;
+}
+
 #endif /* CONFIG_NFS_V4_1 */
 
 struct pnfs_layout_segment {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 98a8dc0..478b00c 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -203,11 +203,10 @@ struct nfs_inode {
 #define NFS_INO_RW_LAYOUT_FAILED 1 	/* get rw layout failed stop trying */
 #define NFS_INO_LAYOUT_ALLOC     2	/* bit lock for layout allocation */
 	time_t pnfs_layout_suspend;
+	struct rpc_cred		*lo_cred; /* layoutcommit credential */
 	wait_queue_head_t lo_waitq;
 	spinlock_t lo_lock;
 	struct pnfs_layout_type layout;
-	/* use rpc_creds in this open_context to send LAYOUTCOMMIT to MDS */
-	struct nfs_open_context *layoutcommit_ctx;
 	/* DH: These vars keep track of the maximum write range
 	 * so the values can be used for layoutcommit.
 	 */
diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
index a0bf341..154b04e 100644
--- a/include/linux/pnfs_xdr.h
+++ b/include/linux/pnfs_xdr.h
@@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
 	bool is_sync;
 	struct rpc_cred *cred;
 	struct nfs_fattr fattr;
-	struct nfs_open_context *ctx;
 	struct pnfs_layoutcommit_arg args;
 	struct pnfs_layoutcommit_res res;
 	int status;
-- 
1.6.2.5

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Boaz Harrosh]

On 05/24/2010 09:28 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Taking a reference on the nfs_open_context to signal that a layoutcommit is
> needed is problematic because the last reference to the context triggers a
> close (nfs_release).  But, if the layout holds a reference on the
> nfs_open_context, then close will not be triggered.
> 
> Since we only use the rpc credential from the layoutcommit_ctx, replace the
> layoutcommit_ctx with the rpc_cred.
> 
> Hold a reference on the rpc_cred until the layoutcommit rpc returns.
> 
> If the layoutdriver fails to setup the layoutcommit, clear the layoutcommit
> properties and put the credential.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>

OK with this one I'm able to pass basic tests (git clone linux)

Missing is Tao's patch tho write.c::nfs_wb_all()

I've updated his patch ontop of this one and posting it (as separate email)

Thanks
Boaz

> ---
>  fs/nfs/inode.c            |    4 ++--
>  fs/nfs/nfs4state.c        |    2 +-
>  fs/nfs/pnfs.c             |   42 +++++++++++++++++-------------------------
>  fs/nfs/write.c            |    2 +-
>  include/linux/nfs4_pnfs.h |    6 ++++++
>  include/linux/nfs_fs.h    |    3 +--
>  include/linux/pnfs_xdr.h  |    1 -
>  7 files changed, 28 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 7fd33d9..2b8e8e6 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  	/*
>  	 * file needs layout commit, server attributes may be stale
>  	 */
> -	if (nfsi->layoutcommit_ctx && nfsi->change_attr >= fattr->change_attr) {
> +	if (do_layoutcommit(nfsi) && nfsi->change_attr >= fattr->change_attr) {
>  		dprintk("NFS: %s: layoutcommit is needed for file %s/%ld\n",
>  			__func__, inode->i_sb->s_id, inode->i_ino);
>  		return 0;
> @@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>  	nfsi->pnfs_layout_state = 0;
>  	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>  	nfsi->layout.roc_iomode = 0;
> -	nfsi->layoutcommit_ctx = NULL;
> +	nfsi->lo_cred = NULL;
>  	nfsi->pnfs_write_begin_pos = 0;
>  	nfsi->pnfs_write_end_pos = 0;
>  #endif /* CONFIG_NFS_V4_1 */
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index d145de1..bf03fde 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
>  #ifdef CONFIG_NFS_V4_1
>  		struct nfs_inode *nfsi = NFS_I(state->inode);
>  
> -		if (nfsi->layoutcommit_ctx)
> +		if (do_layoutcommit(nfsi))
>  			pnfs_layoutcommit_inode(state->inode, wait);
>  		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
>  			struct nfs4_pnfs_layout_segment range;
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 20285bc..ee530c8 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) {
>  	return 0;
>  }
>  
> -/* Set context to indicate we require a layoutcommit
> +/* Set lo_cred to indicate we require a layoutcommit
>   * If we don't even have a layout, we don't need to commit it.
>   */
>  void
>  pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
>  {
> -	dprintk("%s: has_layout=%d layoutcommit_ctx=%p ctx=%p\n", __func__,
> -		has_layout(nfsi), nfsi->layoutcommit_ctx, ctx);
> +	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
>  	spin_lock(&nfsi->lo_lock);
> -	if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
> -		nfsi->layoutcommit_ctx = get_nfs_open_context(ctx);
> +	if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
> +		nfsi->lo_cred = get_rpccred(ctx->state->owner->so_cred);
>  		nfsi->change_attr++;
>  		spin_unlock(&nfsi->lo_lock);
> -		dprintk("%s: Set layoutcommit_ctx=%p\n", __func__,
> -			nfsi->layoutcommit_ctx);
> +		dprintk("%s: Set layoutcommit\n", __func__);
>  		return;
>  	}
>  	spin_unlock(&nfsi->lo_lock);
> @@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
>  				!pnfs_return_layout_barrier(nfsi, &arg));
>  		}
>  
> -		if (nfsi->layoutcommit_ctx) {
> +		if (do_layoutcommit(nfsi)) {
>  			status = pnfs_layoutcommit_inode(ino, wait);
>  			if (status) {
>  				dprintk("%s: layoutcommit failed, status=%d. "
> @@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>  
>  	dprintk("%s: (status %d)\n", __func__, data->status);
>  
> -	/* TODO: For now, set an error in the open context (just like
> -	 * if a commit failed) We may want to do more, much more, like
> -	 * replay all writes through the NFSv4
> -	 * server, or something.
> -	 */
> -	if (data->status < 0) {
> +	if (data->status < 0)
>  		printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
>  		       __func__, data->status);
> -		data->ctx->error = data->status;
> -	}
>  
>  	/* TODO: Maybe we should avoid this by allowing the layout driver
>  	 * to directly xdr its layout on the wire.
> @@ -1929,9 +1920,7 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>  							&nfsi->layout,
>  							&data->args,
>  							data->status);
> -
> -	/* release the open_context acquired in pnfs_writeback_done */
> -	put_nfs_open_context(data->ctx);
> +	put_rpccred(data->cred);
>  }
>  
>  /*
> @@ -1995,24 +1984,27 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>  		return -ENOMEM;
>  
>  	spin_lock(&nfsi->lo_lock);
> -	if (!nfsi->layoutcommit_ctx)
> +	if (!do_layoutcommit(nfsi))
>  		goto out_unlock;
>  
>  	data->args.inode = inode;
> -	data->cred  = nfsi->layoutcommit_ctx->cred;
> -	data->ctx = nfsi->layoutcommit_ctx;
> +	data->cred = nfsi->lo_cred;
>  
>  	/* Set up layout commit args*/
>  	status = pnfs_layoutcommit_setup(data, sync);
> -	if (status)
> -		goto out_unlock;
>  
>  	/* Clear layoutcommit properties in the inode so
>  	 * new lc info can be generated
>  	 */
>  	nfsi->pnfs_write_begin_pos = 0;
>  	nfsi->pnfs_write_end_pos = 0;
> -	nfsi->layoutcommit_ctx = NULL;
> +	nfsi->lo_cred = NULL;
> +
> +	if (status) {
> +		/* The layout driver failed to setup the layoutcommit */
> +		put_rpccred(data->cred);
> +		goto out_unlock;
> +	}
>  
>  	/* release lock on pnfs layoutcommit attrs */
>  	spin_unlock(&nfsi->lo_lock);
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index a4c95a0..2c1918e 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode, int how)
>  					nfs_wait_bit_killable,
>  					TASK_KILLABLE);
>  #ifdef CONFIG_NFS_V4_1
> -		if (may_wait && NFS_I(inode)->layoutcommit_ctx) {
> +		if (may_wait && do_layoutcommit(NFS_I(inode))) {
>  			error = pnfs_layoutcommit_inode(inode, 1);
>  			if (error < 0)
>  				return error;
> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
> index 4d47b48..c9ef43e 100644
> --- a/include/linux/nfs4_pnfs.h
> +++ b/include/linux/nfs4_pnfs.h
> @@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
>  	return nfsi->layout.ld_data != NULL;
>  }
>  
> +static inline bool
> +do_layoutcommit(struct nfs_inode *nfsi)
> +{
> +	return nfsi->lo_cred != NULL;
> +}
> +
>  #endif /* CONFIG_NFS_V4_1 */
>  
>  struct pnfs_layout_segment {
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 98a8dc0..478b00c 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -203,11 +203,10 @@ struct nfs_inode {
>  #define NFS_INO_RW_LAYOUT_FAILED 1 	/* get rw layout failed stop trying */
>  #define NFS_INO_LAYOUT_ALLOC     2	/* bit lock for layout allocation */
>  	time_t pnfs_layout_suspend;
> +	struct rpc_cred		*lo_cred; /* layoutcommit credential */
>  	wait_queue_head_t lo_waitq;
>  	spinlock_t lo_lock;
>  	struct pnfs_layout_type layout;
> -	/* use rpc_creds in this open_context to send LAYOUTCOMMIT to MDS */
> -	struct nfs_open_context *layoutcommit_ctx;
>  	/* DH: These vars keep track of the maximum write range
>  	 * so the values can be used for layoutcommit.
>  	 */
> diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
> index a0bf341..154b04e 100644
> --- a/include/linux/pnfs_xdr.h
> +++ b/include/linux/pnfs_xdr.h
> @@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
>  	bool is_sync;
>  	struct rpc_cred *cred;
>  	struct nfs_fattr fattr;
> -	struct nfs_open_context *ctx;
>  	struct pnfs_layoutcommit_arg args;
>  	struct pnfs_layoutcommit_res res;
>  	int status;

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Benny Halevy]

On May. 24, 2010, 21:28 +0300, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Taking a reference on the nfs_open_context to signal that a layoutcommit is
> needed is problematic because the last reference to the context triggers a
> close (nfs_release).  But, if the layout holds a reference on the
> nfs_open_context, then close will not be triggered.
> 
> Since we only use the rpc credential from the layoutcommit_ctx, replace the
> layoutcommit_ctx with the rpc_cred.
> 
> Hold a reference on the rpc_cred until the layoutcommit rpc returns.
> 
> If the layoutdriver fails to setup the layoutcommit, clear the layoutcommit
> properties and put the credential.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/inode.c            |    4 ++--
>  fs/nfs/nfs4state.c        |    2 +-
>  fs/nfs/pnfs.c             |   42 +++++++++++++++++-------------------------
>  fs/nfs/write.c            |    2 +-
>  include/linux/nfs4_pnfs.h |    6 ++++++
>  include/linux/nfs_fs.h    |    3 +--
>  include/linux/pnfs_xdr.h  |    1 -
>  7 files changed, 28 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 7fd33d9..2b8e8e6 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  	/*
>  	 * file needs layout commit, server attributes may be stale
>  	 */
> -	if (nfsi->layoutcommit_ctx && nfsi->change_attr >= fattr->change_attr) {
> +	if (do_layoutcommit(nfsi) && nfsi->change_attr >= fattr->change_attr) {

Andy, the patch looks good overall, thanks!
One nit though: mind if I rename "do_layoutcommit" which reads as if it actually performs
the layoutcommit call to "layoutcommit_needed"?

Benny

>  		dprintk("NFS: %s: layoutcommit is needed for file %s/%ld\n",
>  			__func__, inode->i_sb->s_id, inode->i_ino);
>  		return 0;
> @@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>  	nfsi->pnfs_layout_state = 0;
>  	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>  	nfsi->layout.roc_iomode = 0;
> -	nfsi->layoutcommit_ctx = NULL;
> +	nfsi->lo_cred = NULL;
>  	nfsi->pnfs_write_begin_pos = 0;
>  	nfsi->pnfs_write_end_pos = 0;
>  #endif /* CONFIG_NFS_V4_1 */
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index d145de1..bf03fde 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
>  #ifdef CONFIG_NFS_V4_1
>  		struct nfs_inode *nfsi = NFS_I(state->inode);
>  
> -		if (nfsi->layoutcommit_ctx)
> +		if (do_layoutcommit(nfsi))
>  			pnfs_layoutcommit_inode(state->inode, wait);
>  		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
>  			struct nfs4_pnfs_layout_segment range;
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 20285bc..ee530c8 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) {
>  	return 0;
>  }
>  
> -/* Set context to indicate we require a layoutcommit
> +/* Set lo_cred to indicate we require a layoutcommit
>   * If we don't even have a layout, we don't need to commit it.
>   */
>  void
>  pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
>  {
> -	dprintk("%s: has_layout=%d layoutcommit_ctx=%p ctx=%p\n", __func__,
> -		has_layout(nfsi), nfsi->layoutcommit_ctx, ctx);
> +	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
>  	spin_lock(&nfsi->lo_lock);
> -	if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
> -		nfsi->layoutcommit_ctx = get_nfs_open_context(ctx);
> +	if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
> +		nfsi->lo_cred = get_rpccred(ctx->state->owner->so_cred);
>  		nfsi->change_attr++;
>  		spin_unlock(&nfsi->lo_lock);
> -		dprintk("%s: Set layoutcommit_ctx=%p\n", __func__,
> -			nfsi->layoutcommit_ctx);
> +		dprintk("%s: Set layoutcommit\n", __func__);
>  		return;
>  	}
>  	spin_unlock(&nfsi->lo_lock);
> @@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
>  				!pnfs_return_layout_barrier(nfsi, &arg));
>  		}
>  
> -		if (nfsi->layoutcommit_ctx) {
> +		if (do_layoutcommit(nfsi)) {
>  			status = pnfs_layoutcommit_inode(ino, wait);
>  			if (status) {
>  				dprintk("%s: layoutcommit failed, status=%d. "
> @@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>  
>  	dprintk("%s: (status %d)\n", __func__, data->status);
>  
> -	/* TODO: For now, set an error in the open context (just like
> -	 * if a commit failed) We may want to do more, much more, like
> -	 * replay all writes through the NFSv4
> -	 * server, or something.
> -	 */
> -	if (data->status < 0) {
> +	if (data->status < 0)
>  		printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
>  		       __func__, data->status);
> -		data->ctx->error = data->status;
> -	}
>  
>  	/* TODO: Maybe we should avoid this by allowing the layout driver
>  	 * to directly xdr its layout on the wire.
> @@ -1929,9 +1920,7 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>  							&nfsi->layout,
>  							&data->args,
>  							data->status);
> -
> -	/* release the open_context acquired in pnfs_writeback_done */
> -	put_nfs_open_context(data->ctx);
> +	put_rpccred(data->cred);
>  }
>  
>  /*
> @@ -1995,24 +1984,27 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>  		return -ENOMEM;
>  
>  	spin_lock(&nfsi->lo_lock);
> -	if (!nfsi->layoutcommit_ctx)
> +	if (!do_layoutcommit(nfsi))
>  		goto out_unlock;
>  
>  	data->args.inode = inode;
> -	data->cred  = nfsi->layoutcommit_ctx->cred;
> -	data->ctx = nfsi->layoutcommit_ctx;
> +	data->cred = nfsi->lo_cred;
>  
>  	/* Set up layout commit args*/
>  	status = pnfs_layoutcommit_setup(data, sync);
> -	if (status)
> -		goto out_unlock;
>  
>  	/* Clear layoutcommit properties in the inode so
>  	 * new lc info can be generated
>  	 */
>  	nfsi->pnfs_write_begin_pos = 0;
>  	nfsi->pnfs_write_end_pos = 0;
> -	nfsi->layoutcommit_ctx = NULL;
> +	nfsi->lo_cred = NULL;
> +
> +	if (status) {
> +		/* The layout driver failed to setup the layoutcommit */
> +		put_rpccred(data->cred);
> +		goto out_unlock;
> +	}
>  
>  	/* release lock on pnfs layoutcommit attrs */
>  	spin_unlock(&nfsi->lo_lock);
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index a4c95a0..2c1918e 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode, int how)
>  					nfs_wait_bit_killable,
>  					TASK_KILLABLE);
>  #ifdef CONFIG_NFS_V4_1
> -		if (may_wait && NFS_I(inode)->layoutcommit_ctx) {
> +		if (may_wait && do_layoutcommit(NFS_I(inode))) {
>  			error = pnfs_layoutcommit_inode(inode, 1);
>  			if (error < 0)
>  				return error;
> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
> index 4d47b48..c9ef43e 100644
> --- a/include/linux/nfs4_pnfs.h
> +++ b/include/linux/nfs4_pnfs.h
> @@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
>  	return nfsi->layout.ld_data != NULL;
>  }
>  
> +static inline bool
> +do_layoutcommit(struct nfs_inode *nfsi)
> +{
> +	return nfsi->lo_cred != NULL;
> +}
> +
>  #endif /* CONFIG_NFS_V4_1 */
>  
>  struct pnfs_layout_segment {
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 98a8dc0..478b00c 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -203,11 +203,10 @@ struct nfs_inode {
>  #define NFS_INO_RW_LAYOUT_FAILED 1 	/* get rw layout failed stop trying */
>  #define NFS_INO_LAYOUT_ALLOC     2	/* bit lock for layout allocation */
>  	time_t pnfs_layout_suspend;
> +	struct rpc_cred		*lo_cred; /* layoutcommit credential */
>  	wait_queue_head_t lo_waitq;
>  	spinlock_t lo_lock;
>  	struct pnfs_layout_type layout;
> -	/* use rpc_creds in this open_context to send LAYOUTCOMMIT to MDS */
> -	struct nfs_open_context *layoutcommit_ctx;
>  	/* DH: These vars keep track of the maximum write range
>  	 * so the values can be used for layoutcommit.
>  	 */
> diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
> index a0bf341..154b04e 100644
> --- a/include/linux/pnfs_xdr.h
> +++ b/include/linux/pnfs_xdr.h
> @@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
>  	bool is_sync;
>  	struct rpc_cred *cred;
>  	struct nfs_fattr fattr;
> -	struct nfs_open_context *ctx;
>  	struct pnfs_layoutcommit_arg args;
>  	struct pnfs_layoutcommit_res res;
>  	int status;

Re: [PATCH 1/1] SQUASHME pnfs-submit: replace layoutcommit_ctx with rpc_cred

[Benny Halevy]

On May. 25, 2010, 7:07 +0300, Benny Halevy <bhalevy@panasas.com> wrote:
> On May. 24, 2010, 21:28 +0300, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Taking a reference on the nfs_open_context to signal that a layoutcommit is
>> needed is problematic because the last reference to the context triggers a
>> close (nfs_release).  But, if the layout holds a reference on the
>> nfs_open_context, then close will not be triggered.
>>
>> Since we only use the rpc credential from the layoutcommit_ctx, replace the
>> layoutcommit_ctx with the rpc_cred.
>>
>> Hold a reference on the rpc_cred until the layoutcommit rpc returns.
>>
>> If the layoutdriver fails to setup the layoutcommit, clear the layoutcommit
>> properties and put the credential.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/inode.c            |    4 ++--
>>  fs/nfs/nfs4state.c        |    2 +-
>>  fs/nfs/pnfs.c             |   42 +++++++++++++++++-------------------------
>>  fs/nfs/write.c            |    2 +-
>>  include/linux/nfs4_pnfs.h |    6 ++++++
>>  include/linux/nfs_fs.h    |    3 +--
>>  include/linux/pnfs_xdr.h  |    1 -
>>  7 files changed, 28 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 7fd33d9..2b8e8e6 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1110,7 +1110,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>>  	/*
>>  	 * file needs layout commit, server attributes may be stale
>>  	 */
>> -	if (nfsi->layoutcommit_ctx && nfsi->change_attr >= fattr->change_attr) {
>> +	if (do_layoutcommit(nfsi) && nfsi->change_attr >= fattr->change_attr) {
> 
> Andy, the patch looks good overall, thanks!
> One nit though: mind if I rename "do_layoutcommit" which reads as if it actually performs
> the layoutcommit call to "layoutcommit_needed"?
> 
> Benny
> 

Committed at pnfs-all-2.6.34-2010-05-25
(with the renamed function)

Thanks!

Benny

>>  		dprintk("NFS: %s: layoutcommit is needed for file %s/%ld\n",
>>  			__func__, inode->i_sb->s_id, inode->i_ino);
>>  		return 0;
>> @@ -1331,7 +1331,7 @@ static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>>  	nfsi->pnfs_layout_state = 0;
>>  	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>>  	nfsi->layout.roc_iomode = 0;
>> -	nfsi->layoutcommit_ctx = NULL;
>> +	nfsi->lo_cred = NULL;
>>  	nfsi->pnfs_write_begin_pos = 0;
>>  	nfsi->pnfs_write_end_pos = 0;
>>  #endif /* CONFIG_NFS_V4_1 */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index d145de1..bf03fde 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -589,7 +589,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
>>  #ifdef CONFIG_NFS_V4_1
>>  		struct nfs_inode *nfsi = NFS_I(state->inode);
>>  
>> -		if (nfsi->layoutcommit_ctx)
>> +		if (do_layoutcommit(nfsi))
>>  			pnfs_layoutcommit_inode(state->inode, wait);
>>  		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
>>  			struct nfs4_pnfs_layout_segment range;
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 20285bc..ee530c8 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -145,21 +145,19 @@ find_pnfs(u32 id, struct pnfs_module **module) {
>>  	return 0;
>>  }
>>  
>> -/* Set context to indicate we require a layoutcommit
>> +/* Set lo_cred to indicate we require a layoutcommit
>>   * If we don't even have a layout, we don't need to commit it.
>>   */
>>  void
>>  pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
>>  {
>> -	dprintk("%s: has_layout=%d layoutcommit_ctx=%p ctx=%p\n", __func__,
>> -		has_layout(nfsi), nfsi->layoutcommit_ctx, ctx);
>> +	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
>>  	spin_lock(&nfsi->lo_lock);
>> -	if (has_layout(nfsi) && !nfsi->layoutcommit_ctx) {
>> -		nfsi->layoutcommit_ctx = get_nfs_open_context(ctx);
>> +	if (has_layout(nfsi) && !do_layoutcommit(nfsi)) {
>> +		nfsi->lo_cred = get_rpccred(ctx->state->owner->so_cred);
>>  		nfsi->change_attr++;
>>  		spin_unlock(&nfsi->lo_lock);
>> -		dprintk("%s: Set layoutcommit_ctx=%p\n", __func__,
>> -			nfsi->layoutcommit_ctx);
>> +		dprintk("%s: Set layoutcommit\n", __func__);
>>  		return;
>>  	}
>>  	spin_unlock(&nfsi->lo_lock);
>> @@ -755,7 +753,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
>>  				!pnfs_return_layout_barrier(nfsi, &arg));
>>  		}
>>  
>> -		if (nfsi->layoutcommit_ctx) {
>> +		if (do_layoutcommit(nfsi)) {
>>  			status = pnfs_layoutcommit_inode(ino, wait);
>>  			if (status) {
>>  				dprintk("%s: layoutcommit failed, status=%d. "
>> @@ -1910,16 +1908,9 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>>  
>>  	dprintk("%s: (status %d)\n", __func__, data->status);
>>  
>> -	/* TODO: For now, set an error in the open context (just like
>> -	 * if a commit failed) We may want to do more, much more, like
>> -	 * replay all writes through the NFSv4
>> -	 * server, or something.
>> -	 */
>> -	if (data->status < 0) {
>> +	if (data->status < 0)
>>  		printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
>>  		       __func__, data->status);
>> -		data->ctx->error = data->status;
>> -	}
>>  
>>  	/* TODO: Maybe we should avoid this by allowing the layout driver
>>  	 * to directly xdr its layout on the wire.
>> @@ -1929,9 +1920,7 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>>  							&nfsi->layout,
>>  							&data->args,
>>  							data->status);
>> -
>> -	/* release the open_context acquired in pnfs_writeback_done */
>> -	put_nfs_open_context(data->ctx);
>> +	put_rpccred(data->cred);
>>  }
>>  
>>  /*
>> @@ -1995,24 +1984,27 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>>  		return -ENOMEM;
>>  
>>  	spin_lock(&nfsi->lo_lock);
>> -	if (!nfsi->layoutcommit_ctx)
>> +	if (!do_layoutcommit(nfsi))
>>  		goto out_unlock;
>>  
>>  	data->args.inode = inode;
>> -	data->cred  = nfsi->layoutcommit_ctx->cred;
>> -	data->ctx = nfsi->layoutcommit_ctx;
>> +	data->cred = nfsi->lo_cred;
>>  
>>  	/* Set up layout commit args*/
>>  	status = pnfs_layoutcommit_setup(data, sync);
>> -	if (status)
>> -		goto out_unlock;
>>  
>>  	/* Clear layoutcommit properties in the inode so
>>  	 * new lc info can be generated
>>  	 */
>>  	nfsi->pnfs_write_begin_pos = 0;
>>  	nfsi->pnfs_write_end_pos = 0;
>> -	nfsi->layoutcommit_ctx = NULL;
>> +	nfsi->lo_cred = NULL;
>> +
>> +	if (status) {
>> +		/* The layout driver failed to setup the layoutcommit */
>> +		put_rpccred(data->cred);
>> +		goto out_unlock;
>> +	}
>>  
>>  	/* release lock on pnfs layoutcommit attrs */
>>  	spin_unlock(&nfsi->lo_lock);
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index a4c95a0..2c1918e 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -1490,7 +1490,7 @@ static int nfs_commit_inode(struct inode *inode, int how)
>>  					nfs_wait_bit_killable,
>>  					TASK_KILLABLE);
>>  #ifdef CONFIG_NFS_V4_1
>> -		if (may_wait && NFS_I(inode)->layoutcommit_ctx) {
>> +		if (may_wait && do_layoutcommit(NFS_I(inode))) {
>>  			error = pnfs_layoutcommit_inode(inode, 1);
>>  			if (error < 0)
>>  				return error;
>> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
>> index 4d47b48..c9ef43e 100644
>> --- a/include/linux/nfs4_pnfs.h
>> +++ b/include/linux/nfs4_pnfs.h
>> @@ -80,6 +80,12 @@ has_layout(struct nfs_inode *nfsi)
>>  	return nfsi->layout.ld_data != NULL;
>>  }
>>  
>> +static inline bool
>> +do_layoutcommit(struct nfs_inode *nfsi)
>> +{
>> +	return nfsi->lo_cred != NULL;
>> +}
>> +
>>  #endif /* CONFIG_NFS_V4_1 */
>>  
>>  struct pnfs_layout_segment {
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index 98a8dc0..478b00c 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -203,11 +203,10 @@ struct nfs_inode {
>>  #define NFS_INO_RW_LAYOUT_FAILED 1 	/* get rw layout failed stop trying */
>>  #define NFS_INO_LAYOUT_ALLOC     2	/* bit lock for layout allocation */
>>  	time_t pnfs_layout_suspend;
>> +	struct rpc_cred		*lo_cred; /* layoutcommit credential */
>>  	wait_queue_head_t lo_waitq;
>>  	spinlock_t lo_lock;
>>  	struct pnfs_layout_type layout;
>> -	/* use rpc_creds in this open_context to send LAYOUTCOMMIT to MDS */
>> -	struct nfs_open_context *layoutcommit_ctx;
>>  	/* DH: These vars keep track of the maximum write range
>>  	 * so the values can be used for layoutcommit.
>>  	 */
>> diff --git a/include/linux/pnfs_xdr.h b/include/linux/pnfs_xdr.h
>> index a0bf341..154b04e 100644
>> --- a/include/linux/pnfs_xdr.h
>> +++ b/include/linux/pnfs_xdr.h
>> @@ -86,7 +86,6 @@ struct pnfs_layoutcommit_data {
>>  	bool is_sync;
>>  	struct rpc_cred *cred;
>>  	struct nfs_fattr fattr;
>> -	struct nfs_open_context *ctx;
>>  	struct pnfs_layoutcommit_arg args;
>>  	struct pnfs_layoutcommit_res res;
>>  	int status;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html