On Thu, Jun 07, 2012 at 12:49:53PM -0400, Brian Foster wrote:
> xfsaild idle mode logic currently leads to a couple hangs:
>
> 1.) If xfsaild is rescheduled in during an incremental scan
> (i.e., tout != 0) and the target has been updated since
> the previous run, we can hit the new target and go into
> idle mode with a still populated ail.
> 2.) A wake up is only issued when the target is pushed forward.
> The wake up can race with xfsaild if it is currently in the
> process of entering idle mode, causing future wake up
> events to be lost.
>
> Both hangs are reproducible by running xfstests 273 in a loop.
> Modify xfsaild to enter idle mode only when the ail is empty
> and the push target has not been moved forward since the last
> push.
What tree is this against? The current XFS tree never fully idles,
so something is missing here, probably just in the patch description.
> spin_lock(&ailp->xa_lock);
> +
> + /* barrier matches the xa_target update in xfs_ail_push() */
> + smp_rmb();
> + target = ailp->xa_target;
> + ailp->xa_target_prev = target;
> +
> lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->xa_last_pushed_lsn);
> if (!lip) {
> /*
> +
> + spin_lock(&ailp->xa_lock);
> +
> + /*
> + * Idle if the AIL is empty and we are not racing with a target
> + * update. The barrier matches the xa_target update in
> + * xfs_ail_push().
> + */
> + smp_rmb();
Given that both sides are under xa_lock I can't see any need for
barriers here.
> + if (!xfs_ail_min(ailp) && (ailp->xa_target ==
> ailp->xa_target_prev)) {
> + spin_unlock(&ailp->xa_lock);
> + schedule();
> + tout = 0;
> + continue;
> + }
This seems to add the actual idling, but in a rather confusing way.
Can you add the xfs_ail_min and target checks to the end of xfsaild_push
so that they are in one place with the other decisions for the timeout.
Please also add a comment explaining the conditions when we want to
idle.
Also two small style nipicks:
- please make sure lines are shorter than 80 characters
- no need for the braces around the target comparism above.
|