[PATCH 0/3] pnfs refcount patches v3

[PATCH 0/3] pnfs refcount patches v3

[Fred Isaman]

Changes layout refcounting.

This version includes changes to patch 3 in response to Benny's comments.

Fred

[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 |   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

Re: [PATCH 0/3] pnfs refcount patches v3

[Benny Halevy]

All merged in pnfs-all-2.6.35-rc3-2010-06-21

Thanks!

Benny

On Jun. 21, 2010, 16:16 +0300, Fred Isaman <iisaman@netapp.com> wrote:
> Changes layout refcounting.
> 
> This version includes changes to patch 3 in response to Benny's comments.
> 
> Fred
> 
> --
> 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