xfs
[Top] [All Lists]

Re: [PATCH 50/50] xfs: use reference counts to free clean buffer items

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 50/50] xfs: use reference counts to free clean buffer items
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Tue, 13 Aug 2013 17:00:25 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130813214648.GC6023@dastard>
References: <1376304611-22994-1-git-send-email-david@xxxxxxxxxxxxx> <1376304611-22994-51-git-send-email-david@xxxxxxxxxxxxx> <520A4AB7.1080207@xxxxxxx> <20130813214648.GC6023@dastard>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 08/13/13 16:46, Dave Chinner wrote:
On Tue, Aug 13, 2013 at 10:03:19AM -0500, Mark Tinguely wrote:
On 08/12/13 05:50, Dave Chinner wrote:
From: Dave Chinner<dchinner@xxxxxxxxxx>

When a transaction is cancelled and the buffer log item is clean in

...


why is a clean buffer on the AIL? Racing with a completion handler?

"clean" means that it wasn't dirtied in the transaction - it can be
in the AIL and holding a reference count that way.

I am wondering because it should not have made it into the CIL if it was not dirtied in a transaction - at least according to the the log item descriptor flag at least.


rather than this:

        if (clean || aborted) {
                if (atomic_dec_and_test(&bip->bli_refcount)) {
                        ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
                        xfs_buf_item_relse(bp);
                }
        } else
                atomic_dec(&bip->bli_refcount);

why not fold it into this:

        if (atomic_dec_and_test(&bip->bli_refcount)) {
                ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
                xfs_buf_item_relse(bp);
        }

Basically, the code as it stands documents the different exit
conditions of a transaction commit. If the item was logged, we
expect it to continue existing and something else will free it.  If
we change it, we lose awareness that there are different exit
criteria, and it risks introducing a use after free if there is ever
a race condition w.r.t. unpinning the item in an async CIL commit.

I'm not saying that there is a problem here, just that the code as
it stands will not free the item in this case. The stale buffer
handling has similar requirements (i.e. decrement without freeing so
the unpin on log IO completion will free it) and there's a comment in
xfs_log_commit_cil() related to avoiding such race conditions.

So I'd prefer to leave it as it stands...

okay with me.

--Mark.

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