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

[Mark Tinguely]

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 at redhat.com>
>>>
>>> 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.