On Wed, Apr 03, 2013 at 04:45:18PM -0500, Mark Tinguely wrote: > On 04/03/13 16:02, Eric Sandeen wrote: > >On 4/3/13 2:46 PM, Eric Sandeen wrote: > >>On 4/3/13 2:12 PM, Mark Tinguely wrote: > >>>On 04/02/13 22:09, Dave Chinner wrote: > >>>>From: Dave Chinner<dchinner@xxxxxxxxxx> > >>>> > >>>>Filesystems are occasionally being shut down with this error: > >>>> > >>>>xfs_trans_ail_delete_bulk: attempting to delete a log item that is > >>>>not in the AIL. > >>>> > >>>>It was diagnosed to be related to the EFI/EFD commit order when the > >>>>EFI and EFD are in different checkpoints and the EFD is committed > >>>>before the EFI here: > >>>> > >>>>http://oss.sgi.com/archives/xfs/2013-01/msg00082.html > >>>> > >>>>The real problem is that a single bit cannot fully describe the > >>>>states that the EFI/EFD processing can be in. These completion > >>>>states are: > >>>> > >>>>EFI EFI in AIL EFD Result > >>>>committed/unpinned Yes committed OK > >>>>committed/pinned No committed Shutdown > >>>>uncommitted No committed Shutdown > >>>> > >>>> > >>>>Note that the "result" field is what should happen, not what does > >>>>happen. The current logic is broken and handles the first two cases > >>>>correctly by luck. That is, the code will free the EFI if the > >>>>XFS_EFI_COMMITTED bit is*not* set, rather than if it is set. The > >>>>inverted logic "works" because if both EFI and EFD are committed, > >>>>then the first __xfs_efi_release() call clears the XFS_EFI_COMMITTED > >>>>bit, and the second frees the EFI item. Hence as long as > >>>>xfs_efi_item_committed() has been called, everything appears to be > >>>>fine. > >>>> > >>>>It is the third case where the logic fails - where > >>>>xfs_efd_item_committed() is called before xfs_efi_item_committed(), > >>>>and that results in the EFI being freed before it has been > >>>>committed. That is the bug that triggered the shutdown, d hence > >>>>keeping track of whether the EFI has been committed or not is > >>>>insufficient to correctly order the EFI/EFD operations w.r.t. the > >>>>AIL. > >>>> > >>> > >>>I think the exist xfs_efi routines are working correctly. > >>> > >>>the xfs_trans_committed_bulk() for the efi does the IOP_COMMITTED > >>>(sets the XFS_EFI_COMMITTED bit), add the entry to the AIL and then > >>>an IOP_UNPIN() which clears the XFS_EFI_COMMITTED since it is set, it > >>>should not release the efi and remove the entry from the AIL. It also > >>>correctly detects if the EFD is committed before the EFI by shutting > >>>down the filesystem. > >> > >>Well hang on, not everything can be cool in EFI-land here, if nothing > >>else this: > >> > >>__xfs_efi_release( > >> struct xfs_efi_log_item *efip) > >>{ > >> struct xfs_ail *ailp = efip->efi_item.li_ailp; > >> > >> if (!test_and_clear_bit(XFS_EFI_COMMITTED,&efip->efi_flags)) { > >> spin_lock(&ailp->xa_lock); > >> /* xfs_trans_ail_delete() drops the AIL lock. */ > >> xfs_trans_ail_delete(ailp,&efip->efi_item, > >> SHUTDOWN_LOG_IO_ERROR); > >> xfs_efi_item_free(efip); > >> } > >>} > >> > >>seems obviously broken. > >> > >> * test_and_clear_bit - Clear a bit and return its old value > >> > >>so it is saying if XFS_EFI_COMMITED was *not* set, free the efi. > >>Which I think we must admit is broken. > >> > >>I have to get up to speed on this code, but it seems like at least > >>this much above is quite obviously broken, and the previously > >>proposed patch doesn't address it. Am I missing something? > > > >Or are you saying that it's working as designed, so that the first > >caller finds it set, clears it and does nothing, and the 2nd caller > >finds it unset, and frees it? help me out here ;) > > > >-Eric > > > > Yes. The order is IOP_COMMITTED and then IOP_UNPIN: Batching means you can get: IOP_COMMITTED(a) IOP_COMMITTED(b) IOP_COMMITTED(c) ..... IOP_COMMITTED(6) AIL_INSERT(a) AIL_INSERT(b) AIL_INSERT(c) ..... AIL_INSERT(6) IOP_UNPIN(a) IOP_UNPIN(b) IOP_UNPIN(c) ..... IOP_UNPIN(6) So that means at minimum we have to handle the EFI committed/pinned case when the EFD is committed as well. This is what the commit b199c8a (xfs: Pull EFI/EFD handling out from under the AIL lock) did - it moved the COMMITTED bit from the EFI IOP_UNPIN() callback to the IOP_COMMITTED() callback. But then the IOP_UNPIN() callback has to handle the same case by freeing the EFI as well, and hence the XFS_EFI_COMMITTED() bit became a proxy reference count. Which doesn't handle the third case, which is the EFD being processed before the EFI, and hence we get a shutdown because the EFD is trying to remove the EFI from the AIL before it is inserted. > The code makes sure the sequence EFI and then EFD are done in order. No, it doesn't ensure they are done in order - it *works* when they are done in order. If it ensured they were done in order, then the EFD commit would not be allowed to proceed until the EFI wsa committed. The code does not do that, not does it need to. But it needs to handle the EFD being processed before the EFI... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
