diff mbox

[6/6] xfs: remove xfs_trans_ail_delete_bulk

Message ID 20170421152123.11316-7-hch@lst.de (mailing list archive)
State Accepted
Headers show

Commit Message

Christoph Hellwig April 21, 2017, 3:21 p.m. UTC 
xfs_iflush_done uses an on-stack variable length array to pass the log
items to be deleted to xfs_trans_ail_delete_bulk.  On-stack VLAs are a
nasty gcc extension that can lead to unbounded stack allocations, but
fortunately we can easily avoid them by simply open coding
xfs_trans_ail_delete_bulk in xfs_iflush_done, which is the only caller
of it except for the single-item xfs_trans_ail_delete.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode_item.c | 29 +++++++++++---------
 fs/xfs/xfs_trans_ail.c  | 71 ++++++++++++++++++++++++-------------------------
 fs/xfs/xfs_trans_priv.h | 15 +++--------
 3 files changed, 55 insertions(+), 60 deletions(-)

Comments

Darrick J. Wong April 21, 2017, 7:35 p.m. UTC | #1 
On Fri, Apr 21, 2017 at 05:21:23PM +0200, Christoph Hellwig wrote:
> xfs_iflush_done uses an on-stack variable length array to pass the log
> items to be deleted to xfs_trans_ail_delete_bulk.  On-stack VLAs are a
> nasty gcc extension that can lead to unbounded stack allocations, but
> fortunately we can easily avoid them by simply open coding
> xfs_trans_ail_delete_bulk in xfs_iflush_done, which is the only caller
> of it except for the single-item xfs_trans_ail_delete.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok, will throw it on the testing pile...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_inode_item.c | 29 +++++++++++---------
>  fs/xfs/xfs_trans_ail.c  | 71 ++++++++++++++++++++++++-------------------------
>  fs/xfs/xfs_trans_priv.h | 15 +++--------
>  3 files changed, 55 insertions(+), 60 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index d90e7811ccdd..08cb7d1a4a3a 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -731,22 +731,27 @@ xfs_iflush_done(
>  	 * holding the lock before removing the inode from the AIL.
>  	 */
>  	if (need_ail) {
> -		struct xfs_log_item *log_items[need_ail];
> -		int i = 0;
> +		bool			mlip_changed = false;
> +
> +		/* 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) {
> -			iip = INODE_ITEM(blip);
> -			if (iip->ili_logged &&
> -			    blip->li_lsn == iip->ili_flush_lsn) {
> -				log_items[i++] = blip;
> -			}
> -			ASSERT(i <= need_ail);
> +			if (INODE_ITEM(blip)->ili_logged &&
> +			    blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
> +				mlip_changed |= xfs_ail_delete_one(ailp, blip);
>  		}
> -		/* xfs_trans_ail_delete_bulk() drops the AIL lock. */
> -		xfs_trans_ail_delete_bulk(ailp, log_items, i,
> -					  SHUTDOWN_CORRUPT_INCORE);
> -	}
>  
> +		if (mlip_changed) {
> +			if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
> +				xlog_assign_tail_lsn_locked(ailp->xa_mount);
> +			if (list_empty(&ailp->xa_ail))
> +				wake_up_all(&ailp->xa_empty);
> +		}
> +		spin_unlock(&ailp->xa_lock);
> +
> +		if (mlip_changed)
> +			xfs_log_space_wake(ailp->xa_mount);
> +	}
>  
>  	/*
>  	 * clean up and unlock the flush lock now we are done. We can clear the
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index d6c9c3e9e02b..9056c0f34a3c 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -684,8 +684,23 @@ xfs_trans_ail_update_bulk(
>  	}
>  }
>  
> -/*
> - * xfs_trans_ail_delete_bulk - remove multiple log items from the AIL
> +bool
> +xfs_ail_delete_one(
> +	struct xfs_ail		*ailp,
> +	struct xfs_log_item 	*lip)
> +{
> +	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
> +
> +	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> +	xfs_ail_delete(ailp, lip);
> +	lip->li_flags &= ~XFS_LI_IN_AIL;
> +	lip->li_lsn = 0;
> +
> +	return mlip == lip;
> +}
> +
> +/**
> + * Remove a log items from the AIL
>   *
>   * @xfs_trans_ail_delete_bulk takes an array of log items that all need to
>   * removed from the AIL. The caller is already holding the AIL lock, and done
> @@ -706,52 +721,36 @@ xfs_trans_ail_update_bulk(
>   * before returning.
>   */
>  void
> -xfs_trans_ail_delete_bulk(
> +xfs_trans_ail_delete(
>  	struct xfs_ail		*ailp,
> -	struct xfs_log_item	**log_items,
> -	int			nr_items,
> +	struct xfs_log_item	*lip,
>  	int			shutdown_type) __releases(ailp->xa_lock)
>  {
> -	xfs_log_item_t		*mlip;
> -	int			mlip_changed = 0;
> -	int			i;
> +	struct xfs_mount	*mp = ailp->xa_mount;
> +	bool			mlip_changed;
>  
> -	mlip = xfs_ail_min(ailp);
> -
> -	for (i = 0; i < nr_items; i++) {
> -		struct xfs_log_item *lip = log_items[i];
> -		if (!(lip->li_flags & XFS_LI_IN_AIL)) {
> -			struct xfs_mount	*mp = ailp->xa_mount;
> -
> -			spin_unlock(&ailp->xa_lock);
> -			if (!XFS_FORCED_SHUTDOWN(mp)) {
> -				xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> -		"%s: attempting to delete a log item that is not in the AIL",
> -						__func__);
> -				xfs_force_shutdown(mp, shutdown_type);
> -			}
> -			return;
> +	if (!(lip->li_flags & XFS_LI_IN_AIL)) {
> +		spin_unlock(&ailp->xa_lock);
> +		if (!XFS_FORCED_SHUTDOWN(mp)) {
> +			xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> +	"%s: attempting to delete a log item that is not in the AIL",
> +					__func__);
> +			xfs_force_shutdown(mp, shutdown_type);
>  		}
> -
> -		trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> -		xfs_ail_delete(ailp, lip);
> -		lip->li_flags &= ~XFS_LI_IN_AIL;
> -		lip->li_lsn = 0;
> -		if (mlip == lip)
> -			mlip_changed = 1;
> +		return;
>  	}
>  
> +	mlip_changed = xfs_ail_delete_one(ailp, lip);
>  	if (mlip_changed) {
> -		if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
> -			xlog_assign_tail_lsn_locked(ailp->xa_mount);
> +		if (!XFS_FORCED_SHUTDOWN(mp))
> +			xlog_assign_tail_lsn_locked(mp);
>  		if (list_empty(&ailp->xa_ail))
>  			wake_up_all(&ailp->xa_empty);
> -		spin_unlock(&ailp->xa_lock);
> +	}
>  
> +	spin_unlock(&ailp->xa_lock);
> +	if (mlip_changed)
>  		xfs_log_space_wake(ailp->xa_mount);
> -	} else {
> -		spin_unlock(&ailp->xa_lock);
> -	}
>  }
>  
>  int
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 49931b72da8a..d91706c56c63 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -106,18 +106,9 @@ xfs_trans_ail_update(
>  	xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
>  }
>  
> -void	xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
> -				struct xfs_log_item **log_items, int nr_items,
> -				int shutdown_type)
> -				__releases(ailp->xa_lock);
> -static inline void
> -xfs_trans_ail_delete(
> -	struct xfs_ail	*ailp,
> -	xfs_log_item_t	*lip,
> -	int		shutdown_type) __releases(ailp->xa_lock)
> -{
> -	xfs_trans_ail_delete_bulk(ailp, &lip, 1, shutdown_type);
> -}
> +bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
> +void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
> +		int shutdown_type) __releases(ailp->xa_lock);
>  
>  static inline void
>  xfs_trans_ail_remove(
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index d90e7811ccdd..08cb7d1a4a3a 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -731,22 +731,27 @@  xfs_iflush_done(
 	 * holding the lock before removing the inode from the AIL.
 	 */
 	if (need_ail) {
-		struct xfs_log_item *log_items[need_ail];
-		int i = 0;
+		bool			mlip_changed = false;
+
+		/* 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) {
-			iip = INODE_ITEM(blip);
-			if (iip->ili_logged &&
-			    blip->li_lsn == iip->ili_flush_lsn) {
-				log_items[i++] = blip;
-			}
-			ASSERT(i <= need_ail);
+			if (INODE_ITEM(blip)->ili_logged &&
+			    blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
+				mlip_changed |= xfs_ail_delete_one(ailp, blip);
 		}
-		/* xfs_trans_ail_delete_bulk() drops the AIL lock. */
-		xfs_trans_ail_delete_bulk(ailp, log_items, i,
-					  SHUTDOWN_CORRUPT_INCORE);
-	}
 
+		if (mlip_changed) {
+			if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
+				xlog_assign_tail_lsn_locked(ailp->xa_mount);
+			if (list_empty(&ailp->xa_ail))
+				wake_up_all(&ailp->xa_empty);
+		}
+		spin_unlock(&ailp->xa_lock);
+
+		if (mlip_changed)
+			xfs_log_space_wake(ailp->xa_mount);
+	}
 
 	/*
 	 * clean up and unlock the flush lock now we are done. We can clear the
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index d6c9c3e9e02b..9056c0f34a3c 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -684,8 +684,23 @@  xfs_trans_ail_update_bulk(
 	}
 }
 
-/*
- * xfs_trans_ail_delete_bulk - remove multiple log items from the AIL
+bool
+xfs_ail_delete_one(
+	struct xfs_ail		*ailp,
+	struct xfs_log_item 	*lip)
+{
+	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
+
+	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
+	xfs_ail_delete(ailp, lip);
+	lip->li_flags &= ~XFS_LI_IN_AIL;
+	lip->li_lsn = 0;
+
+	return mlip == lip;
+}
+
+/**
+ * Remove a log items from the AIL
  *
  * @xfs_trans_ail_delete_bulk takes an array of log items that all need to
  * removed from the AIL. The caller is already holding the AIL lock, and done
@@ -706,52 +721,36 @@  xfs_trans_ail_update_bulk(
  * before returning.
  */
 void
-xfs_trans_ail_delete_bulk(
+xfs_trans_ail_delete(
 	struct xfs_ail		*ailp,
-	struct xfs_log_item	**log_items,
-	int			nr_items,
+	struct xfs_log_item	*lip,
 	int			shutdown_type) __releases(ailp->xa_lock)
 {
-	xfs_log_item_t		*mlip;
-	int			mlip_changed = 0;
-	int			i;
+	struct xfs_mount	*mp = ailp->xa_mount;
+	bool			mlip_changed;
 
-	mlip = xfs_ail_min(ailp);
-
-	for (i = 0; i < nr_items; i++) {
-		struct xfs_log_item *lip = log_items[i];
-		if (!(lip->li_flags & XFS_LI_IN_AIL)) {
-			struct xfs_mount	*mp = ailp->xa_mount;
-
-			spin_unlock(&ailp->xa_lock);
-			if (!XFS_FORCED_SHUTDOWN(mp)) {
-				xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
-		"%s: attempting to delete a log item that is not in the AIL",
-						__func__);
-				xfs_force_shutdown(mp, shutdown_type);
-			}
-			return;
+	if (!(lip->li_flags & XFS_LI_IN_AIL)) {
+		spin_unlock(&ailp->xa_lock);
+		if (!XFS_FORCED_SHUTDOWN(mp)) {
+			xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
+	"%s: attempting to delete a log item that is not in the AIL",
+					__func__);
+			xfs_force_shutdown(mp, shutdown_type);
 		}
-
-		trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
-		xfs_ail_delete(ailp, lip);
-		lip->li_flags &= ~XFS_LI_IN_AIL;
-		lip->li_lsn = 0;
-		if (mlip == lip)
-			mlip_changed = 1;
+		return;
 	}
 
+	mlip_changed = xfs_ail_delete_one(ailp, lip);
 	if (mlip_changed) {
-		if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
-			xlog_assign_tail_lsn_locked(ailp->xa_mount);
+		if (!XFS_FORCED_SHUTDOWN(mp))
+			xlog_assign_tail_lsn_locked(mp);
 		if (list_empty(&ailp->xa_ail))
 			wake_up_all(&ailp->xa_empty);
-		spin_unlock(&ailp->xa_lock);
+	}
 
+	spin_unlock(&ailp->xa_lock);
+	if (mlip_changed)
 		xfs_log_space_wake(ailp->xa_mount);
-	} else {
-		spin_unlock(&ailp->xa_lock);
-	}
 }
 
 int
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 49931b72da8a..d91706c56c63 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -106,18 +106,9 @@  xfs_trans_ail_update(
 	xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
 }
 
-void	xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
-				struct xfs_log_item **log_items, int nr_items,
-				int shutdown_type)
-				__releases(ailp->xa_lock);
-static inline void
-xfs_trans_ail_delete(
-	struct xfs_ail	*ailp,
-	xfs_log_item_t	*lip,
-	int		shutdown_type) __releases(ailp->xa_lock)
-{
-	xfs_trans_ail_delete_bulk(ailp, &lip, 1, shutdown_type);
-}
+bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
+void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
+		int shutdown_type) __releases(ailp->xa_lock);
 
 static inline void
 xfs_trans_ail_remove(