public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 2/8] xfs: fix efi item leak on forced shutdown
Date: Tue, 25 Jan 2011 18:53:54 -0500	[thread overview]
Message-ID: <20110125235354.GA3832@infradead.org> (raw)
In-Reply-To: <1295945444-29488-3-git-send-email-david@fromorbit.com>

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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  reply	other threads:[~2011-01-25 23:51 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-25  8:50 [PATCH 0/8] xfs: 2.6.38-rc candidate fixes V2 Dave Chinner
2011-01-25  8:50 ` [PATCH 1/8] xfs: fix log ticket leak on forced shutdown Dave Chinner
2011-01-26 21:22   ` Alex Elder
2011-01-25  8:50 ` [PATCH 2/8] xfs: fix efi item " Dave Chinner
2011-01-25 23:53   ` Christoph Hellwig [this message]
2011-01-27  0:35     ` Dave Chinner
2011-01-26 21:22   ` Alex Elder
2011-01-25  8:50 ` [PATCH 3/8] xfs: speculative delayed allocation uses rounddown_power_of_2 badly Dave Chinner
2011-01-26 21:22   ` Alex Elder
2011-01-25  8:50 ` [PATCH 4/8] xfs: limit extent length for allocation to AG size Dave Chinner
2011-01-26 21:22   ` Alex Elder
2011-01-27  0:38     ` Dave Chinner
2011-01-25  8:50 ` [PATCH 5/8] xfs: prevent extsize alignment from exceeding maximum extent size Dave Chinner
2011-01-25  9:49   ` Christoph Hellwig
2011-01-26 21:22   ` Alex Elder
2011-01-27  0:50     ` Dave Chinner
2011-01-25  8:50 ` [PATCH 6/8] xfs: limit extsize to size of AGs and/or MAXEXTLEN Dave Chinner
2011-01-26 21:23   ` Alex Elder
2011-01-25  8:50 ` [PATCH 7/8] xfs: handle CIl transaction commit failures correctly Dave Chinner
2011-01-25  9:53   ` Christoph Hellwig
2011-01-26 21:23   ` Alex Elder
2011-01-25  8:50 ` [PATCH 8/8] xfs: fix dquot shaker deadlock Dave Chinner
2011-01-25  9:52   ` Christoph Hellwig
2011-01-27  1:54     ` Dave Chinner
2011-01-27  2:24       ` Dave Chinner
2011-01-26 21:23   ` Alex Elder
2011-01-25  9:20 ` [PATCH 0/8] xfs: 2.6.38-rc candidate fixes V2 Christoph Hellwig
2011-01-26 21:23 ` Alex Elder
  -- strict thread matches above, loose matches on Subject: below --
2011-01-27  3:53 [PATCH 0/8] xfs: candidate 2.6.38-rc fixes V3 Dave Chinner
2011-01-27  3:53 ` [PATCH 2/8] xfs: fix efi item leak on forced shutdown Dave Chinner
2011-01-28 14:54   ` Alex Elder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110125235354.GA3832@infradead.org \
    --to=hch@infradead.org \
    --cc=david@fromorbit.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox