On Fri, Aug 07, 2015 at 10:41:00AM +1000, Dave Chinner wrote:
> On Thu, Aug 06, 2015 at 01:44:24PM -0400, Brian Foster wrote:
> ....
> > Update the EFI/EFD item handling code to use a more straightforward and
> > reliable approach to error handling. If an error occurs after the EFI
> > transaction is committed and before the EFD is constructed, release the
> > EFI explicitly from xfs_bmap_finish(). If the EFI transaction is
> > cancelled, release the EFI in the unlock handler.
> >
> > Once the EFD is constructed, it is responsible for releasing the EFI
> > under any circumstances (including whether the EFI item aborts due to
> > log I/O error).
>
> Can you document these rules in a comment at the top of
> fs/xfs/xfs_extfree_item.c?
>
Sure.
> > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > index 42c9b05..aceb54f 100644
> > --- a/fs/xfs/xfs_extfree_item.c
> > +++ b/fs/xfs/xfs_extfree_item.c
> > @@ -61,9 +61,15 @@ __xfs_efi_release(
> >
> > if (atomic_dec_and_test(&efip->efi_refcount)) {
> > 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);
> > + /*
> > + * We don't know whether the EFI made it to the AIL. Remove it
> > + * if so. Note that xfs_trans_ail_delete() drops the AIL lock.
> > + */
> > + if (efip->efi_item.li_flags & XFS_LI_IN_AIL)
> > + xfs_trans_ail_delete(ailp, &efip->efi_item,
> > + SHUTDOWN_LOG_IO_ERROR);
> > + else
> > + spin_unlock(&ailp->xa_lock);
> > xfs_efi_item_free(efip);
> > }
>
> We now have a lot of this pattern:
>
> spin_lock(&ailp->xa_lock);
> if (lip->li_flags & XFS_LI_IN_AIL)
> xfs_trans_ail_delete(ailp, lip, shutdown_reason);
> else
> spin_unlock(&ailp->xa_lock);
>
> occurring in the code. Can you look into adding a helper function
> for this, say xfs_trans_ail_remove()? (separate patch, of course!)
>
There was only one other instance of this with regard to the EFI that is
ultimately replaced with an xfs_efi_release() call. I'll look into
whether there's enough instances of this for other items to justify a
helper.
Brian
> The rest of the patch looks fine - getting rid of almost all of
> those aborted flag special cases is a big win :)
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
