2010-06-21 13:16:55

by Fred Isaman

[permalink] [raw]
Subject: [PATCH 0/3] pnfs refcount patches v3

Changes layout refcounting.

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

Fred



2010-06-21 13:16:52

by Fred Isaman

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

This is needed by next patch that changes refcounting

Signed-off-by: Fred Isaman <[email protected]>
---
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


2010-06-21 13:16:55

by Fred Isaman

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

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 <[email protected]>
---
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


2010-06-21 13:16:53

by Fred Isaman

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

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

Signed-off-by: Fred Isaman <[email protected]>
---
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


2010-06-22 06:56:23

by Benny Halevy

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

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

Thanks!

Benny

On Jun. 21, 2010, 16:16 +0300, Fred Isaman <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html