xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: remove efi from AIL in log recovery error

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH 1/2] xfs: remove efi from AIL in log recovery error
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 28 Mar 2014 11:24:35 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140325195819.638326569@xxxxxxx>
References: <20140325195733.510384972@xxxxxxx> <20140325195819.638326569@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Mar 25, 2014 at 03:06:34PM -0500, Mark Tinguely wrote:
> xlog_recover_process_efi{s}() functions are completing the
> second half of xfs_bmap_finish() which frees extents. If this
> operation fails, the EFI will stay on the AIL and prevents
> the xfs_ail_push all_sync() from completing and the mount will
> fail to unmount.
> 
> Rather than have a special log recovery flag XFS_EFI_RECOVERED
> to decrement the EFI/EFD counter, call the same decrement function
> from the log recovery routine that is called then the EFI is added
> to the AIL from a log write.
> 
> Remove all other unprocessed EFIs from the log recovery AIL
> when one is discovered in error.
> 
> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> ---
> Rewritten with suggestions from Dave.
> Note: calling xfs_efi_item_unpin() seemed more explainatory than calling
>       the helper __xfs_efi_release().
> 
>  fs/xfs/xfs_extfree_item.c |    9 +++------
>  fs/xfs/xfs_log_recover.c  |   28 +++++++++++++++-------------
>  fs/xfs/xfs_trans.h        |    1 +
>  3 files changed, 19 insertions(+), 19 deletions(-)
> 
> Index: b/fs/xfs/xfs_extfree_item.c
> ===================================================================
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -134,9 +134,10 @@ xfs_efi_item_pin(
>   * remove the EFI it's because the transaction has been canceled and by
>   * definition that means the EFI cannot be in the AIL so remove it from the
>   * transaction and free it.  Otherwise coordinate with xfs_efi_release()
> - * to determine who gets to free the EFI.
> + * to determine who gets to free the EFI. Call from log recovery of EFI
> + * entries so the EFD or error handling will remove the entry.
>   */
> -STATIC void
> +void
>  xfs_efi_item_unpin(
>       struct xfs_log_item     *lip,
>       int                     remove)
> @@ -313,10 +314,6 @@ xfs_efi_release(xfs_efi_log_item_t       *efip
>  {
>       ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
>       if (atomic_sub_and_test(nextents, &efip->efi_next_extent)) {
> -             /* recovery needs us to drop the EFI reference, too */
> -             if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
> -                     __xfs_efi_release(efip);
> -
>               __xfs_efi_release(efip);
>               /* efip may now have been freed, do not reference it again. */
>       }
> Index: b/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3634,6 +3634,7 @@ xlog_recover_process_data(
>  /*
>   * Process an extent free intent item that was recovered from
>   * the log.  We need to free the extents that it describes.
> + * The caller will release this and any following EFIs upon error.
>   */
>  STATIC int
>  xlog_recover_process_efi(
> @@ -3648,6 +3649,13 @@ xlog_recover_process_efi(
>       xfs_fsblock_t           startblock_fsb;
>  
>       ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
> +     set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
> +
> +     /*
> +      * Decrement the EFI/EFD counter so the EFI is removed after
> +      * processing the EFD or error handling in the caller.
> +      */
> +     xfs_efi_item_unpin(&efip->efi_item, 0);
>  
>       /*
>        * First check the validity of the extents described by the
> @@ -3662,12 +3670,6 @@ xlog_recover_process_efi(
>                   (extp->ext_len == 0) ||
>                   (startblock_fsb >= mp->m_sb.sb_dblocks) ||
>                   (extp->ext_len >= mp->m_sb.sb_agblocks)) {
> -                     /*
> -                      * This will pull the EFI from the AIL and
> -                      * free the memory associated with it.
> -                      */
> -                     set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
> -                     xfs_efi_release(efip, efip->efi_format.efi_nextents);
>                       return XFS_ERROR(EIO);
>               }
>       }
> @@ -3687,7 +3689,6 @@ xlog_recover_process_efi(
>                                        extp->ext_len);
>       }
>  
> -     set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
>       error = xfs_trans_commit(tp, 0);
>       return error;
>  
> @@ -3718,8 +3719,8 @@ STATIC int
>  xlog_recover_process_efis(
>       struct xlog     *log)
>  {
> -     xfs_log_item_t          *lip;
> -     xfs_efi_log_item_t      *efip;
> +     struct xfs_log_item     *lip;
> +     struct xfs_efi_log_item *efip;
>       int                     error = 0;
>       struct xfs_ail_cursor   cur;
>       struct xfs_ail          *ailp;
> @@ -3750,13 +3751,14 @@ xlog_recover_process_efis(
>               }
>  
>               spin_unlock(&ailp->xa_lock);
> -             error = xlog_recover_process_efi(log->l_mp, efip);
> -             spin_lock(&ailp->xa_lock);
> +             /* Skip all EFIs after first EFI in error. */
> +             if (!error)
> +                     error = xlog_recover_process_efi(log->l_mp, efip);
>               if (error)
> -                     goto out;
> +                     xfs_efi_release(efip, efip->efi_format.efi_nextents);

Hi Mark,

If we hit the scenario where we start skipping EFIs after an error, is
the equivalent unpin() call from process_efi() not necessary on the
subsequent EFIs?

Brian

> +             spin_lock(&ailp->xa_lock);
>               lip = xfs_trans_ail_cursor_next(ailp, &cur);
>       }
> -out:
>       xfs_trans_ail_cursor_done(ailp, &cur);
>       spin_unlock(&ailp->xa_lock);
>       return error;
> Index: b/fs/xfs/xfs_trans.h
> ===================================================================
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -216,6 +216,7 @@ void              xfs_trans_ijoin(struct xfs_trans *
>  void         xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
>  void         xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
>  struct xfs_efi_log_item      *xfs_trans_get_efi(xfs_trans_t *, uint);
> +void         xfs_efi_item_unpin(struct xfs_log_item *, int);
>  void         xfs_efi_release(struct xfs_efi_log_item *, uint);
>  void         xfs_trans_log_efi_extent(xfs_trans_t *,
>                                        struct xfs_efi_log_item *,
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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