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@xxxxxxxxxxxxx
