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@xxxxxxxxxx>
> ---
> 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:
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
