On Tue, Sep 03, 2019 at 11:42:21PM -0700, Christoph Hellwig wrote:
> On Wed, Sep 04, 2019 at 02:24:49PM +1000, Dave Chinner wrote:
> > + /* Skip all iclogs in the ACTIVE & DIRTY states */
> > + if (iclog->ic_state & (XLOG_STATE_ACTIVE|XLOG_STATE_DIRTY))
> > + return false;
>
> Please use spaces around the "|".
Fixed.
>
> > + if (iclog->ic_state & XLOG_STATE_IOERROR)
> > + ioerrors++;
>
> This now also counts the ierrror flag for dirty and active iclogs.
> Not sure it matters given our state machine, but it does change
> behavior.
True. There's an ioerror check in xlog_state_iodone_process_iclog()
so I can pass the ioerror parameter into it and retain the existing
logic.
> > + ret = xlog_state_iodone_process_iclog(log, iclog,
> > + ciclog);
> > + if (ret)
> > + break;
>
> No need for the ret variable.
Fixed.
> >
> > - } else
> > - ioerrors++;
> > + if (!(iclog->ic_state &
> > + (XLOG_STATE_CALLBACK | XLOG_STATE_IOERROR))) {
> > + iclog = iclog->ic_next;
> > + continue;
> > + }
>
> Btw, one cleanup I had pending is that all our loops ovr the iclog
> list can be cleaned up nicely so that continue does that right thing
> without all these manual "iclog = iclog->ic_next" next statements. Just
> turn the loop into:
>
> do {
> ..
> } while ((iclog = iclog->ic_next) != first_iclog);
>
> this might be applicable to a few of your patches.
Yup, as I mentioned earlier, that's in progress :P
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
