btrfs: Add assert to catch nested transaction commit
diff mbox series

Message ID 20190912153144.26638-1-nborisov@suse.com
State New
Headers show
Series
  • btrfs: Add assert to catch nested transaction commit
Related show

Commit Message

Nikolay Borisov Sept. 12, 2019, 3:31 p.m. UTC
A recent patch to btrfs showed that there was at least 1 case where a
nesed transaction was committed. Nested transaction in this case  means
a code which has a transaction handle calls some function which in turn
obtains a copy of the same transaction handle. In such cases the correct
thing to do is for the lower callee to call btrfs_end_transaction which
contains appropriate checks so as to not commit the transaction which
will result in stale trans handler for the caller.

To catch such cases add an assert in btrfs_commit_transaction ensuring
btrfs_trans_handle::use_count is always 1.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

A full xfs test run didn't trigger this assert. 

 fs/btrfs/transaction.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Josef Bacik Sept. 13, 2019, 4:11 p.m. UTC | #1
On Thu, Sep 12, 2019 at 06:31:44PM +0300, Nikolay Borisov wrote:
> A recent patch to btrfs showed that there was at least 1 case where a
> nesed transaction was committed. Nested transaction in this case  means
> a code which has a transaction handle calls some function which in turn
> obtains a copy of the same transaction handle. In such cases the correct
> thing to do is for the lower callee to call btrfs_end_transaction which
> contains appropriate checks so as to not commit the transaction which
> will result in stale trans handler for the caller.
> 
> To catch such cases add an assert in btrfs_commit_transaction ensuring
> btrfs_trans_handle::use_count is always 1.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef
David Sterba Sept. 23, 2019, 3:24 p.m. UTC | #2
On Thu, Sep 12, 2019 at 06:31:44PM +0300, Nikolay Borisov wrote:
> A recent patch to btrfs showed that there was at least 1 case where a
> nesed transaction was committed. Nested transaction in this case  means
> a code which has a transaction handle calls some function which in turn
> obtains a copy of the same transaction handle. In such cases the correct
> thing to do is for the lower callee to call btrfs_end_transaction which
> contains appropriate checks so as to not commit the transaction which
> will result in stale trans handler for the caller.
> 
> To catch such cases add an assert in btrfs_commit_transaction ensuring
> btrfs_trans_handle::use_count is always 1.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Added to misc-next, thanks.

Patch
diff mbox series

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 8624bdee8c5b..8b75426c349e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1949,6 +1949,8 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	struct btrfs_transaction *prev_trans = NULL;
 	int ret;
 
+	ASSERT(refcount_read(&trans->use_count) == 1);
+
 	/* Stop the commit early if ->aborted is set */
 	if (unlikely(READ_ONCE(cur_trans->aborted))) {
 		ret = cur_trans->aborted;