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

[Chandan Babu R <chandan.babu@oracle.com>]

On 17 Mar 2022 at 04:54, Dave Chinner wrote:
> 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.

Ah. I had missed xlog_cil_push_background().

>
> 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).
>

Thanks for clarifying my doubts.

-- 
chandan