On Mon, Mar 16, 2020 at 03:42:25PM +0100, Christoph Hellwig wrote:
> Factor out a few self-container helper from xlog_state_clean_iclog, and
> update the documentation so it primarily documents why things happens
> instead of how.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_log.c | 180 +++++++++++++++++++++++------------------------
> 1 file changed, 87 insertions(+), 93 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8ede2852f104..23979d08a2a3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2540,112 +2540,106 @@ xlog_write(
...
> +static int
> +xlog_covered_state(
> + struct xlog *log,
> + int iclogs_changed)
> +{
> /*
> - * Wake up threads waiting in xfs_log_force() for the dirty iclog
> - * to be cleaned.
> + * We usually go to NEED. But we go to NEED2 if the changed indicates we
> + * are done writing the dummy record. If we are done with the second
> + * dummy recored (DONE2), then we go to IDLE.
> */
> - wake_up_all(&dirty_iclog->ic_force_wait);
> + switch (log->l_covered_state) {
> + case XLOG_STATE_COVER_IDLE:
> + case XLOG_STATE_COVER_NEED:
> + case XLOG_STATE_COVER_NEED2:
> + break;
> + case XLOG_STATE_COVER_DONE:
> + if (iclogs_changed == 1)
> + return XLOG_STATE_COVER_NEED2;
> + break;
> + case XLOG_STATE_COVER_DONE2:
> + if (iclogs_changed == 1)
> + return XLOG_STATE_COVER_IDLE;
> + break;
> + default:
> + ASSERT(0);
> + }
The code looks mostly fine, but I'm not a fan of this factoring where we
deref ->l_covered_state here and return a value only for the caller to
assign it to ->l_covered_state again. Can we just let this function
assign ->l_covered_state itself (i.e. assign a local variable rather than
return within the switch)?
Brian
>
> - /*
> - * Change state for the dummy log recording.
> - * We usually go to NEED. But we go to NEED2 if the changed indicates
> - * we are done writing the dummy record.
> - * If we are done with the second dummy recored (DONE2), then
> - * we go to IDLE.
> - */
> - if (changed) {
> - switch (log->l_covered_state) {
> - case XLOG_STATE_COVER_IDLE:
> - case XLOG_STATE_COVER_NEED:
> - case XLOG_STATE_COVER_NEED2:
> - log->l_covered_state = XLOG_STATE_COVER_NEED;
> - break;
> + return XLOG_STATE_COVER_NEED;
> +}
>
> - case XLOG_STATE_COVER_DONE:
> - if (changed == 1)
> - log->l_covered_state = XLOG_STATE_COVER_NEED2;
> - else
> - log->l_covered_state = XLOG_STATE_COVER_NEED;
> - break;
> +STATIC void
> +xlog_state_clean_iclog(
> + struct xlog *log,
> + struct xlog_in_core *dirty_iclog)
> +{
> + int iclogs_changed = 0;
>
> - case XLOG_STATE_COVER_DONE2:
> - if (changed == 1)
> - log->l_covered_state = XLOG_STATE_COVER_IDLE;
> - else
> - log->l_covered_state = XLOG_STATE_COVER_NEED;
> - break;
> + if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> + dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>
> - default:
> - ASSERT(0);
> - }
> - }
> + xlog_state_activate_iclogs(log, &iclogs_changed);
> + wake_up_all(&dirty_iclog->ic_force_wait);
> +
> + if (iclogs_changed)
> + log->l_covered_state = xlog_covered_state(log, iclogs_changed);
> }
>
> STATIC xfs_lsn_t
> --
> 2.24.1
>
