xfs
[Top] [All Lists]

Re: [PATCH] xfs: free the EFI entries from AIL on forced shutdown

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH] xfs: free the EFI entries from AIL on forced shutdown
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 21 Mar 2014 10:11:01 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <532B45E1.3060309@xxxxxxx>
References: <20140314163723.916178776@xxxxxxx> <20140317052951.GE7072@dastard> <532B45E1.3060309@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Mar 20, 2014 at 02:47:45PM -0500, Mark Tinguely wrote:
> On 03/17/14 00:29, Dave Chinner wrote:
> >On Fri, Mar 14, 2014 at 11:37:01AM -0500, Mark Tinguely wrote:
> >>@@ -439,10 +446,15 @@ xfs_efd_item_committed(
> >>    struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
> >>
> >>    /*
> >>-    * If we got a log I/O error, it's always the case that the LR with the
> >>-    * EFI got unpinned and freed before the EFD got aborted.
> >>+    * If we got a log I/O error and the EFI is also in this buffer, it
> >>+    * will be unpinned and freed before the EFD got aborted. But the EFI
> >>+    * is in an earlier transaction and could be on the AIL when the log
> >>+    * I/O error happened for this EFD. In that case, manually remove the
> >>+    * remaining EFIs from the AIL.
> >>     */
> >>-   if (!(lip->li_flags&  XFS_LI_ABORTED))
> >>+   if (lip->li_flags&  XFS_LI_ABORTED)
> >>+           xfs_efi_clear_ail(lip->li_ailp);
> >>+   else
> >>            xfs_efi_release(efdp->efd_efip, efdp->efd_format.efd_nextents);
> >
> >Same here - we can simply always call xfs_efi_release() and we'll
> >end up doing the right thing w.r.t. the EFi reference count.
> >
> >And if we do the right thing with the EFI reference count when
> >aborting the EFI (i.e. call __xfs_efi_release() rather than freeing
> >it) the aborting of transactions will correctly free all the
> >references to the EFI and remove them from the AIL.
> 
> This is fine if the EFI made it to the AIL before the log abort -
> which it may or may not have done.
> 
> Log errors will not force the log. The call to
> xfs_trans_committed_bulk() with the abort flag set will not place
> the EFI on the AIL (takes the iop_unpin/continue path).

Sure - we don't want it added to the AIL on abort. Instead, we want
the reference count that was intended for the AIL to be dropped when
we abort the object so that we can free the object when the last
reference count goes away.

> Your proposal of changing xfs_efi_committed() from freeing the EFI
> to a call to __xfs_efi_release() and changing xfs_efd_committed() to
> xfs_efi_release() will do the right thing for the counters and
> removing both the EFI/EFD entries, but it will also want to remove
> the non-existent EFI entry from the AIL. I do not like adding the
> EFI to the AIL in the abort path.

If the EFI is not in the AIL, then don't try to remove it from the
AIL.  We do this for all sorts of objects on abort/shutdown. e.g.
the abort path in xfs_buf_item_unlock() checks if the item is in the
AIL before removing it. We do the same when flushing a dquot and a
shutdown is encountered. Same for aborting an inode flush, or
completing an inode cluster freeing.

> >The only case we
> >then have to handle specially is the xfs_trans_reserve failure in
> >xfs_bmap_finish(), where we need to release the EFI directly in the
> >error path.
> 
> any reason to not move the efd creation earlier to not special case it?

You can't join objects to a transaction before you've reserved space
for the transaction.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>