Re: [PATCH 0/8 V2] xfs: log fixes for for-next

Dave Chinner
2021-06-18T22:28:47Z

On Fri, Jun 18, 2021 at 02:55:03PM +0100, Christoph Hellwig wrote:
> On Fri, Jun 18, 2021 at 09:08:15AM -0400, Brian Foster wrote:
> > Also FYI, earlier iterations of generic/475 triggered a couple instances
> > of the following assert failure before things broke down more severely:
> >
> > XFS: Assertion failed: *log_offset + *len <= iclog->ic_size || iclog->ic_state == XLOG_STATE_WANT_SYNC, file: fs/xfs/xfs_log.c, line: 2115
>
> As you mentioned the placement of this exact assert in my cleanups
> series: after looking at a right place to move it, I'm really not sure
> this assert makes much sense in this form.

It actually makes perfect sense when you look at the iclog state
transitions in xlog_state_get_iclog_space() w.r.t. the length that
is passed to it.

> xlog_write_single is always entered first by xlog_write, so we also
> get here for something that later gets handled by xlog_write_partial.
> Which means it could be way bigger than the current iclog, and I see no
> reason why that iclog would have to be XLOG_STATE_WANT_SYNC.

Yup, completely intentional and if len is larger than can fit in the
iclog we are writing into, the iclog *must* be in
XLOG_STATE_WANT_SYNC.

That is, if the length requested in xlog_state_get_iclog_space()
fits entirely in the iclog that is returned, _get_space() will
increment the offset of the iclog to exclusively reserve that amount
of space for the write we are going to do. It then leaves the state
as ACTIVE so another process can then also reserve some/all of the
remaining unused space in the iclog. Hence here in
xlog_write_single() we will have *log_offset + *len <=
iclog->ic_size and ic_state = ACTIVE as true for a write that fits
entirely in the iclog.

If _get_space() finds that the len is larger than will fit in the
iclog, it will reserve the entire remaining space in the iclog for the current caller
by switching out the iclog and moving the state to
XLOG_STATE_WANT_SYNC. This means no other caller to _get_space() will
be able to reserve space in this iclog because the state is no
longer ACTIVE.

IOWs, if *log_offset + *len > iclog->ic_size, then _get_space()
*must* have set the state of the iclog to _WANT_SYNC so that the
owner of the iclog has exclusive use of the space in the iclog from
*log_offset all the way to the end of the iclog. The overlap beyond
the end of this iclog will be handled by the xlog_write_partial(),
and it will release this iclog and get a new one to continue the
write.

Long story short, the assert is valid, but asynchronous shutdown
changing ic_state without having references to the iclogs or caring
about how they are being used is turning out to be a massive Charlie
Foxtrot right now...

Cheers,

Dave.
>

--
Dave Chinner
david@fromorbit.com