xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: fix efi item leak on forced shutdown
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 20 Jan 2011 08:27:38 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110118233346.GA28803@dastard>
References: <1295010430-12495-1-git-send-email-david@xxxxxxxxxxxxx> <1295010430-12495-3-git-send-email-david@xxxxxxxxxxxxx> <20110118124625.GB12516@xxxxxxxxxxxxx> <20110118233346.GA28803@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Jan 19, 2011 at 10:33:46AM +1100, Dave Chinner wrote:
> > Hmm, this is not symmetric with the non-delaylog path.
> > xfs_trans_item_committed never sets the remove flag to IOP_UNPIN,
> > even if the transaction commit was aborted.
> 
> Right, because the delaylog and non-delaylog paths are not symmetric
> w.r.t. log write failures.
> 
> > It seems like the CIL code is missing an equivalent to
> > xfs_trans_uncommit for the case that xfs_log_write or xfs_log_done
> > fail.
> 
> There isn't an equivalent. In the delaylog case, we don't have a
> transaction to "uncommit" when a log write failure occurs - we are
> aborting the checkpoint of the CIL, not a transaction. As the items
> have already gone through IOP_COMMITTING and IOP_UNLOCK, we have to
> treat the failures like they came from the log IO completion
> handler.

True.

> In the case of non-delaylog, neither IOP_COMMITTING or IOP_UNLOCK
> has been called on the items when the xfs_log_write() fails. They
> are still linked into the xfs_trans structure, so they can be
> handled by xfs_trans_uncommit() which simply needs to walk the items
> in the transaction and  IOP_UNPIN(lip, abort), IOP_UNLOCK and free
> the items.

You're right.  Care to extend the comment to explain this a bit better
in the code?

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