Re: [PATCH 4/7] xfs: async CIL flushes need pending pushes to be made stable

[Dave Chinner <david@fromorbit.com>]

On Wed, Mar 16, 2022 at 04:04:55PM +0530, Chandan Babu R wrote:
> On 15 Mar 2022 at 12:12, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > When the AIL tries to flush the CIL, it relies on the CIL push
> > ending up on stable storage without having to wait for and
> > manipulate iclog state directly. However, if there is already a
> > pending CIL push when the AIL tries to flush the CIL, it won't set
> > the cil->xc_push_commit_stable flag and so the CIL push will not
> > actively flush the commit record iclog.
> 
> I think the above sentence maps to the following snippet from
> xlog_cil_push_now(),
> 
> 	if (list_empty(&cil->xc_cil) || push_seq <= cil->xc_push_seq) {
> 		spin_unlock(&cil->xc_push_lock);
> 		return;
> 	}
> 
> i.e. if the CIL sequence that we are trying to push is already being pushed
> then xlog_cil_push_now() returns without queuing work on cil->xc_push_wq.
> 
> However, the push_seq could have been previously pushed by,
> 1. xfsaild_push()
>    In this case, cil->xc_push_commit_stable is set to true. Hence,
>    xlog_cil_push_work() will definitely make sure to submit the commit record
>    iclog for write I/O.
> 2. xfs_log_force_seq() => xlog_cil_force_seq()
>    xfs_log_force_seq() invokes xlog_force_lsn() after executing
>    xlog_cil_force_seq(). Here, A partially filled iclog will be in
>    XLOG_STATE_ACTIVE state. This will cause xlog_force_and_check_iclog() to be
>    invoked and hence the iclog is submitted for write I/O.
> 
> In both the cases listed above, iclog is guaranteed to be submitted for I/O
> without any help from the log worker task.
> 
> Looks like I am missing something obvious here.

Pushes triggered by xlog_cil_push_background() can complete leaving
the partially filled iclog in ACTIVE state. Then xlog_cil_push_now()
does nothing because it doesn't trigger a new CIL push and so
setting the cil->xc_push_commit_stable flag doesn't trigger a flush
of the ACTIVE iclog.

The AIL flush does not use xfs_log_force_seq() because that blocks
waiting for the entire CIL to hit the disk before it can force the
last iclog to disk. Hence the second piece of this patch is
necessary, and that is to call xfs_log_force() if the CIL is empty
(i.e. the case where xlog_cil_push_now() is a no-op because the
CIL is empty due to background pushes).


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com