[PATCH 1/3] pnfs-submit: separate locking from get and put of layout

[PATCH 1/3] pnfs-submit: separate locking from get and put of layout

[Fred Isaman]

This is needed by next patch that changes refcounting

Signed-off-by: Fred Isaman <iisaman@netapp.com>
---
 fs/nfs/pnfs.c |   59 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 025675b..ed4c72e 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -313,30 +313,20 @@ pnfs_unregister_layoutdriver(struct pnfs_layoutdriver_type *ld_type)
 #define BUG_ON_UNLOCKED_LO(lo) do {} while (0)
 #endif /* CONFIG_SMP */
 
-/*
- * get and lock nfsi->layout
- */
 static inline struct pnfs_layout_type *
-get_lock_current_layout(struct nfs_inode *nfsi)
+get_current_layout(struct nfs_inode *nfsi)
 {
-	struct pnfs_layout_type *lo;
+	struct pnfs_layout_type *lo = &nfsi->layout;
 
-	lo = &nfsi->layout;
-	spin_lock(&nfsi->vfs_inode.i_lock);
-	if (!lo->ld_data) {
-		spin_unlock(&nfsi->vfs_inode.i_lock);
+	BUG_ON_UNLOCKED_LO(lo);
+	if (!lo->ld_data)
 		return NULL;
-	}
-
 	lo->refcount++;
 	return lo;
 }
 
-/*
- * put and unlock nfs->layout
- */
 static inline void
-put_unlock_current_layout(struct pnfs_layout_type *lo)
+put_current_layout(struct pnfs_layout_type *lo)
 {
 	struct inode *inode = PNFS_INODE(lo);
 	struct nfs_client *clp;
@@ -358,7 +348,6 @@ put_unlock_current_layout(struct pnfs_layout_type *lo)
 		list_del_init(&lo->lo_layouts);
 		spin_unlock(&clp->cl_lock);
 	}
-	spin_unlock(&inode->i_lock);
 }
 
 void
@@ -370,7 +359,8 @@ pnfs_layout_release(struct pnfs_layout_type *lo,
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (range)
 		pnfs_free_layout(lo, range);
-	put_unlock_current_layout(lo);
+	put_current_layout(lo);
+	spin_unlock(&nfsi->vfs_inode.i_lock);
 	wake_up_all(&nfsi->lo_waitq);
 }
 
@@ -384,11 +374,13 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
 		.length = NFS4_MAX_UINT64,
 	};
 
-	lo = get_lock_current_layout(nfsi);
+	spin_lock(&nfsi->vfs_inode.i_lock);
+	lo = get_current_layout(nfsi);
 	if (lo) {
 		pnfs_free_layout(lo, &range);
-		put_unlock_current_layout(lo);
+		put_current_layout(lo);
 	}
+	spin_unlock(&nfsi->vfs_inode.i_lock);
 }
 
 static inline void
@@ -751,12 +743,14 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 	arg.length = NFS4_MAX_UINT64;
 
 	if (type == RETURN_FILE) {
-		lo = get_lock_current_layout(nfsi);
+		spin_lock(&ino->i_lock);
+		lo = get_current_layout(nfsi);
 		if (lo && !has_layout_to_return(lo, &arg)) {
-			put_unlock_current_layout(lo);
+			put_current_layout(lo);
 			lo = NULL;
 		}
 		if (!lo) {
+			spin_unlock(&ino->i_lock);
 			dprintk("%s: no layout segments to return\n", __func__);
 			goto out;
 		}
@@ -770,7 +764,8 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 			if (stateid) { /* callback */
 				status = -EAGAIN;
 				spin_lock(&ino->i_lock);
-				put_unlock_current_layout(lo);
+				put_current_layout(lo);
+				spin_unlock(&ino->i_lock);
 				goto out;
 			}
 			dprintk("%s: waiting\n", __func__);
@@ -914,8 +909,7 @@ static int pnfs_wait_schedule(void *word)
  * get, possibly allocate, and lock current_layout
  *
  * Note: If successful, ino->i_lock is taken and the caller
- * must put and unlock current_layout by using put_unlock_current_layout()
- * when the returned layout is released.
+ * must put and unlock current_layout when the returned layout is released.
  */
 static struct pnfs_layout_type *
 get_lock_alloc_layout(struct inode *ino)
@@ -926,7 +920,9 @@ get_lock_alloc_layout(struct inode *ino)
 
 	dprintk("%s Begin\n", __func__);
 
-	while ((lo = get_lock_current_layout(nfsi)) == NULL) {
+	spin_lock(&ino->i_lock);
+	while ((lo = get_current_layout(nfsi)) == NULL) {
+		spin_unlock(&ino->i_lock);
 		/* Compete against other threads on who's doing the allocation,
 		 * wait until bit is cleared if we lost this race.
 		 */
@@ -942,8 +938,10 @@ get_lock_alloc_layout(struct inode *ino)
 		/* Was current_layout already allocated while we slept?
 		 * If so, retry get_lock'ing it. Otherwise, allocate it.
 		 */
-		if (nfsi->layout.ld_data)
+		if (nfsi->layout.ld_data) {
+			spin_lock(&ino->i_lock);
 			continue;
+		}
 
 		lo = alloc_init_layout(ino);
 		if (lo) {
@@ -1112,7 +1110,8 @@ out:
 out_put:
 	if (lsegpp)
 		*lsegpp = lseg;
-	put_unlock_current_layout(lo);
+	put_current_layout(lo);
+	spin_unlock(&ino->i_lock);
 	goto out;
 }
 
@@ -1328,11 +1327,13 @@ pnfs_getboundary(struct inode *inode)
 		goto out;
 
 	nfsi = NFS_I(inode);
-	lo = get_lock_current_layout(nfsi);;
+	spin_lock(&inode->i_lock);
+	lo = get_current_layout(nfsi);;
 	if (lo) {
 		stripe_size = policy_ops->get_stripesize(lo);
-		put_unlock_current_layout(lo);
+		put_current_layout(lo);
 	}
+	spin_unlock(&inode->i_lock);
 out:
 	return stripe_size;
 }
-- 
1.6.6.1

[PATCH 2/3] pnfs-submit: split get_layout and grab_current_layout

[Fred Isaman]

Split get_layout (inc refcount) and grab_current_layout (find layout and inc refcount)
functionality and rename appropriately.

Signed-off-by: Fred Isaman <iisaman@netapp.com>
---
 fs/nfs/pnfs.c |   37 ++++++++++++++++++++++---------------
 1 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index ed4c72e..49093a0 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -313,20 +313,27 @@ pnfs_unregister_layoutdriver(struct pnfs_layoutdriver_type *ld_type)
 #define BUG_ON_UNLOCKED_LO(lo) do {} while (0)
 #endif /* CONFIG_SMP */
 
+static inline void
+get_layout(struct pnfs_layout_type *lo)
+{
+	BUG_ON_UNLOCKED_LO(lo);
+	lo->refcount++;
+}
+
 static inline struct pnfs_layout_type *
-get_current_layout(struct nfs_inode *nfsi)
+grab_current_layout(struct nfs_inode *nfsi)
 {
 	struct pnfs_layout_type *lo = &nfsi->layout;
 
 	BUG_ON_UNLOCKED_LO(lo);
 	if (!lo->ld_data)
 		return NULL;
-	lo->refcount++;
+	get_layout(lo);
 	return lo;
 }
 
 static inline void
-put_current_layout(struct pnfs_layout_type *lo)
+put_layout(struct pnfs_layout_type *lo)
 {
 	struct inode *inode = PNFS_INODE(lo);
 	struct nfs_client *clp;
@@ -359,7 +366,7 @@ pnfs_layout_release(struct pnfs_layout_type *lo,
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (range)
 		pnfs_free_layout(lo, range);
-	put_current_layout(lo);
+	put_layout(lo);
 	spin_unlock(&nfsi->vfs_inode.i_lock);
 	wake_up_all(&nfsi->lo_waitq);
 }
@@ -375,10 +382,10 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
 	};
 
 	spin_lock(&nfsi->vfs_inode.i_lock);
-	lo = get_current_layout(nfsi);
+	lo = grab_current_layout(nfsi);
 	if (lo) {
 		pnfs_free_layout(lo, &range);
-		put_current_layout(lo);
+		put_layout(lo);
 	}
 	spin_unlock(&nfsi->vfs_inode.i_lock);
 }
@@ -548,7 +555,7 @@ pnfs_layout_from_open_stateid(nfs4_stateid *dst, struct nfs4_state *state)
 *    arg->length: all ones
 */
 static int
-get_layout(struct inode *ino,
+send_layoutget(struct inode *ino,
 	   struct nfs_open_context *ctx,
 	   struct nfs4_pnfs_layout_segment *range,
 	   struct pnfs_layout_segment **lsegpp,
@@ -744,9 +751,9 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 
 	if (type == RETURN_FILE) {
 		spin_lock(&ino->i_lock);
-		lo = get_current_layout(nfsi);
+		lo = grab_current_layout(nfsi);
 		if (lo && !has_layout_to_return(lo, &arg)) {
-			put_current_layout(lo);
+			put_layout(lo);
 			lo = NULL;
 		}
 		if (!lo) {
@@ -764,7 +771,7 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 			if (stateid) { /* callback */
 				status = -EAGAIN;
 				spin_lock(&ino->i_lock);
-				put_current_layout(lo);
+				put_layout(lo);
 				spin_unlock(&ino->i_lock);
 				goto out;
 			}
@@ -921,7 +928,7 @@ get_lock_alloc_layout(struct inode *ino)
 	dprintk("%s Begin\n", __func__);
 
 	spin_lock(&ino->i_lock);
-	while ((lo = get_current_layout(nfsi)) == NULL) {
+	while ((lo = grab_current_layout(nfsi)) == NULL) {
 		spin_unlock(&ino->i_lock);
 		/* Compete against other threads on who's doing the allocation,
 		 * wait until bit is cleared if we lost this race.
@@ -1102,7 +1109,7 @@ _pnfs_update_layout(struct inode *ino,
 	/* Lose lock, but not reference, match this with pnfs_layout_release */
 	spin_unlock(&ino->i_lock);
 
-	get_layout(ino, ctx, &arg, lsegpp, lo);
+	send_layoutget(ino, ctx, &arg, lsegpp, lo);
 out:
 	dprintk("%s end, state 0x%lx lseg %p\n", __func__,
 		nfsi->layout.pnfs_layout_state, lseg);
@@ -1110,7 +1117,7 @@ out:
 out_put:
 	if (lsegpp)
 		*lsegpp = lseg;
-	put_current_layout(lo);
+	put_layout(lo);
 	spin_unlock(&ino->i_lock);
 	goto out;
 }
@@ -1328,10 +1335,10 @@ pnfs_getboundary(struct inode *inode)
 
 	nfsi = NFS_I(inode);
 	spin_lock(&inode->i_lock);
-	lo = get_current_layout(nfsi);;
+	lo = grab_current_layout(nfsi);;
 	if (lo) {
 		stripe_size = policy_ops->get_stripesize(lo);
-		put_current_layout(lo);
+		put_layout(lo);
 	}
 	spin_unlock(&inode->i_lock);
 out:
-- 
1.6.6.1

[PATCH 3/3] pnfs-submit: remove list_empty check from put_layout

[Fred Isaman]

The check on list empty before destroying a layout has always bothered me.
Get rid of it by having lsegs take a reference on their backpointer.

Signed-off-by: Fred Isaman <iisaman@netapp.com>
---
 fs/nfs/pnfs.c |   55 +++++++++++++++++++++++++++++++++----------------------
 1 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 49093a0..c939cb0 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -60,6 +60,7 @@ static int pnfs_initialized;
 static void pnfs_free_layout(struct pnfs_layout_type *lo,
 			     struct nfs4_pnfs_layout_segment *range);
 static enum pnfs_try_status pnfs_commit(struct nfs_write_data *data, int sync);
+static inline void get_layout(struct pnfs_layout_type *lo);
 
 /* Locking:
  *
@@ -153,6 +154,7 @@ pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (has_layout(nfsi) && !layoutcommit_needed(nfsi)) {
 		nfsi->layout.lo_cred = get_rpccred(ctx->state->owner->so_cred);
+		get_layout(&nfsi->layout);
 		nfsi->change_attr++;
 		spin_unlock(&nfsi->vfs_inode.i_lock);
 		dprintk("%s: Set layoutcommit\n", __func__);
@@ -335,25 +337,18 @@ grab_current_layout(struct nfs_inode *nfsi)
 static inline void
 put_layout(struct pnfs_layout_type *lo)
 {
-	struct inode *inode = PNFS_INODE(lo);
-	struct nfs_client *clp;
-
 	BUG_ON_UNLOCKED_LO(lo);
 	BUG_ON(lo->refcount <= 0);
 
-	if (--lo->refcount == 0 && list_empty(&lo->segs)) {
+	lo->refcount--;
+	if (!lo->refcount) {
 		struct layoutdriver_io_operations *io_ops =
 			PNFS_LD_IO_OPS(lo);
 
 		dprintk("%s: freeing layout %p\n", __func__, lo->ld_data);
+		WARN_ON(!list_empty(&lo->lo_layouts));
 		io_ops->free_layout(lo->ld_data);
 		lo->ld_data = NULL;
-
-		/* Unlist the layout. */
-		clp = NFS_SERVER(inode)->nfs_client;
-		spin_lock(&clp->cl_lock);
-		list_del_init(&lo->lo_layouts);
-		spin_unlock(&clp->cl_lock);
 	}
 }
 
@@ -397,6 +392,7 @@ init_lseg(struct pnfs_layout_type *lo, struct pnfs_layout_segment *lseg)
 	kref_init(&lseg->kref);
 	lseg->valid = true;
 	lseg->layout = lo;
+	get_layout(lo);
 }
 
 static void
@@ -406,6 +402,7 @@ destroy_lseg(struct kref *kref)
 		container_of(kref, struct pnfs_layout_segment, kref);
 
 	dprintk("--> %s\n", __func__);
+	put_layout(lseg->layout);
 	PNFS_LD_IO_OPS(lseg->layout)->free_lseg(lseg);
 }
 
@@ -669,6 +666,15 @@ pnfs_free_layout(struct pnfs_layout_type *lo,
 			lseg->range.length);
 		list_del(&lseg->fi_list);
 		put_lseg_locked(lseg);
+		if (list_empty(&lo->segs)) {
+			struct nfs_client *clp;
+
+			clp = PNFS_NFS_SERVER(lo)->nfs_client;
+			spin_lock(&clp->cl_lock);
+			list_del_init(&lo->lo_layouts);
+			spin_unlock(&clp->cl_lock);
+			put_layout(lo);
+		}
 	}
 
 	dprintk("%s:Return\n", __func__);
@@ -854,6 +860,15 @@ pnfs_insert_layout(struct pnfs_layout_type *lo,
 	dprintk("%s:Begin\n", __func__);
 
 	BUG_ON_UNLOCKED_LO(lo);
+	if (list_empty(&lo->segs)) {
+		struct nfs_client *clp = PNFS_NFS_SERVER(lo)->nfs_client;
+
+		spin_lock(&clp->cl_lock);
+		BUG_ON(!list_empty(&lo->lo_layouts));
+		list_add_tail(&lo->lo_layouts, &clp->cl_layouts);
+		spin_unlock(&clp->cl_lock);
+		get_layout(lo);
+	}
 	list_for_each_entry (lp, &lo->segs, fi_list) {
 		if (cmp_layout(&lp->range, &lseg->range) > 0)
 			continue;
@@ -896,11 +911,13 @@ alloc_init_layout(struct inode *ino)
 		return NULL;
 	}
 
+	spin_lock(&ino->i_lock);
 	BUG_ON(lo->ld_data != NULL);
 	lo->ld_data = ld_data;
 	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
 	lo->refcount = 1;
 	lo->roc_iomode = 0;
+	spin_unlock(&ino->i_lock);
 	return lo;
 }
 
@@ -951,17 +968,9 @@ get_lock_alloc_layout(struct inode *ino)
 		}
 
 		lo = alloc_init_layout(ino);
-		if (lo) {
-			struct nfs_client *clp = NFS_SERVER(ino)->nfs_client;
-
-			/* must grab the layout lock before the client lock */
+		if (lo)
 			spin_lock(&ino->i_lock);
-
-			spin_lock(&clp->cl_lock);
-			if (list_empty(&lo->lo_layouts))
-				list_add_tail(&lo->lo_layouts, &clp->cl_layouts);
-			spin_unlock(&clp->cl_lock);
-		} else
+		else
 			lo = ERR_PTR(-ENOMEM);
 
 		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
@@ -1247,14 +1256,13 @@ pnfs_layout_process(struct nfs4_pnfs_layoutget *lgp)
 		goto out;
 	}
 
+	spin_lock(&ino->i_lock);
 	init_lseg(lo, lseg);
 	lseg->range = res->lseg;
 	if (lgp->lsegpp) {
 		get_lseg(lseg);
 		*lgp->lsegpp = lseg;
 	}
-
-	spin_lock(&ino->i_lock);
 	pnfs_insert_layout(lo, lseg);
 
 	if (res->return_on_close) {
@@ -1642,6 +1650,9 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
 	}
 	status = pnfs4_proc_layoutcommit(data, sync);
 out:
+	spin_lock(&inode->i_lock);
+	put_layout(&nfsi->layout);
+	spin_unlock(&inode->i_lock);
 	dprintk("%s end (err:%d)\n", __func__, status);
 	return status;
 out_free:
-- 
1.6.6.1

Re: [PATCH 3/3] pnfs-submit: remove list_empty check from put_layout

[Benny Halevy]

On 2010-06-16 19:44, Fred Isaman wrote:
> The check on list empty before destroying a layout has always bothered me.
> Get rid of it by having lsegs take a reference on their backpointer.
> 
> Signed-off-by: Fred Isaman <iisaman@netapp.com>
> ---
>  fs/nfs/pnfs.c |   55 +++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 49093a0..c939cb0 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -60,6 +60,7 @@ static int pnfs_initialized;
>  static void pnfs_free_layout(struct pnfs_layout_type *lo,
>  			     struct nfs4_pnfs_layout_segment *range);
>  static enum pnfs_try_status pnfs_commit(struct nfs_write_data *data, int sync);
> +static inline void get_layout(struct pnfs_layout_type *lo);
>  
>  /* Locking:
>   *
> @@ -153,6 +154,7 @@ pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
>  	spin_lock(&nfsi->vfs_inode.i_lock);
>  	if (has_layout(nfsi) && !layoutcommit_needed(nfsi)) {
>  		nfsi->layout.lo_cred = get_rpccred(ctx->state->owner->so_cred);
> +		get_layout(&nfsi->layout);

looks like a fallout from a different patch?

>  		nfsi->change_attr++;
>  		spin_unlock(&nfsi->vfs_inode.i_lock);
>  		dprintk("%s: Set layoutcommit\n", __func__);
> @@ -335,25 +337,18 @@ grab_current_layout(struct nfs_inode *nfsi)
>  static inline void
>  put_layout(struct pnfs_layout_type *lo)
>  {
> -	struct inode *inode = PNFS_INODE(lo);
> -	struct nfs_client *clp;
> -
>  	BUG_ON_UNLOCKED_LO(lo);
>  	BUG_ON(lo->refcount <= 0);
>  
> -	if (--lo->refcount == 0 && list_empty(&lo->segs)) {
> +	lo->refcount--;
> +	if (!lo->refcount) {
>  		struct layoutdriver_io_operations *io_ops =
>  			PNFS_LD_IO_OPS(lo);
>  
>  		dprintk("%s: freeing layout %p\n", __func__, lo->ld_data);
> +		WARN_ON(!list_empty(&lo->lo_layouts));
>  		io_ops->free_layout(lo->ld_data);
>  		lo->ld_data = NULL;
> -
> -		/* Unlist the layout. */
> -		clp = NFS_SERVER(inode)->nfs_client;
> -		spin_lock(&clp->cl_lock);
> -		list_del_init(&lo->lo_layouts);
> -		spin_unlock(&clp->cl_lock);
>  	}
>  }
>  
> @@ -397,6 +392,7 @@ init_lseg(struct pnfs_layout_type *lo, struct pnfs_layout_segment *lseg)
>  	kref_init(&lseg->kref);
>  	lseg->valid = true;
>  	lseg->layout = lo;
> +	get_layout(lo);
>  }
>  
>  static void
> @@ -406,6 +402,7 @@ destroy_lseg(struct kref *kref)
>  		container_of(kref, struct pnfs_layout_segment, kref);
>  
>  	dprintk("--> %s\n", __func__);
> +	put_layout(lseg->layout);
>  	PNFS_LD_IO_OPS(lseg->layout)->free_lseg(lseg);
>  }
>  
> @@ -669,6 +666,15 @@ pnfs_free_layout(struct pnfs_layout_type *lo,
>  			lseg->range.length);
>  		list_del(&lseg->fi_list);
>  		put_lseg_locked(lseg);
> +		if (list_empty(&lo->segs)) {
> +			struct nfs_client *clp;
> +
> +			clp = PNFS_NFS_SERVER(lo)->nfs_client;
> +			spin_lock(&clp->cl_lock);
> +			list_del_init(&lo->lo_layouts);
> +			spin_unlock(&clp->cl_lock);
> +			put_layout(lo);
> +		}

How about doing this outside the list_for_each_entry_safe loop?
I don't see a need for checking after every list_del...

>  	}
>  
>  	dprintk("%s:Return\n", __func__);
> @@ -854,6 +860,15 @@ pnfs_insert_layout(struct pnfs_layout_type *lo,
>  	dprintk("%s:Begin\n", __func__);
>  
>  	BUG_ON_UNLOCKED_LO(lo);
> +	if (list_empty(&lo->segs)) {
> +		struct nfs_client *clp = PNFS_NFS_SERVER(lo)->nfs_client;
> +
> +		spin_lock(&clp->cl_lock);
> +		BUG_ON(!list_empty(&lo->lo_layouts));
> +		list_add_tail(&lo->lo_layouts, &clp->cl_layouts);
> +		spin_unlock(&clp->cl_lock);
> +		get_layout(lo);
> +	}
>  	list_for_each_entry (lp, &lo->segs, fi_list) {
>  		if (cmp_layout(&lp->range, &lseg->range) > 0)
>  			continue;
> @@ -896,11 +911,13 @@ alloc_init_layout(struct inode *ino)
>  		return NULL;
>  	}
>  
> +	spin_lock(&ino->i_lock);
>  	BUG_ON(lo->ld_data != NULL);
>  	lo->ld_data = ld_data;
>  	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
>  	lo->refcount = 1;
>  	lo->roc_iomode = 0;
> +	spin_unlock(&ino->i_lock);

this hunk seems unrelated to this patch, no?

>  	return lo;
>  }
>  
> @@ -951,17 +968,9 @@ get_lock_alloc_layout(struct inode *ino)
>  		}
>  
>  		lo = alloc_init_layout(ino);
> -		if (lo) {
> -			struct nfs_client *clp = NFS_SERVER(ino)->nfs_client;
> -
> -			/* must grab the layout lock before the client lock */
> +		if (lo)
>  			spin_lock(&ino->i_lock);
> -
> -			spin_lock(&clp->cl_lock);
> -			if (list_empty(&lo->lo_layouts))
> -				list_add_tail(&lo->lo_layouts, &clp->cl_layouts);
> -			spin_unlock(&clp->cl_lock);
> -		} else
> +		else
>  			lo = ERR_PTR(-ENOMEM);
>  
>  		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
> @@ -1247,14 +1256,13 @@ pnfs_layout_process(struct nfs4_pnfs_layoutget *lgp)
>  		goto out;
>  	}
>  
> +	spin_lock(&ino->i_lock);
>  	init_lseg(lo, lseg);
>  	lseg->range = res->lseg;
>  	if (lgp->lsegpp) {
>  		get_lseg(lseg);
>  		*lgp->lsegpp = lseg;
>  	}
> -
> -	spin_lock(&ino->i_lock);
>  	pnfs_insert_layout(lo, lseg);
>  
>  	if (res->return_on_close) {
> @@ -1642,6 +1650,9 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>  	}
>  	status = pnfs4_proc_layoutcommit(data, sync);
>  out:
> +	spin_lock(&inode->i_lock);
> +	put_layout(&nfsi->layout);
> +	spin_unlock(&inode->i_lock);

same fallout as earlier?

Otherwise, the patchset looks good!

Benny

>  	dprintk("%s end (err:%d)\n", __func__, status);
>  	return status;
>  out_free:

[PATCH 3/3] pnfs-submit: remove list_empty check from put_layout

[Fred Isaman]

The check on list empty before destroying a layout has always bothered me.
Get rid of it by having lsegs take a reference on their backpointer.

Signed-off-by: Fred Isaman <iisaman@netapp.com>
---
 fs/nfs/pnfs.c |   51 +++++++++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 49093a0..9a295cf 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -60,6 +60,7 @@ static int pnfs_initialized;
 static void pnfs_free_layout(struct pnfs_layout_type *lo,
 			     struct nfs4_pnfs_layout_segment *range);
 static enum pnfs_try_status pnfs_commit(struct nfs_write_data *data, int sync);
+static inline void get_layout(struct pnfs_layout_type *lo);
 
 /* Locking:
  *
@@ -335,25 +336,18 @@ grab_current_layout(struct nfs_inode *nfsi)
 static inline void
 put_layout(struct pnfs_layout_type *lo)
 {
-	struct inode *inode = PNFS_INODE(lo);
-	struct nfs_client *clp;
-
 	BUG_ON_UNLOCKED_LO(lo);
 	BUG_ON(lo->refcount <= 0);
 
-	if (--lo->refcount == 0 && list_empty(&lo->segs)) {
+	lo->refcount--;
+	if (!lo->refcount) {
 		struct layoutdriver_io_operations *io_ops =
 			PNFS_LD_IO_OPS(lo);
 
 		dprintk("%s: freeing layout %p\n", __func__, lo->ld_data);
+		WARN_ON(!list_empty(&lo->lo_layouts));
 		io_ops->free_layout(lo->ld_data);
 		lo->ld_data = NULL;
-
-		/* Unlist the layout. */
-		clp = NFS_SERVER(inode)->nfs_client;
-		spin_lock(&clp->cl_lock);
-		list_del_init(&lo->lo_layouts);
-		spin_unlock(&clp->cl_lock);
 	}
 }
 
@@ -397,6 +391,7 @@ init_lseg(struct pnfs_layout_type *lo, struct pnfs_layout_segment *lseg)
 	kref_init(&lseg->kref);
 	lseg->valid = true;
 	lseg->layout = lo;
+	get_layout(lo);
 }
 
 static void
@@ -406,6 +401,7 @@ destroy_lseg(struct kref *kref)
 		container_of(kref, struct pnfs_layout_segment, kref);
 
 	dprintk("--> %s\n", __func__);
+	put_layout(lseg->layout);
 	PNFS_LD_IO_OPS(lseg->layout)->free_lseg(lseg);
 }
 
@@ -670,6 +666,15 @@ pnfs_free_layout(struct pnfs_layout_type *lo,
 		list_del(&lseg->fi_list);
 		put_lseg_locked(lseg);
 	}
+	if (list_empty(&lo->segs)) {
+		struct nfs_client *clp;
+
+		clp = PNFS_NFS_SERVER(lo)->nfs_client;
+		spin_lock(&clp->cl_lock);
+		list_del_init(&lo->lo_layouts);
+		spin_unlock(&clp->cl_lock);
+		put_layout(lo);
+	}
 
 	dprintk("%s:Return\n", __func__);
 }
@@ -854,6 +859,15 @@ pnfs_insert_layout(struct pnfs_layout_type *lo,
 	dprintk("%s:Begin\n", __func__);
 
 	BUG_ON_UNLOCKED_LO(lo);
+	if (list_empty(&lo->segs)) {
+		struct nfs_client *clp = PNFS_NFS_SERVER(lo)->nfs_client;
+
+		spin_lock(&clp->cl_lock);
+		BUG_ON(!list_empty(&lo->lo_layouts));
+		list_add_tail(&lo->lo_layouts, &clp->cl_layouts);
+		spin_unlock(&clp->cl_lock);
+		get_layout(lo);
+	}
 	list_for_each_entry (lp, &lo->segs, fi_list) {
 		if (cmp_layout(&lp->range, &lseg->range) > 0)
 			continue;
@@ -896,11 +910,13 @@ alloc_init_layout(struct inode *ino)
 		return NULL;
 	}
 
+	spin_lock(&ino->i_lock);
 	BUG_ON(lo->ld_data != NULL);
 	lo->ld_data = ld_data;
 	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
 	lo->refcount = 1;
 	lo->roc_iomode = 0;
+	spin_unlock(&ino->i_lock);
 	return lo;
 }
 
@@ -951,17 +967,9 @@ get_lock_alloc_layout(struct inode *ino)
 		}
 
 		lo = alloc_init_layout(ino);
-		if (lo) {
-			struct nfs_client *clp = NFS_SERVER(ino)->nfs_client;
-
-			/* must grab the layout lock before the client lock */
+		if (lo)
 			spin_lock(&ino->i_lock);
-
-			spin_lock(&clp->cl_lock);
-			if (list_empty(&lo->lo_layouts))
-				list_add_tail(&lo->lo_layouts, &clp->cl_layouts);
-			spin_unlock(&clp->cl_lock);
-		} else
+		else
 			lo = ERR_PTR(-ENOMEM);
 
 		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
@@ -1247,14 +1255,13 @@ pnfs_layout_process(struct nfs4_pnfs_layoutget *lgp)
 		goto out;
 	}
 
+	spin_lock(&ino->i_lock);
 	init_lseg(lo, lseg);
 	lseg->range = res->lseg;
 	if (lgp->lsegpp) {
 		get_lseg(lseg);
 		*lgp->lsegpp = lseg;
 	}
-
-	spin_lock(&ino->i_lock);
 	pnfs_insert_layout(lo, lseg);
 
 	if (res->return_on_close) {
-- 
1.6.6.1