Re: [PATCH] xfs: fix incorrect tail lsn tracking when AIL is empty

[Long Li <leo.lilong@huawei.com>]

On Fri, May 23, 2025 at 02:30:46PM +0800, Long Li wrote:

Friendly ping, hoping someone can give me some suggestions.

> When the AIL is empty, we track ail_head_lsn as the tail LSN, where
> ail_head_lsn records the commit LSN of Checkpoint N, the last checkpoint
> inserted into the AIL. There are two possible scenarios when the AIL is
> empty:
> 
> 1. Items from Checkpoint N were previously inserted into the AIL, have
>    been written back, and subsequently removed from the AIL.
> 2. Items from Checkpoint N have not yet been inserted into the AIL, but
>    the preparation for insertion is complete, and ail_head_lsn has already
>    been updated to Checkpoint N's commit LSN.
> 
> For scenario 1, the items in Checkpoint N have already been written to the
> metadata area. Even in the event of a crash, Checkpoint N does not require
> recovery, so forwarding the tail LSN to Checkpoint N's commit LSN is
> reasonable.
> 
> For scenario 2, the items in Checkpoint N have not been written to the
> metadata area. If new logs (ie., Checkpoint N+1) are flushed to disk with
> the tail LSN recorded as Checkpoint N's commit LSN, a crash would make it
> impossible to recover Checkpoint N.
> 
> Checkpoint N    start N       commit N
>                    +-------------+------------+--------------+
> Checkpoint N+1                           start N+1      commit N+1
> 
> Scenario 2 is possible. I encountered this issue in the Linux 6.6, where
> l_last_sync_lsn was used to track the tail LSN when the AIL is empty.
> Although the code has been refactored and I have not reproduced this issue
> in the latest mainline kernel, I believe the problem still exists.
> 
> In the function xlog_cil_ail_insert(), which inserts items from the ctx
> into the AIL, the update of ail_head_lsn and the actual insertion of items
> into the AIL are not atomic. This process is not protected by `ail_lock`
> or `log->l_icloglock`, leaving a window that could result in the iclog
> being filled with an incorrect tail LSN.
> 
> When the AIL is empty, the tail LSN should be set to Checkpoint N's start
> LSN before the items from Checkpoint N are inserted into the AIL. This
> ensures that Checkpoint N can be recovered in case of a crash. After the
> items from Checkpoint N are inserted into the AIL, the tail LSN tracked
> for an empty AIL can then be updated to Checkpoint N's commit LSN. This
> cannot be achieved with ail_head_lsn alone, so a new variable,
> ail_tail_lsn, is introduced specifically to track the tail LSN when the
> AIL is empty.
> 
> Fixes: 14e15f1bcd73 ("xfs: push the grant head when the log head moves forward") # further than this
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/xfs_log_cil.c     | 2 ++
>  fs/xfs/xfs_log_recover.c | 3 +++
>  fs/xfs/xfs_trans_ail.c   | 2 +-
>  fs/xfs/xfs_trans_priv.h  | 1 +
>  4 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index f66d2d430e4f..ecc31329669a 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -775,6 +775,7 @@ xlog_cil_ail_insert(
>  	xfs_trans_ail_cursor_last(ailp, &cur, ctx->start_lsn);
>  	old_head = ailp->ail_head_lsn;
>  	ailp->ail_head_lsn = ctx->commit_lsn;
> +	ailp->ail_tail_lsn = ctx->start_lsn;
>  	/* xfs_ail_update_finish() drops the ail_lock */
>  	xfs_ail_update_finish(ailp, NULLCOMMITLSN);
>  
> @@ -857,6 +858,7 @@ xlog_cil_ail_insert(
>  
>  	spin_lock(&ailp->ail_lock);
>  	xfs_trans_ail_cursor_done(&cur);
> +	ailp->ail_tail_lsn = ctx->commit_lsn;
>  	spin_unlock(&ailp->ail_lock);
>  }
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 2f76531842f8..ef04fd8ded67 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1179,6 +1179,8 @@ xlog_check_unmount_rec(
>  					log->l_curr_cycle, after_umount_blk);
>  			log->l_ailp->ail_head_lsn =
>  					atomic64_read(&log->l_tail_lsn);
> +			log->l_ailp->ail_tail_lsn =
> +					atomic64_read(&log->l_tail_lsn);
>  			*tail_blk = after_umount_blk;
>  
>  			*clean = true;
> @@ -1212,6 +1214,7 @@ xlog_set_state(
>  	if (bump_cycle)
>  		log->l_curr_cycle++;
>  	atomic64_set(&log->l_tail_lsn, be64_to_cpu(rhead->h_tail_lsn));
> +	log->l_ailp->ail_tail_lsn = be64_to_cpu(rhead->h_lsn);
>  	log->l_ailp->ail_head_lsn = be64_to_cpu(rhead->h_lsn);
>  }
>  
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 67c328d23e4a..bdd45aaa5bc1 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -740,7 +740,7 @@ __xfs_ail_assign_tail_lsn(
>  
>  	tail_lsn = __xfs_ail_min_lsn(ailp);
>  	if (!tail_lsn)
> -		tail_lsn = ailp->ail_head_lsn;
> +		tail_lsn = ailp->ail_tail_lsn;
>  
>  	WRITE_ONCE(log->l_tail_space,
>  			xlog_lsn_sub(log, ailp->ail_head_lsn, tail_lsn));
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index f945f0450b16..4ed9ada298ec 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -56,6 +56,7 @@ struct xfs_ail {
>  	spinlock_t		ail_lock;
>  	xfs_lsn_t		ail_last_pushed_lsn;
>  	xfs_lsn_t		ail_head_lsn;
> +	xfs_lsn_t		ail_tail_lsn;
>  	int			ail_log_flush;
>  	unsigned long		ail_opstate;
>  	struct list_head	ail_buf_list;
> -- 
> 2.39.2
>