Re: [PATCH 3/3] Use list_head infra-structure for buffer's log items list

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 24, 2018 at 09:47:36AM +0100, Carlos Maiolino wrote:
> Now that buffer's b_fspriv has been split, just replace the current
> singly linked list of xfs_log_items, by the list_head infrastructure.
> 
> Also, remove the xfs_log_item argument from xfs_buf_resubmit_failed_buffers(),
> there is no need for this argument, once the log items can be walked
> through the list_head in the buffer.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>

> ---
>  fs/xfs/xfs_buf.c        |  1 +
>  fs/xfs/xfs_buf.h        |  2 +-
>  fs/xfs/xfs_buf_item.c   | 64 +++++++++++++++++++++----------------------------
>  fs/xfs/xfs_buf_item.h   |  1 -
>  fs/xfs/xfs_dquot_item.c |  2 +-
>  fs/xfs/xfs_inode.c      |  8 +++----
>  fs/xfs/xfs_inode_item.c | 41 ++++++++++---------------------
>  fs/xfs/xfs_log.c        |  1 +
>  fs/xfs/xfs_trans.h      |  2 +-
>  9 files changed, 47 insertions(+), 75 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 0820c1ccf97c..d1da2ee9e6db 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -236,6 +236,7 @@ _xfs_buf_alloc(
>  	init_completion(&bp->b_iowait);
>  	INIT_LIST_HEAD(&bp->b_lru);
>  	INIT_LIST_HEAD(&bp->b_list);
> +	INIT_LIST_HEAD(&bp->b_li_list);
>  	sema_init(&bp->b_sema, 0); /* held, no waiters */
>  	spin_lock_init(&bp->b_lock);
>  	XB_SET_OWNER(bp);
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 6fcba7536d7e..2f4c91452861 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -177,7 +177,7 @@ typedef struct xfs_buf {
>  	xfs_buf_iodone_t	b_iodone;	/* I/O completion function */
>  	struct completion	b_iowait;	/* queue for I/O waiters */
>  	void			*b_log_item;
> -	struct xfs_log_item	*b_li_list;
> +	struct list_head	b_li_list;	/* Log items list head */
>  	struct xfs_trans	*b_transp;
>  	struct page		**b_pages;	/* array of page pointers */
>  	struct page		*b_page_array[XB_PAGES]; /* inline pages */
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index b27ef1fc5538..4713d3c8e715 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -457,7 +457,7 @@ xfs_buf_item_unpin(
>  		if (bip->bli_flags & XFS_BLI_STALE_INODE) {
>  			xfs_buf_do_callbacks(bp);
>  			bp->b_log_item = NULL;
> -			bp->b_li_list = NULL;
> +			list_del_init(&bp->b_li_list);
>  			bp->b_iodone = NULL;
>  		} else {
>  			spin_lock(&ailp->xa_lock);
> @@ -955,13 +955,12 @@ xfs_buf_item_relse(
>  	xfs_buf_t	*bp)
>  {
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> -	struct xfs_log_item	*lip = bp->b_li_list;
>  
>  	trace_xfs_buf_item_relse(bp, _RET_IP_);
>  	ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
>  
>  	bp->b_log_item = NULL;
> -	if (lip == NULL)
> +	if (list_empty(&bp->b_li_list))
>  		bp->b_iodone = NULL;
>  
>  	xfs_buf_rele(bp);
> @@ -982,18 +981,10 @@ xfs_buf_attach_iodone(
>  	void		(*cb)(xfs_buf_t *, xfs_log_item_t *),
>  	xfs_log_item_t	*lip)
>  {
> -	xfs_log_item_t	*head_lip;
> -
>  	ASSERT(xfs_buf_islocked(bp));
>  
>  	lip->li_cb = cb;
> -	head_lip = bp->b_li_list;
> -	if (head_lip) {
> -		lip->li_bio_list = head_lip->li_bio_list;
> -		head_lip->li_bio_list = lip;
> -	} else {
> -		bp->b_li_list = lip;
> -	}
> +	list_add_tail(&lip->li_bio_list, &bp->b_li_list);
>  
>  	ASSERT(bp->b_iodone == NULL ||
>  	       bp->b_iodone == xfs_buf_iodone_callbacks);
> @@ -1003,12 +994,12 @@ xfs_buf_attach_iodone(
>  /*
>   * We can have many callbacks on a buffer. Running the callbacks individually
>   * can cause a lot of contention on the AIL lock, so we allow for a single
> - * callback to be able to scan the remaining lip->li_bio_list for other items
> - * of the same type and callback to be processed in the first call.
> + * callback to be able to scan the remaining items in bp->b_li_list for other
> + * items of the same type and callback to be processed in the first call.
>   *
>   * As a result, the loop walking the callback list below will also modify the
>   * list. it removes the first item from the list and then runs the callback.
> - * The loop then restarts from the new head of the list. This allows the
> + * The loop then restarts from the new first item int the list. This allows the
>   * callback to scan and modify the list attached to the buffer and we don't
>   * have to care about maintaining a next item pointer.
>   */
> @@ -1025,16 +1016,17 @@ xfs_buf_do_callbacks(
>  		lip->li_cb(bp, lip);
>  	}
>  
> -	while ((lip = bp->b_li_list) != NULL) {
> -		bp->b_li_list = lip->li_bio_list;
> -		ASSERT(lip->li_cb != NULL);
> +	while (!list_empty(&bp->b_li_list)) {
> +		lip = list_first_entry(&bp->b_li_list, struct xfs_log_item,
> +				       li_bio_list);
> +
>  		/*
> -		 * Clear the next pointer so we don't have any
> +		 * Remove the item from the list, so we don't have any
>  		 * confusion if the item is added to another buf.
>  		 * Don't touch the log item after calling its
>  		 * callback, because it could have freed itself.
>  		 */
> -		lip->li_bio_list = NULL;
> +		list_del_init(&lip->li_bio_list);
>  		lip->li_cb(bp, lip);
>  	}
>  }
> @@ -1051,8 +1043,7 @@ STATIC void
>  xfs_buf_do_callbacks_fail(
>  	struct xfs_buf		*bp)
>  {
> -	struct xfs_log_item	*lip = bp->b_li_list;
> -	struct xfs_log_item	*next;
> +	struct xfs_log_item	*lip;
>  	struct xfs_ail		*ailp;
>  
>  	/*
> @@ -1060,14 +1051,16 @@ xfs_buf_do_callbacks_fail(
>  	 * and xfs_buf_iodone_callback_error, and they have no IO error
>  	 * callbacks. Check only for items in b_li_list.
>  	 */
> -	if (lip == NULL)
> +	if (list_empty(&bp->b_li_list)) {
>  		return;
> -	else
> +	} else {
> +		lip = list_first_entry(&bp->b_li_list, struct xfs_log_item,
> +				       li_bio_list);
>  		ailp = lip->li_ailp;
> +	}
>  
>  	spin_lock(&ailp->xa_lock);
> -	for (; lip; lip = next) {
> -		next = lip->li_bio_list;
> +	list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
>  		if (lip->li_ops->iop_error)
>  			lip->li_ops->iop_error(lip, bp);
>  	}
> @@ -1079,7 +1072,7 @@ xfs_buf_iodone_callback_error(
>  	struct xfs_buf		*bp)
>  {
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> -	struct xfs_log_item	*lip = bp->b_li_list;
> +	struct xfs_log_item	*lip;
>  	struct xfs_mount	*mp;
>  	static ulong		lasttime;
>  	static xfs_buftarg_t	*lasttarg;
> @@ -1090,10 +1083,10 @@ xfs_buf_iodone_callback_error(
>  	 * log_item list might be empty. Get the mp from the available
>  	 * xfs_log_item
>  	 */
> -	if (bip == NULL)
> -		mp = lip->li_mountp;
> -	else
> -		mp = bip->bli_item.li_mountp;
> +	lip = list_first_entry_or_null(&bp->b_li_list, struct xfs_log_item,
> +				       li_bio_list);
> +
> +	mp = lip ? lip->li_mountp : bip->bli_item.li_mountp;
>  
>  	/*
>  	 * If we've already decided to shutdown the filesystem because of
> @@ -1204,7 +1197,7 @@ xfs_buf_iodone_callbacks(
>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_log_item = NULL;
> -	bp->b_li_list = NULL;
> +	list_del_init(&bp->b_li_list);
>  	bp->b_iodone = NULL;
>  	xfs_buf_ioend(bp);
>  }
> @@ -1249,10 +1242,9 @@ xfs_buf_iodone(
>  bool
>  xfs_buf_resubmit_failed_buffers(
>  	struct xfs_buf		*bp,
> -	struct xfs_log_item	*lip,
>  	struct list_head	*buffer_list)
>  {
> -	struct xfs_log_item	*next;
> +	struct xfs_log_item	*lip;
>  
>  	/*
>  	 * Clear XFS_LI_FAILED flag from all items before resubmit
> @@ -1260,10 +1252,8 @@ xfs_buf_resubmit_failed_buffers(
>  	 * XFS_LI_FAILED set/clear is protected by xa_lock, caller  this
>  	 * function already have it acquired
>  	 */
> -	for (; lip; lip = next) {
> -		next = lip->li_bio_list;
> +	list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
>  		xfs_clear_li_failed(lip);
> -	}
>  
>  	/* Add this buffer back to the delayed write list */
>  	return xfs_buf_delwri_queue(bp, buffer_list);
> diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
> index 0febfbbf6ba9..643f53dcfe51 100644
> --- a/fs/xfs/xfs_buf_item.h
> +++ b/fs/xfs/xfs_buf_item.h
> @@ -71,7 +71,6 @@ void	xfs_buf_attach_iodone(struct xfs_buf *,
>  void	xfs_buf_iodone_callbacks(struct xfs_buf *);
>  void	xfs_buf_iodone(struct xfs_buf *, struct xfs_log_item *);
>  bool	xfs_buf_resubmit_failed_buffers(struct xfs_buf *,
> -					struct xfs_log_item *,
>  					struct list_head *);
>  
>  extern kmem_zone_t	*xfs_buf_item_zone;
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 51ee848a550e..96eaa6933709 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -176,7 +176,7 @@ xfs_qm_dquot_logitem_push(
>  		if (!xfs_buf_trylock(bp))
>  			return XFS_ITEM_LOCKED;
>  
> -		if (!xfs_buf_resubmit_failed_buffers(bp, lip, buffer_list))
> +		if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list))
>  			rval = XFS_ITEM_FLUSHING;
>  
>  		xfs_buf_unlock(bp);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 8a3ff6343d91..c66effc8e7dd 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2214,7 +2214,7 @@ xfs_ifree_cluster(
>  	xfs_buf_t		*bp;
>  	xfs_inode_t		*ip;
>  	xfs_inode_log_item_t	*iip;
> -	xfs_log_item_t		*lip;
> +	struct xfs_log_item	*lip;
>  	struct xfs_perag	*pag;
>  	xfs_ino_t		inum;
>  
> @@ -2272,8 +2272,7 @@ xfs_ifree_cluster(
>  		 * stale first, we will not attempt to lock them in the loop
>  		 * below as the XFS_ISTALE flag will be set.
>  		 */
> -		lip = bp->b_li_list;
> -		while (lip) {
> +		list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
>  			if (lip->li_type == XFS_LI_INODE) {
>  				iip = (xfs_inode_log_item_t *)lip;
>  				ASSERT(iip->ili_logged == 1);
> @@ -2283,7 +2282,6 @@ xfs_ifree_cluster(
>  							&iip->ili_item.li_lsn);
>  				xfs_iflags_set(iip->ili_inode, XFS_ISTALE);
>  			}
> -			lip = lip->li_bio_list;
>  		}
>  
>  
> @@ -3649,7 +3647,7 @@ xfs_iflush_int(
>  	/* generate the checksum. */
>  	xfs_dinode_calc_crc(mp, dip);
>  
> -	ASSERT(bp->b_li_list != NULL);
> +	ASSERT(!list_empty(&bp->b_li_list));
>  	ASSERT(bp->b_iodone != NULL);
>  	return 0;
>  
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 993736032b4b..ddfc2c80af5e 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -521,7 +521,7 @@ xfs_inode_item_push(
>  		if (!xfs_buf_trylock(bp))
>  			return XFS_ITEM_LOCKED;
>  
> -		if (!xfs_buf_resubmit_failed_buffers(bp, lip, buffer_list))
> +		if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list))
>  			rval = XFS_ITEM_FLUSHING;
>  
>  		xfs_buf_unlock(bp);
> @@ -712,37 +712,23 @@ xfs_iflush_done(
>  	struct xfs_log_item	*lip)
>  {
>  	struct xfs_inode_log_item *iip;
> -	struct xfs_log_item	*blip;
> -	struct xfs_log_item	*next;
> -	struct xfs_log_item	*prev;
> +	struct xfs_log_item	*blip, *n;
>  	struct xfs_ail		*ailp = lip->li_ailp;
>  	int			need_ail = 0;
> +	LIST_HEAD(tmp);
>  
>  	/*
>  	 * Scan the buffer IO completions for other inodes being completed and
>  	 * attach them to the current inode log item.
>  	 */
> -	blip = bp->b_li_list;
> -	prev = NULL;
> -	while (blip != NULL) {
> -		if (blip->li_cb != xfs_iflush_done) {
> -			prev = blip;
> -			blip = blip->li_bio_list;
> -			continue;
> -		}
>  
> -		/* remove from list */
> -		next = blip->li_bio_list;
> -		if (!prev) {
> -			bp->b_li_list = next;
> -		} else {
> -			prev->li_bio_list = next;
> -		}
> +	list_add_tail(&lip->li_bio_list, &tmp);
>  
> -		/* add to current list */
> -		blip->li_bio_list = lip->li_bio_list;
> -		lip->li_bio_list = blip;
> +	list_for_each_entry_safe(blip, n, &bp->b_li_list, li_bio_list) {
> +		if (lip->li_cb != xfs_iflush_done)
> +			continue;
>  
> +		list_move_tail(&blip->li_bio_list, &tmp);
>  		/*
>  		 * while we have the item, do the unlocked check for needing
>  		 * the AIL lock.
> @@ -751,8 +737,6 @@ xfs_iflush_done(
>  		if ((iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn) ||
>  		    (blip->li_flags & XFS_LI_FAILED))
>  			need_ail++;
> -
> -		blip = next;
>  	}
>  
>  	/* make sure we capture the state of the initial inode. */
> @@ -775,7 +759,7 @@ xfs_iflush_done(
>  
>  		/* this is an opencoded batch version of xfs_trans_ail_delete */
>  		spin_lock(&ailp->xa_lock);
> -		for (blip = lip; blip; blip = blip->li_bio_list) {
> +		list_for_each_entry(blip, &tmp, li_bio_list) {
>  			if (INODE_ITEM(blip)->ili_logged &&
>  			    blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
>  				mlip_changed |= xfs_ail_delete_one(ailp, blip);
> @@ -801,15 +785,14 @@ xfs_iflush_done(
>  	 * ili_last_fields bits now that we know that the data corresponding to
>  	 * them is safely on disk.
>  	 */
> -	for (blip = lip; blip; blip = next) {
> -		next = blip->li_bio_list;
> -		blip->li_bio_list = NULL;
> -
> +	list_for_each_entry_safe(blip, n, &tmp, li_bio_list) {
> +		list_del_init(&blip->li_bio_list);
>  		iip = INODE_ITEM(blip);
>  		iip->ili_logged = 0;
>  		iip->ili_last_fields = 0;
>  		xfs_ifunlock(iip->ili_inode);
>  	}
> +	list_del(&tmp);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 20483b654ef1..3e5ba1ecc080 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1047,6 +1047,7 @@ xfs_log_item_init(
>  
>  	INIT_LIST_HEAD(&item->li_ail);
>  	INIT_LIST_HEAD(&item->li_cil);
> +	INIT_LIST_HEAD(&item->li_bio_list);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 815b53d20e26..9d542dfe0052 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -50,7 +50,7 @@ typedef struct xfs_log_item {
>  	uint				li_type;	/* item type */
>  	uint				li_flags;	/* misc flags */
>  	struct xfs_buf			*li_buf;	/* real buffer pointer */
> -	struct xfs_log_item		*li_bio_list;	/* buffer item list */
> +	struct list_head		li_bio_list;	/* buffer item list */
>  	void				(*li_cb)(struct xfs_buf *,
>  						 struct xfs_log_item *);
>  							/* buffer item iodone */
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux