On Tue, Mar 15, 2022 at 05:42:37PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> xfs_ail_push_all_sync() has a loop like this:
>
> while max_ail_lsn {
> prepare_to_wait(ail_empty)
> target = max_ail_lsn
> wake_up(ail_task);
> schedule()
> }
>
> Which is designed to sleep until the AIL is emptied. When
> xfs_ail_finish_update() moves the tail of the log, it does:
>
> if (list_empty(&ailp->ail_head))
> wake_up_all(&ailp->ail_empty);
>
> So it will only wake up the sync push waiter when the AIL goes
> empty. If, by the time the push waiter has woken, the AIL has more
> in it, it will reset the target, wake the push task and go back to
> sleep.
>
> The problem here is that if the AIL is having items added to it
> when xfs_ail_push_all_sync() is called, then they may get inserted
> into the AIL at a LSN higher than the target LSN. At this point,
> xfsaild_push() will see that the target is X, the item LSNs are
> (X+N) and skip over them, hence never pushing the out.
>
> The result of this the AIL will not get emptied by the AIL push
> thread, hence xfs_ail_finish_update() will never see the AIL being
> empty even if it moves the tail. Hence xfs_ail_push_all_sync() never
> gets woken and hence cannot update the push target to capture the
> items beyond the current target on the LSN.
>
> This is a TOCTOU type of issue so the way to avoid it is to not
> use the push target at all for sync pushes. We know that a sync push
> is being requested by the fact the ail_empty wait queue is active,
> hence the xfsaild can just set the target to max_ail_lsn on every
> push that we see the wait queue active. Hence we no longer will
> leave items on the AIL that are beyond the LSN sampled at the start
> of a sync push.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_trans_ail.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 2a8c8dc54c95..1b52952097c1 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -448,10 +448,22 @@ xfsaild_push(
>
> spin_lock(&ailp->ail_lock);
>
> - /* barrier matches the ail_target update in xfs_ail_push() */
> - smp_rmb();
> - target = ailp->ail_target;
> - ailp->ail_target_prev = target;
> + /*
> + * If we have a sync push waiter, we always have to push till the AIL is
> + * empty. Update the target to point to the end of the AIL so that
> + * capture updates that occur after the sync push waiter has gone to
> + * sleep.
> + */
> + if (waitqueue_active(&ailp->ail_empty)) {
> + lip = xfs_ail_max(ailp);
> + if (lip)
> + target = lip->li_lsn;
> + } else {
> + /* barrier matches the ail_target update in xfs_ail_push() */
> + smp_rmb();
Doesn't the spin_lock provide the required rmb? I think it's
unnecessary given that, but I also don't think it hurts anything, so:
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> + target = ailp->ail_target;
> + ailp->ail_target_prev = target;
> + }
>
> /* we're done if the AIL is empty or our push has reached the end */
> lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->ail_last_pushed_lsn);
> @@ -724,7 +736,6 @@ xfs_ail_push_all_sync(
> spin_lock(&ailp->ail_lock);
> while ((lip = xfs_ail_max(ailp)) != NULL) {
> prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
> - ailp->ail_target = lip->li_lsn;
> wake_up_process(ailp->ail_task);
> spin_unlock(&ailp->ail_lock);
> schedule();
> --
> 2.35.1
>
