Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 27, 2014 at 07:19:10AM -0400, Brian Foster wrote:
> On Wed, Aug 27, 2014 at 08:34:07AM +1000, Dave Chinner wrote:
> > > > +	switch (flags) {
> > > > +	/* expected flag values */
> > > > +	case 0:
> > > > +	case XLOG_CONTINUE_TRANS:
> > > > +		error = xlog_recover_add_to_trans(log, trans, dp, hlen);
> > > > +		break;
> > > > +	case XLOG_WAS_CONT_TRANS:
> > > > +		error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
> > > > +		break;
> > > > +	case XLOG_COMMIT_TRANS:
> > > > +		error = xlog_recover_commit_trans(log, trans, pass);
> > > > +		break;
> > > > +
> > > > +	/* unexpected flag values */
> > > > +	case XLOG_UNMOUNT_TRANS:
> > > > +		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> > > > +		error = 0;
> > > > +		break;
> > > > +	case XLOG_START_TRANS:
> > > > +		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
> > > > +		ASSERT(0);
> > > > +		break;
> > > 
> > > 		xlog_recover_new_tid(&rhash[hash], tid, be64_to_cpu(rhead->h_lsn)
> > > 		error = 0;
> > > 		break;
> > > 
> > 
> > I like the idea, but I don't like the suggested implementation. I
> > was in two minds as to whether I should factor
> > xlog_recover_find_tid() further.  There's only one caller of it and
> > only one caller of xlog_recover_new_tid() and the happen within
> > three lines of each other. Hence I'm thinking that it makes more
> > sense to wrap the "find or allocate trans" code in a single helper
> > and lift all that logic clean out of this function. That helper can
> > handle all the XLOG_START_TRANS logic more cleanly, I think....
> > 
> 
> Fair enough, that sounds reasonable so long as it isn't pulling the core
> state management off into disparate functions. What I like about the
> above (combined with the result of the rest of this series) is that the
> lifecycle is obvious and contained in a single block of code.

Well, it's not "state" as in "state machine". What we are doing is
decoding ophdrs, not walking through a state machine, and so I think
that the "start" ophdrs need to be treated differently because all
the other types of ophdrs have a dependency on the trans structure
existing.

Indeed, I suspect the correct thing to do is check for the start
flag *first*, before we do the lookup to find the trans structure,
because the start flag implies that we should not find an existing
trans structure for that tid. And once we've done that, we're
completely finished processing that ophdr, and hence should
return and not run any of the other code we run for all the
remaining ophdrs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs