Re: question about transaction-abort-related commits

[Josef Bacik <jbacik@fusionio.com>]

On Sun, Jun 23, 2013 at 09:52:14PM +0300, Alex Lyakas wrote:
> Hello Josef, Liu,
> I am reviewing commits in the mainline tree:
> 
> e4a2bcaca9643e7430207810653222fc5187f2be Btrfs: if we aren't
> committing just end the transaction if we error out
> (call end_transaction() instead of goto cleanup_transaction) - Josef
> 
> f094ac32aba3a51c00e970a2ea029339af2ca048 Btrfs: fix NULL pointer after
> aborting a transaction
> (wait until all writers detach, before setting running_transaction to
> NULL) - Liu
> 
> 66b6135b7cf741f6f44ba938b27583ea3b83bd12 Btrfs: avoid deadlock on
> transaction waiting list
> (check if transaction was already removed from the transactions list) - Liu
> 
> Josef, according to your fix, if the committer encounters a problem
> early, it just doesn't commit. Instead it aborts the transaction
> (possibly setting FS to read-only) and detaches from the transaction.
> So if this was the only committer (e.g., the transaction_kthread),
> then transaction commit will not happen at all. Is this what you
> intended? So then the user will notice that FS went read-only, and she
> will unmount the FS, and transaction will be cleaned up in
> btrfs_error_commit_super()=>btrfs_cleanup_transaction(), and not in
> cleanup_transaction() via btrfs_commit_transaction(). Is my
> understanding correct?
> 
> Liu, it looks like after having Josef's fix, the above two commits of
> yours are not strictly needed, right? Because Josef's fix ensures that
> only the "real" committer will call cleanup_transaction(), so at this
> point there is only one writer attached to the transaction, which is
> the committer itself (your fixes doesn't hurt though). Is that
> correct?
> 

I've looked at the patches and I'm going to say yes with the caveat that I
stopped thinking about it when my head started hurting :).  Thanks,

Josef