[009/119] xfs: convert list of extents to free into a regular list
diff mbox

Message ID 146612632997.12839.18026491074892368053.stgit@birch.djwong.org
State New
Headers show

Commit Message

Darrick J. Wong June 17, 2016, 1:18 a.m. UTC
In struct xfs_bmap_free, convert the open-coded free extent list to
a regular list, then use list_sort to sort it prior to processing.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |   39 +++++++++++----------------------------
 fs/xfs/libxfs/xfs_bmap.h |   14 ++++++++------
 fs/xfs/xfs_bmap_util.c   |   32 +++++++++++++++++++++++++-------
 fs/xfs/xfs_bmap_util.h   |    1 -
 fs/xfs/xfs_super.c       |    5 +++--
 5 files changed, 47 insertions(+), 44 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig June 17, 2016, 11:59 a.m. UTC | #1
>  {
> +	struct xfs_bmap_free_item	*new;		/* new element */
>  #ifdef DEBUG
>  	xfs_agnumber_t		agno;
>  	xfs_agblock_t		agbno;
> @@ -597,17 +595,7 @@ xfs_bmap_add_free(
>  	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
>  	new->xbfi_startblock = bno;
>  	new->xbfi_blockcount = (xfs_extlen_t)len;
> +	list_add(&new->xbfi_list, &flist->xbf_flist);
>  	flist->xbf_count++;

Please kill xbf_count while you're at it, it's entirely superflous.

> @@ -617,14 +605,10 @@ xfs_bmap_add_free(
>   */
>  void
>  xfs_bmap_del_free(
> -	xfs_bmap_free_t		*flist,	/* free item list header */
> -	xfs_bmap_free_item_t	*prev,	/* previous item on list, if any */
> -	xfs_bmap_free_item_t	*free)	/* list item to be freed */
> +	struct xfs_bmap_free		*flist,	/* free item list header */
> +	struct xfs_bmap_free_item	*free)	/* list item to be freed */

Which then also gets rid of the flist argument here.

> @@ -634,17 +618,16 @@ xfs_bmap_del_free(
>   */
>  void
>  xfs_bmap_cancel(
> +	struct xfs_bmap_free		*flist)	/* list of bmap_free_items */
>  {
> +	struct xfs_bmap_free_item	*free;	/* free list item */
>  
>  	if (flist->xbf_count == 0)
>  		return;
> +	while (!list_empty(&flist->xbf_flist)) {
> +		free = list_first_entry(&flist->xbf_flist,
> +				struct xfs_bmap_free_item, xbfi_list);

	while ((free = list_first_entry_or_null(...))

> +	list_sort((*tp)->t_mountp, &flist->xbf_flist, xfs_bmap_free_list_cmp);

Can you add a comment on why we are sorting the list?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong June 18, 2016, 8:15 p.m. UTC | #2
On Fri, Jun 17, 2016 at 04:59:30AM -0700, Christoph Hellwig wrote:
> >  {
> > +	struct xfs_bmap_free_item	*new;		/* new element */
> >  #ifdef DEBUG
> >  	xfs_agnumber_t		agno;
> >  	xfs_agblock_t		agbno;
> > @@ -597,17 +595,7 @@ xfs_bmap_add_free(
> >  	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
> >  	new->xbfi_startblock = bno;
> >  	new->xbfi_blockcount = (xfs_extlen_t)len;
> > +	list_add(&new->xbfi_list, &flist->xbf_flist);
> >  	flist->xbf_count++;
> 
> Please kill xbf_count while you're at it, it's entirely superflous.

The deferred ops conversion patch kills this off by moving the whole
"defer an op to the next transaction by logging redo items" logic
into a separate file and mechanism.

This patch is just a cleanup to reduce some of the open coded list ugliness
before starting on the rmap stuff.  Once the deferred ops code lands, all
three of these functions go away.

> > @@ -617,14 +605,10 @@ xfs_bmap_add_free(
> >   */
> >  void
> >  xfs_bmap_del_free(
> > -	xfs_bmap_free_t		*flist,	/* free item list header */
> > -	xfs_bmap_free_item_t	*prev,	/* previous item on list, if any */
> > -	xfs_bmap_free_item_t	*free)	/* list item to be freed */
> > +	struct xfs_bmap_free		*flist,	/* free item list header */
> > +	struct xfs_bmap_free_item	*free)	/* list item to be freed */
> 
> Which then also gets rid of the flist argument here.
> 
> > @@ -634,17 +618,16 @@ xfs_bmap_del_free(
> >   */
> >  void
> >  xfs_bmap_cancel(
> > +	struct xfs_bmap_free		*flist)	/* list of bmap_free_items */
> >  {
> > +	struct xfs_bmap_free_item	*free;	/* free list item */
> >  
> >  	if (flist->xbf_count == 0)
> >  		return;
> > +	while (!list_empty(&flist->xbf_flist)) {
> > +		free = list_first_entry(&flist->xbf_flist,
> > +				struct xfs_bmap_free_item, xbfi_list);
> 
> 	while ((free = list_first_entry_or_null(...))
> 
> > +	list_sort((*tp)->t_mountp, &flist->xbf_flist, xfs_bmap_free_list_cmp);
> 
> Can you add a comment on why we are sorting the list?

We sort the list so that we process the freed extents in AG order to
avoid deadlocking.

I'll add a comment to the deferred ops code if there isn't one already.

--D

> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner June 21, 2016, 12:57 a.m. UTC | #3
On Sat, Jun 18, 2016 at 01:15:10PM -0700, Darrick J. Wong wrote:
> On Fri, Jun 17, 2016 at 04:59:30AM -0700, Christoph Hellwig wrote:
> > >  {
> > > +	struct xfs_bmap_free_item	*new;		/* new element */
> > >  #ifdef DEBUG
> > >  	xfs_agnumber_t		agno;
> > >  	xfs_agblock_t		agbno;
> > > @@ -597,17 +595,7 @@ xfs_bmap_add_free(
> > >  	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
> > >  	new->xbfi_startblock = bno;
> > >  	new->xbfi_blockcount = (xfs_extlen_t)len;
> > > +	list_add(&new->xbfi_list, &flist->xbf_flist);
> > >  	flist->xbf_count++;
> > 
> > Please kill xbf_count while you're at it, it's entirely superflous.
> 
> The deferred ops conversion patch kills this off by moving the whole
> "defer an op to the next transaction by logging redo items" logic
> into a separate file and mechanism.
> 
> This patch is just a cleanup to reduce some of the open coded list ugliness
> before starting on the rmap stuff.  Once the deferred ops code lands, all
> three of these functions go away.

Ok, so because all these functions go away, I'll take this patch now
without the suggested cleanups so that you don't have to rework it.

....

> > > +	list_sort((*tp)->t_mountp, &flist->xbf_flist, xfs_bmap_free_list_cmp);
> > 
> > Can you add a comment on why we are sorting the list?
> 
> We sort the list so that we process the freed extents in AG order to
> avoid deadlocking.
> 
> I'll add a comment to the deferred ops code if there isn't one already.

This seems best - add the clean up to the later patches rather than
have to rework lots of patches because of minor mods to early
patches...

Cheers,

Dave.

Patch
diff mbox

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index ea7b3df..a5d207a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -575,9 +575,7 @@  xfs_bmap_add_free(
 	xfs_fsblock_t		bno,		/* fs block number of extent */
 	xfs_filblks_t		len)		/* length of extent */
 {
-	xfs_bmap_free_item_t	*cur;		/* current (next) element */
-	xfs_bmap_free_item_t	*new;		/* new element */
-	xfs_bmap_free_item_t	*prev;		/* previous element */
+	struct xfs_bmap_free_item	*new;		/* new element */
 #ifdef DEBUG
 	xfs_agnumber_t		agno;
 	xfs_agblock_t		agbno;
@@ -597,17 +595,7 @@  xfs_bmap_add_free(
 	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
 	new->xbfi_startblock = bno;
 	new->xbfi_blockcount = (xfs_extlen_t)len;
-	for (prev = NULL, cur = flist->xbf_first;
-	     cur != NULL;
-	     prev = cur, cur = cur->xbfi_next) {
-		if (cur->xbfi_startblock >= bno)
-			break;
-	}
-	if (prev)
-		prev->xbfi_next = new;
-	else
-		flist->xbf_first = new;
-	new->xbfi_next = cur;
+	list_add(&new->xbfi_list, &flist->xbf_flist);
 	flist->xbf_count++;
 }
 
@@ -617,14 +605,10 @@  xfs_bmap_add_free(
  */
 void
 xfs_bmap_del_free(
-	xfs_bmap_free_t		*flist,	/* free item list header */
-	xfs_bmap_free_item_t	*prev,	/* previous item on list, if any */
-	xfs_bmap_free_item_t	*free)	/* list item to be freed */
+	struct xfs_bmap_free		*flist,	/* free item list header */
+	struct xfs_bmap_free_item	*free)	/* list item to be freed */
 {
-	if (prev)
-		prev->xbfi_next = free->xbfi_next;
-	else
-		flist->xbf_first = free->xbfi_next;
+	list_del(&free->xbfi_list);
 	flist->xbf_count--;
 	kmem_zone_free(xfs_bmap_free_item_zone, free);
 }
@@ -634,17 +618,16 @@  xfs_bmap_del_free(
  */
 void
 xfs_bmap_cancel(
-	xfs_bmap_free_t		*flist)	/* list of bmap_free_items */
+	struct xfs_bmap_free		*flist)	/* list of bmap_free_items */
 {
-	xfs_bmap_free_item_t	*free;	/* free list item */
-	xfs_bmap_free_item_t	*next;
+	struct xfs_bmap_free_item	*free;	/* free list item */
 
 	if (flist->xbf_count == 0)
 		return;
-	ASSERT(flist->xbf_first != NULL);
-	for (free = flist->xbf_first; free; free = next) {
-		next = free->xbfi_next;
-		xfs_bmap_del_free(flist, NULL, free);
+	while (!list_empty(&flist->xbf_flist)) {
+		free = list_first_entry(&flist->xbf_flist,
+				struct xfs_bmap_free_item, xbfi_list);
+		xfs_bmap_del_free(flist, free);
 	}
 	ASSERT(flist->xbf_count == 0);
 }
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 0b2f72c..0ef4c6b 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -62,12 +62,12 @@  struct xfs_bmalloca {
  * List of extents to be free "later".
  * The list is kept sorted on xbf_startblock.
  */
-typedef struct xfs_bmap_free_item
+struct xfs_bmap_free_item
 {
 	xfs_fsblock_t		xbfi_startblock;/* starting fs block number */
 	xfs_extlen_t		xbfi_blockcount;/* number of blocks in extent */
-	struct xfs_bmap_free_item *xbfi_next;	/* link to next entry */
-} xfs_bmap_free_item_t;
+	struct list_head	xbfi_list;
+};
 
 /*
  * Header for free extent list.
@@ -85,7 +85,7 @@  typedef struct xfs_bmap_free_item
  */
 typedef	struct xfs_bmap_free
 {
-	xfs_bmap_free_item_t	*xbf_first;	/* list of to-be-free extents */
+	struct list_head	xbf_flist;	/* list of to-be-free extents */
 	int			xbf_count;	/* count of items on list */
 	int			xbf_low;	/* alloc in low mode */
 } xfs_bmap_free_t;
@@ -141,8 +141,10 @@  static inline int xfs_bmapi_aflag(int w)
 
 static inline void xfs_bmap_init(xfs_bmap_free_t *flp, xfs_fsblock_t *fbp)
 {
-	((flp)->xbf_first = NULL, (flp)->xbf_count = 0, \
-		(flp)->xbf_low = 0, *(fbp) = NULLFSBLOCK);
+	INIT_LIST_HEAD(&flp->xbf_flist);
+	flp->xbf_count = 0;
+	flp->xbf_low = 0;
+	*fbp = NULLFSBLOCK;
 }
 
 /*
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 28c42fb..1aac0ba 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -79,6 +79,23 @@  xfs_zero_extent(
 		GFP_NOFS, true);
 }
 
+/* Sort bmap items by AG. */
+static int
+xfs_bmap_free_list_cmp(
+	void			*priv,
+	struct list_head	*a,
+	struct list_head	*b)
+{
+	struct xfs_mount	*mp = priv;
+	struct xfs_bmap_free_item	*ra;
+	struct xfs_bmap_free_item	*rb;
+
+	ra = container_of(a, struct xfs_bmap_free_item, xbfi_list);
+	rb = container_of(b, struct xfs_bmap_free_item, xbfi_list);
+	return  XFS_FSB_TO_AGNO(mp, ra->xbfi_startblock) -
+		XFS_FSB_TO_AGNO(mp, rb->xbfi_startblock);
+}
+
 /*
  * Routine to be called at transaction's end by xfs_bmapi, xfs_bunmapi
  * caller.  Frees all the extents that need freeing, which must be done
@@ -99,14 +116,15 @@  xfs_bmap_finish(
 	int				error;	/* error return value */
 	int				committed;/* xact committed or not */
 	struct xfs_bmap_free_item	*free;	/* free extent item */
-	struct xfs_bmap_free_item	*next;	/* next item on free list */
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
 	if (flist->xbf_count == 0)
 		return 0;
 
+	list_sort((*tp)->t_mountp, &flist->xbf_flist, xfs_bmap_free_list_cmp);
+
 	efi = xfs_trans_get_efi(*tp, flist->xbf_count);
-	for (free = flist->xbf_first; free; free = free->xbfi_next)
+	list_for_each_entry(free, &flist->xbf_flist, xbfi_list)
 		xfs_trans_log_efi_extent(*tp, efi, free->xbfi_startblock,
 			free->xbfi_blockcount);
 
@@ -136,15 +154,15 @@  xfs_bmap_finish(
 	 * on error.
 	 */
 	efd = xfs_trans_get_efd(*tp, efi, flist->xbf_count);
-	for (free = flist->xbf_first; free != NULL; free = next) {
-		next = free->xbfi_next;
-
+	while (!list_empty(&flist->xbf_flist)) {
+		free = list_first_entry(&flist->xbf_flist,
+				struct xfs_bmap_free_item, xbfi_list);
 		error = xfs_trans_free_extent(*tp, efd, free->xbfi_startblock,
 					      free->xbfi_blockcount);
 		if (error)
 			return error;
 
-		xfs_bmap_del_free(flist, NULL, free);
+		xfs_bmap_del_free(flist, free);
 	}
 
 	return 0;
@@ -797,7 +815,7 @@  xfs_bmap_punch_delalloc_range(
 		if (error)
 			break;
 
-		ASSERT(!flist.xbf_count && !flist.xbf_first);
+		ASSERT(!flist.xbf_count && list_empty(&flist.xbf_flist));
 next_block:
 		start_fsb++;
 		remaining--;
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 1492348..f200714 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -41,7 +41,6 @@  int	xfs_getbmap(struct xfs_inode *ip, struct getbmapx *bmv,
 
 /* functions in xfs_bmap.c that are only needed by xfs_bmap_util.c */
 void	xfs_bmap_del_free(struct xfs_bmap_free *flist,
-			  struct xfs_bmap_free_item *prev,
 			  struct xfs_bmap_free_item *free);
 int	xfs_bmap_extsize_align(struct xfs_mount *mp, struct xfs_bmbt_irec *gotp,
 			       struct xfs_bmbt_irec *prevp, xfs_extlen_t extsz,
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 4700f09..09722a7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1692,8 +1692,9 @@  xfs_init_zones(void)
 	if (!xfs_log_ticket_zone)
 		goto out_free_ioend_bioset;
 
-	xfs_bmap_free_item_zone = kmem_zone_init(sizeof(xfs_bmap_free_item_t),
-						"xfs_bmap_free_item");
+	xfs_bmap_free_item_zone = kmem_zone_init(
+			sizeof(struct xfs_bmap_free_item),
+			"xfs_bmap_free_item");
 	if (!xfs_bmap_free_item_zone)
 		goto out_destroy_log_ticket_zone;