On Thu, Mar 19, 2020 at 02:05:36PM +0100, Christoph Hellwig wrote:
> On Thu, Mar 19, 2020 at 07:36:03AM -0400, Brian Foster wrote:
> > > True. I think we just need to clear cycled_icloglock in the
> > > shutdown branch. I prefer that flow over falling through to the
> > > main loop body as that clearly separates out the shutdown case.
> > >
> >
> > Sure, but a shutdown can still happen at any point so this is just a
> > duplicate branch to maintain.
>
> I don't understand. We are in the inner loop and under l_icloglock.
> The next time a shutdown can come in is when
> xlog_state_do_iclog_callbacks drops l_icloglock. That is at the end
> of the inner loop, which means we will always go back to the
> force shutdown check quickly. So how is the branch duplicate? Yes,
> it also calls xlog_state_do_iclog_callbacks and does the wakeup,
> but in doing that early it avoid a whole lot of complicated logic
> in the previous code base.
>
We'll get back to the shutdown check for the next iclog, but not the
iclog we're running callbacks on. So basically we can be in CALLBACK, a
shutdown can set IOERROR where _do_iclog_callbacks() cycles the lock,
the callback picks up the shutdown state and aborts, and then the first
thing we do after that function returns is:
iclog->ic_state = XLOG_STATE_DIRTY;
xlog_state_activate_iclogs(log, &iclogs_changed);
... and thus finish the loop by reactivating the (IOERROR) iclog. So for
any particular iclog, we might process it for shutdown normally or in
the special shutdown branch based on timing.
> > I think you're misreading me. I'm not suggesting to fake state changes.
> > I'd argue that's actually what the special case shutdown branch does.
> > And to the contrary, this patch already implements what I'm suggesting,
> > it's just not consistent behavior..
>
> I'm rather confused now.
>
Sorry. What I'm saying can probably be simplified to the following
question: if we just removed the special shutdown branch and let the
iclogs fall through the normal completion sequence (once IOERROR is out
of the picture) during shutdown, is that actually a problem?
It seems to me it isn't (subject to testing of course). If that is true,
then that is more simple and consistent than what we seem to be doing in
this patch, which to my eyes seems to want to maintain some of the
IOERROR functional cruft even though the state itself is being removed.
Also note I think it would be reasonable to lift it out in a
later/separate patch if that was more straightforward than reworking
these patches.
If it is a problem, I think that's a potential argument for leaving the
IOERROR state around because then the state technically has meaning.
> > First, we basically already go from whatever state we're in to "logical
> > CALLBACK" during shutdown. This is just forcibly implemented via the
> > IOERROR state. With IOERROR eventually removed, this highlights things
> > like whether it's actually safe to make some of those arbitrary
> > transitions. It's actually not, because going from WANT_SYNC -> CALLBACK
> > is a potential use after free vector of the CIL ctx (as soon as the ctx
> > is added to the callback list in the CIL push code). This is yet another
> > functional problem that should be fixed before removing IOERROR, IMO
> > (and is reproducible via kasan splat, btw). At this point I think some
> > of these shutdown checks associated with CALLBACK are simply to ensure
> > IOERROR remains persistent once it's set on an iclog. We don't need to
> > carry that logic around if IOERROR is going away.
>
> What shutdown check associated with CALLBACK?
>
The one(s) that issue callbacks on an IOERROR iclog (note that I'm
referring to the mainline code). Specifically the IOERROR check in
_process_iclog() and the following logic in the caller:
if (iclog->ic_state != XLOG_STATE_CALLBACK &&
iclog->ic_state != XLOG_STATE_IOERROR) {
iclog = iclog->ic_next;
continue;
}
IOW, the current code treats an IOERROR iclog as if it were CALLBACK
with the sole exception of updating ic_state.
> > SYNCING -> CALLBACK is another hokey transition in the existing code,
> > even if it doesn't currently manifest in a bug that I can see, because
> > we should probably still expect (wait for) an I/O completion despite
> > that the filesystem had shutdown in the meantime. Fixing that one might
> > require tweaks to how the shutdown code actually works (i.e. waiting on
> > an I/O vs. running callbacks while in-flight). It's not immediately
> > clear to me what the best solution is for that, but I suspect it could
> > tie in with fixing the problem noted above.
>
> True, actually running callbacks on various kinds of "in-flight" iclogs
> seems rather dangerous. So should I interpret your above comments
> in that we should fix that first before killing of the IOERROR state?
>
Yes. Note that the WANT_SYNC -> CALLBACK (IOERROR) behavior is
explicitly problematic in the current code as well because the submitter
context still has the ctx while the callbacks could be freeing it.
That's a reproducible use after free in the current code.
Brian
