xfs
[Top] [All Lists]

Re: [PATCH 2/8] xfs: fix efi item leak on forced shutdown

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/8] xfs: fix efi item leak on forced shutdown
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 25 Jan 2011 18:53:54 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1295945444-29488-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1295945444-29488-1-git-send-email-david@xxxxxxxxxxxxx> <1295945444-29488-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Jan 25, 2011 at 07:50:38PM +1100, Dave Chinner wrote:
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 75f2ef6..effbb41 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -138,7 +138,6 @@ xfs_efi_item_unpin(
>  
>       if (remove) {
>               ASSERT(!(lip->li_flags & XFS_LI_IN_AIL));
> -             xfs_trans_del_item(lip);
>               xfs_efi_item_free(efip);
>               return;
>       }

>  STATIC void
>  xfs_trans_uncommit(
>       struct xfs_trans        *tp,
>       uint                    flags)
>  {
> -     struct xfs_log_item_desc *lidp;
> +     struct xfs_log_item_desc *lidp, *n;
>  
> -     list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> +     list_for_each_entry_safe(lidp, n, &tp->t_items, lid_trans) {
>               /*
>                * Unpin all but those that aren't dirty.
>                */
> -             if (lidp->lid_flags & XFS_LID_DIRTY)
> +             if (lidp->lid_flags & XFS_LID_DIRTY) {
> +                     xfs_trans_del_item(lidp->lid_item);
>                       IOP_UNPIN(lidp->lid_item, 1);
> +             }

This part of the patch isn't explained in the changelog, and I suspect
it should be a separate patch from the addition of the IOP_UNPIN with
remove call to the CIL error path.

Moving the xfs_trans_del_item from the IOP_UNPIN implementation into
the caller seems sane to me, but:

 - why is the call to xfs_trans_del_item left in xfs_buf_item_unpin
 - why did xfs_buf_item_unpin get away only calling it for the stale
   case, and the other log item implementations completely without
   it

I suspect the answer lies in xfs_trans_free_items opencoding the
call to xfs_trans_del_item, thus avoiding any leak of log item
descriptors or log items on the transaction list.  By adding the
call of xfs_trans_del_item to xfs_trans_uncommit we thus skip
the calls to IOP_UNLOCK for dirty log items, which is a large
change in behaviour for the existing log items that didn't have
the xfs_trans_del_item calls, and at least for the dquot and
inode items seems incorrect.

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