Message-ID: <4EBB9136.7010603@tonian.com>
Date: Thu, 10 Nov 2011 10:54:14 +0200
From: Benny Halevy <bhalevy@tonian.com>
MIME-Version: 1.0
To: Peng Tao <bergwolf@gmail.com>
CC: linux-nfs@vger.kernel.org, Trond.Myklebust@netapp.com,
        Peng Tao <peng_tao@emc.com>
Subject: Re: [PATCH 7/8] pnfsblock: add im_extents to pnfs_inval_markings
References: <1320851766-1834-1-git-send-email-bergwolf@gmail.com> <1320851766-1834-8-git-send-email-bergwolf@gmail.com>
In-Reply-To: <1320851766-1834-8-git-send-email-bergwolf@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-nfs-owner@vger.kernel.org

On 2011-11-09 17:16, Peng Tao wrote:
> It stores a list of short extents for INVAL->RW conversion.
> Also add two functions to manipulate them, in preparation to
> move malloc logic out of end_io.
> 
> Signed-off-by: Peng Tao <peng_tao@emc.com>
> ---
>  fs/nfs/blocklayout/blocklayout.c |    6 ++++++
>  fs/nfs/blocklayout/blocklayout.h |    5 +++++
>  fs/nfs/blocklayout/extents.c     |   37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
> index 815c0c3..cb4ff0f 100644
> --- a/fs/nfs/blocklayout/blocklayout.c
> +++ b/fs/nfs/blocklayout/blocklayout.c
> @@ -706,11 +706,17 @@ static void
>  release_inval_marks(struct pnfs_inval_markings *marks)
>  {
>  	struct pnfs_inval_tracking *pos, *temp;
> +	struct pnfs_block_short_extent *se, *stemp;
>  
>  	list_for_each_entry_safe(pos, temp, &marks->im_tree.mtt_stub, it_link) {
>  		list_del(&pos->it_link);
>  		kfree(pos);
>  	}
> +
> +	list_for_each_entry_safe(se, stemp, &marks->im_extents, bse_node) {
> +		list_del(&se->bse_node);
> +		kfree(se);
> +	}
>  	return;
>  }
>  
> diff --git a/fs/nfs/blocklayout/blocklayout.h b/fs/nfs/blocklayout/blocklayout.h
> index 60728ac..df0e0fb 100644
> --- a/fs/nfs/blocklayout/blocklayout.h
> +++ b/fs/nfs/blocklayout/blocklayout.h
> @@ -70,6 +70,7 @@ struct pnfs_inval_markings {
>  	spinlock_t	im_lock;
>  	struct my_tree	im_tree;	/* Sectors that need LAYOUTCOMMIT */
>  	sector_t	im_block_size;	/* Server blocksize in sectors */
> +	struct list_head im_extents;	/* List of short extents for INVAL->RW conversion */
>  };
>  
>  struct pnfs_inval_tracking {
> @@ -105,6 +106,7 @@ BL_INIT_INVAL_MARKS(struct pnfs_inval_markings *marks, sector_t blocksize)
>  {
>  	spin_lock_init(&marks->im_lock);
>  	INIT_LIST_HEAD(&marks->im_tree.mtt_stub);
> +	INIT_LIST_HEAD(&marks->im_extents);
>  	marks->im_block_size = blocksize;
>  	marks->im_tree.mtt_step_size = min((sector_t)PAGE_CACHE_SECTORS,
>  					   blocksize);
> @@ -200,5 +202,8 @@ int bl_add_merge_extent(struct pnfs_block_layout *bl,
>  			 struct pnfs_block_extent *new);
>  int bl_mark_for_commit(struct pnfs_block_extent *be,
>  			sector_t offset, sector_t length);
> +int bl_push_one_short_extent(struct pnfs_inval_markings *marks);
> +struct pnfs_block_short_extent*
> +bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop);
>  
>  #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */
> diff --git a/fs/nfs/blocklayout/extents.c b/fs/nfs/blocklayout/extents.c
> index 952ea8a..72c7fa1 100644
> --- a/fs/nfs/blocklayout/extents.c
> +++ b/fs/nfs/blocklayout/extents.c
> @@ -863,3 +863,40 @@ clean_pnfs_block_layoutupdate(struct pnfs_block_layout *bl,
>  		}
>  	}
>  }
> +
> +int
> +bl_push_one_short_extent(struct pnfs_inval_markings *marks) {
> +	struct pnfs_block_short_extent *new;
> +
> +	new = kmalloc(sizeof(*new), GFP_NOFS);
> +	if (unlikely(!new))
> +		return -ENOMEM;
> +
> +	spin_lock(&marks->im_lock);
> +	list_add(&new->bse_node, &marks->im_extents);
> +	spin_unlock(&marks->im_lock);
> +
> +	return 0;
> +}
> +
> +struct pnfs_block_short_extent*
> +bl_pop_short_extent(struct pnfs_inval_markings *marks, int num_to_pop) {
> +	struct pnfs_block_short_extent *rv = NULL;
> +
> +	if (unlikely(num_to_pop <= 0))
> +		return rv;

How unlikely is it?
Is doing the extra compare really worth saving the spin_lock?

> +
> +	spin_lock(&marks->im_lock);
> +	while (!list_empty(&marks->im_extents) && num_to_pop-- > 0) {
> +		rv = list_entry((&marks->im_extents)->next,
> +				struct pnfs_block_short_extent, bse_node);
> +		list_del_init(&rv->bse_node);
> +		if (num_to_pop)
> +			kfree(rv);

Please correct me if I'm wrong, you don't want to free the last element
you pop since you want to return it. This is worth a comment...

I'd consider moving the decrement expression down here or
changing the loop to be a for loop to improve its readability.
In the latter case this will say if (num_to_pop > 1) kfree(rv)
which is more straight forward IMHO.

Benny

> +	}
> +	spin_unlock(&marks->im_lock);
> +
> +	BUG_ON(num_to_pop > 0);
> +
> +	return rv;
> +}