Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()
     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
	   mutex_lock(fs_info->qgroup_ioctl_lock)
	    --> deadlock, mutex already locked
	        by this task at
		btrfs_quota_enable()

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

fs/btrfs/qgroup.c +1 -0

index d4917c0cddf5..3aec3bfa3d70 100644
@@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
 		trans = NULL;
 		goto out;
 	}
+	trans->enabling_quotas = true;
 
 	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
 	if (!fs_info->qgroup_ulist) {

fs/btrfs/qgroup.c +2 -1

 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
 		goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
 	spin_unlock(&fs_info->qgroup_lock);
 out:
-	mutex_unlock(&fs_info->qgroup_ioctl_lock);
+	if (!trans->enabling_quotas)
+		mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
 	bool reloc_reserved;
 	bool sync;
 	bool dirty;
+	bool enabling_quotas;
 	struct btrfs_root *root;
 	struct btrfs_fs_info *fs_info;
 	struct list_head new_bgs;

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()

     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
	   mutex_lock(fs_info->qgroup_ioctl_lock)
	    --> deadlock, mutex already locked
	        by this task at
		btrfs_quota_enable()

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

fs/btrfs/qgroup.c +0 -0

index d4917c0cddf5..3aec3bfa3d70 100644

fs/btrfs/qgroup.c +7 -2

 	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
 	if (!fs_info->qgroup_ulist) {

@@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 	u32 level_size = 0;
 	u64 nums;
 
-	mutex_lock(&fs_info->qgroup_ioctl_lock);
+	if (trans->enabling_quotas)
+		lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
+	else
+		mutex_lock(&fs_info->qgroup_ioctl_lock);
+
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
 		goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
 	spin_unlock(&fs_info->qgroup_lock);
 out:
-	mutex_unlock(&fs_info->qgroup_ioctl_lock);
+	if (!trans->enabling_quotas)
+		mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
 	bool reloc_reserved;
 	bool sync;
 	bool dirty;
+	bool enabling_quotas;
 	struct btrfs_root *root;
 	struct btrfs_fs_info *fs_info;
 	struct list_head new_bgs;

Quoted 24 lines · On 2018/11/19 下午5:48, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()

Quoted 8 lines

     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

The backtrace looks valid.

Quoted 14 lines

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 911

Quoted 9 lines

fs/btrfs/qgroup.c +1 -0

index d4917c0cddf5..3aec3bfa3d70 100644
@@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              trans = NULL;
              goto out;
      }
+     trans->enabling_quotas = true;

Should we put enabling_quotas bit into btrfs_transaction instead of btrfs_trans_handle?

Isn't it possible to have different trans handle pointed to the same transaction?

And I'm not really sure about the naming "enabling_quotas". What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)

Thanks,
Qu

Quoted 39 lines

fs/btrfs/qgroup.c +7 -2

      fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
      if (!fs_info->qgroup_ulist) {

@@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
      u32 level_size = 0;
      u64 nums;
 
-     mutex_lock(&fs_info->qgroup_ioctl_lock);
+     if (trans->enabling_quotas)
+             lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
+     else
+             mutex_lock(&fs_info->qgroup_ioctl_lock);
+
      if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
              goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
      spin_unlock(&fs_info->qgroup_lock);
 out:
-     mutex_unlock(&fs_info->qgroup_ioctl_lock);
+     if (!trans->enabling_quotas)
+             mutex_unlock(&fs_info->qgroup_ioctl_lock);
      return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
      bool reloc_reserved;
      bool sync;
      bool dirty;
+     bool enabling_quotas;
      struct btrfs_root *root;
      struct btrfs_fs_info *fs_info;
      struct list_head new_bgs;

Quoted 53 lines · Qu Wenruo

Quoted 24 lines · On 2018/11/19 下午5:48, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()

Quoted 8 lines

     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

The backtrace looks valid.

Quoted 14 lines

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

Quoted 12 lines

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 911

Quoted 9 lines

fs/btrfs/qgroup.c +1 -0

index d4917c0cddf5..3aec3bfa3d70 100644
@@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              trans = NULL;
              goto out;
      }
+     trans->enabling_quotas = true;

Should we put enabling_quotas bit into btrfs_transaction instead of btrfs_trans_handle?

Why?
Only the task which is enabling quotas needs to know about it.

Quoted 4 lines

Isn't it possible to have different trans handle pointed to the same transaction?

Yes.

Quoted 3 lines

And I'm not really sure about the naming "enabling_quotas". What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)

Too long.

Quoted 46 lines

Thanks,
Qu

Quoted 39 lines

fs/btrfs/qgroup.c +7 -2

      fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
      if (!fs_info->qgroup_ulist) {

@@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
      u32 level_size = 0;
      u64 nums;
 
-     mutex_lock(&fs_info->qgroup_ioctl_lock);
+     if (trans->enabling_quotas)
+             lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
+     else
+             mutex_lock(&fs_info->qgroup_ioctl_lock);
+
      if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
              goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
      spin_unlock(&fs_info->qgroup_lock);
 out:
-     mutex_unlock(&fs_info->qgroup_ioctl_lock);
+     if (!trans->enabling_quotas)
+             mutex_unlock(&fs_info->qgroup_ioctl_lock);
      return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
      bool reloc_reserved;
      bool sync;
      bool dirty;
+     bool enabling_quotas;
      struct btrfs_root *root;
      struct btrfs_fs_info *fs_info;
      struct list_head new_bgs;

Quoted 67 lines · Filipe Manana

Quoted 65 lines · Qu Wenruo

Quoted 24 lines · On 2018/11/19 下午5:48, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()

Quoted 8 lines

     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

The backtrace looks valid.

Quoted 19 lines

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

fs/btrfs/qgroup.c +0 -0

index d4917c0cddf5..3aec3bfa3d70 100644

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 911

Quoted 4 lines

fs/btrfs/qgroup.c +1 -0

@@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              trans = NULL;
              goto out;
      }
+     trans->enabling_quotas = true;

Should we put enabling_quotas bit into btrfs_transaction instead of btrfs_trans_handle?

Quoted 2 lines

Why?
Only the task which is enabling quotas needs to know about it.

But it's the btrfs_qgroup_inherit() using the trans handler to avoid dead lock.

What makes sure btrfs_qgroup_inherit() get the exactly same trans handler allocated here?

Quoted 9 lines

Quoted 5 lines

Isn't it possible to have different trans handle pointed to the same transaction?

Yes.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

And I'm not really sure about the naming "enabling_quotas". What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)

Too long.

Anyway, current naming doesn't really show why we could skip mutex locking. Just hope to get some name better.

Thanks,
Qu

Quoted 46 lines

Quoted 46 lines

Thanks,
Qu

Quoted 39 lines

fs/btrfs/qgroup.c +7 -2

      fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
      if (!fs_info->qgroup_ulist) {

@@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
      u32 level_size = 0;
      u64 nums;
 
-     mutex_lock(&fs_info->qgroup_ioctl_lock);
+     if (trans->enabling_quotas)
+             lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
+     else
+             mutex_lock(&fs_info->qgroup_ioctl_lock);
+
      if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
              goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
      spin_unlock(&fs_info->qgroup_lock);
 out:
-     mutex_unlock(&fs_info->qgroup_ioctl_lock);
+     if (!trans->enabling_quotas)
+             mutex_unlock(&fs_info->qgroup_ioctl_lock);
      return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
      bool reloc_reserved;
      bool sync;
      bool dirty;
+     bool enabling_quotas;
      struct btrfs_root *root;
      struct btrfs_fs_info *fs_info;
      struct list_head new_bgs;

Quoted 71 lines · Qu Wenruo

Quoted 67 lines · Filipe Manana

Quoted 65 lines · Qu Wenruo

Quoted 24 lines · On 2018/11/19 下午5:48, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()

Quoted 8 lines

     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

The backtrace looks valid.

Quoted 19 lines

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

fs/btrfs/qgroup.c +0 -0

index d4917c0cddf5..3aec3bfa3d70 100644

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 911

Quoted 4 lines

fs/btrfs/qgroup.c +1 -0

@@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              trans = NULL;
              goto out;
      }
+     trans->enabling_quotas = true;

Should we put enabling_quotas bit into btrfs_transaction instead of btrfs_trans_handle?

Quoted 8 lines

Quoted 2 lines

Why?
Only the task which is enabling quotas needs to know about it.

But it's the btrfs_qgroup_inherit() using the trans handler to avoid dead lock.

What makes sure btrfs_qgroup_inherit() get the exactly same trans handler allocated here?

If it's the other task (the one creating a snapshot) that starts the transaction commit,
it will have to wait for the task enabling quotas to release the transaction - once that task
also calls commit_transaction(), it will skip doing the commit itself and wait for the snapshot
one to finish the commit, while holding the qgroup mutex (this part I missed before).
So yes we'll have to use a bit in the transaction itself instead.

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

Isn't it possible to have different trans handle pointed to the same transaction?

Yes.

Quoted 3 lines

And I'm not really sure about the naming "enabling_quotas". What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)

Quoted 4 lines

Quoted 1 line

Too long.

Anyway, current naming doesn't really show why we could skip mutex locking. Just hope to get some name better.

No name will ever show you that.
You'll always have to see where and how it's used, unless you want a name like "dont_lock_mutex_because_we_locked_it_at_btrfs...".

Quoted 52 lines

Thanks,
Qu

Quoted 46 lines

Quoted 46 lines

Thanks,
Qu

Quoted 39 lines

fs/btrfs/qgroup.c +7 -2

      fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
      if (!fs_info->qgroup_ulist) {

@@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
      u32 level_size = 0;
      u64 nums;
 
-     mutex_lock(&fs_info->qgroup_ioctl_lock);
+     if (trans->enabling_quotas)
+             lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
+     else
+             mutex_lock(&fs_info->qgroup_ioctl_lock);
+
      if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
              goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
      spin_unlock(&fs_info->qgroup_lock);
 out:
-     mutex_unlock(&fs_info->qgroup_ioctl_lock);
+     if (!trans->enabling_quotas)
+             mutex_unlock(&fs_info->qgroup_ioctl_lock);
      return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
      bool reloc_reserved;
      bool sync;
      bool dirty;
+     bool enabling_quotas;
      struct btrfs_root *root;
      struct btrfs_fs_info *fs_info;
      struct list_head new_bgs;

Quoted 71 lines · Qu Wenruo

Quoted 67 lines · Filipe Manana

Quoted 65 lines · Qu Wenruo

Quoted 24 lines · On 2018/11/19 下午5:48, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice. The following time diagram shows how this happens.

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()

Quoted 8 lines

     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

The backtrace looks valid.

Quoted 19 lines

So fix this by adding a flag to the transaction handle that signals if the transaction is being used for enabling quotas (only seen by the task doing it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit() if the transaction handle corresponds to the one being used to enable the quotas.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/qgroup.c      | 10 ++++++++--
 fs/btrfs/transaction.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

fs/btrfs/qgroup.c +0 -0

index d4917c0cddf5..3aec3bfa3d70 100644

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 911

Quoted 4 lines

fs/btrfs/qgroup.c +1 -0

@@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              trans = NULL;
              goto out;
      }
+     trans->enabling_quotas = true;

Should we put enabling_quotas bit into btrfs_transaction instead of btrfs_trans_handle?

Quoted 8 lines

Quoted 2 lines

Why?
Only the task which is enabling quotas needs to know about it.

But it's the btrfs_qgroup_inherit() using the trans handler to avoid dead lock.

What makes sure btrfs_qgroup_inherit() get the exactly same trans handler allocated here?

If it's the other task (the one creating a snapshot) that starts the transaction commit,
it will have to wait for the task enabling quotas to release the transaction - once that task
also calls commit_transaction(), it will skip doing the commit itself and wait for the snapshot
one to finish the commit, while holding the qgroup mutex (this part I missed before).
So yes we'll have to use a bit in the transaction itself instead.

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Isn't it possible to have different trans handle pointed to the same transaction?

Yes.

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

And I'm not really sure about the naming "enabling_quotas". What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)

Too long.

Anyway, current naming doesn't really show why we could skip mutex locking. Just hope to get some name better.

No name will ever show you that.
You'll always have to see where and how it's used, unless you want a name like "dont_lock_mutex_because_we_locked_it_at_btrfs...".

Quoted 51 lines

Thanks,
Qu

Quoted 46 lines

Quoted 46 lines

Thanks,
Qu

Quoted 39 lines

fs/btrfs/qgroup.c +7 -2

      fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
      if (!fs_info->qgroup_ulist) {

@@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
      u32 level_size = 0;
      u64 nums;
 
-     mutex_lock(&fs_info->qgroup_ioctl_lock);
+     if (trans->enabling_quotas)
+             lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
+     else
+             mutex_lock(&fs_info->qgroup_ioctl_lock);
+
      if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
              goto out;
 

@@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 unlock:
      spin_unlock(&fs_info->qgroup_lock);
 out:
-     mutex_unlock(&fs_info->qgroup_ioctl_lock);
+     if (!trans->enabling_quotas)
+             mutex_unlock(&fs_info->qgroup_ioctl_lock);
      return ret;
 }
 

fs/btrfs/transaction.h +1 -0

index 703d5116a2fc..a5553a1dee30 100644
@@ -122,6 +122,7 @@ struct btrfs_trans_handle {
      bool reloc_reserved;
      bool sync;
      bool dirty;
+     bool enabling_quotas;
      struct btrfs_root *root;
      struct btrfs_fs_info *fs_info;
      struct list_head new_bgs;

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice, or the task creating a snapshot tries to commit the transaction while the task enabling quota waits for the former task to commit the transaction while holding the mutex. The following time diagrams show how both cases happen.

First scenario:

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()
     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
	   mutex_lock(fs_info->qgroup_ioctl_lock)
	    --> deadlock, mutex already locked
	        by this task at
		btrfs_quota_enable()

Second scenario:

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()
mutex_lock(fs_info->qgroup_ioctl_lock)
btrfs_start_transaction()

btrfs_ioctl()
btrfs_ioctl_snap_create_v2
create_snapshot()
--> adds snapshot to the
list pending_snapshots of the current transaction

btrfs_commit_transaction()
--> waits for task at
CPU 0 to release its transaction handle

btrfs_commit_transaction()
--> sees another task started
the transaction commit first
--> releases its transaction
handle
--> waits for the transaction
commit to be completed by
the task at CPU 1

create_pending_snapshot()
qgroup_account_snapshot()
btrfs_qgroup_inherit()
mutex_lock(fs_info->qgroup_ioctl_lock)
--> deadlock, task at CPU 0
has the mutex locked but it is waiting for us to finish the transaction commit

So fix this by setting the quota enabled flag in fs_info after committing the transaction at btrfs_quota_enable(). This ends up serializing quota enable and snapshot creation as if the snapshot creation happened just before the quota enable request. The quota rescan task, scheduled after committing the transaction in btrfs_quote_enable(), will do the accounting.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---

V2: Added second deadlock example to changelog and changed the fix to deal
with that case as well.

 fs/btrfs/qgroup.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

fs/btrfs/qgroup.c +0 -0

index d4917c0cddf5..ae1358253b7b 100644
@@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
 		btrfs_abort_transaction(trans, ret);
 		goto out_free_path;

fs/btrfs/qgroup.c +10 -0

+	/*
+	 * Set quota enabled flag after committing the transaction, to avoid
+	 * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
+	 * creation.
+	 */
+	spin_lock(&fs_info->qgroup_lock);
+	fs_info->quota_root = quota_root;
+	set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
+	spin_unlock(&fs_info->qgroup_lock);
+
 	ret = qgroup_rescan_init(fs_info, 0, 1);
 	if (!ret) {
 	        qgroup_rescan_zero_tracking(fs_info);
 

Quoted 110 lines · On 2018/11/19 下午10:15, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice, or the task creating a snapshot tries to commit the transaction while the task enabling quota waits for the former task to commit the transaction while holding the mutex. The following time diagrams show how both cases happen.

First scenario:

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()
     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

Second scenario:

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()
mutex_lock(fs_info->qgroup_ioctl_lock)
btrfs_start_transaction()

btrfs_ioctl()
btrfs_ioctl_snap_create_v2
create_snapshot()
--> adds snapshot to the
list pending_snapshots of the current transaction

btrfs_commit_transaction()
--> waits for task at
CPU 0 to release its transaction handle

btrfs_commit_transaction()
--> sees another task started
the transaction commit first
--> releases its transaction
handle
--> waits for the transaction
commit to be completed by
the task at CPU 1

create_pending_snapshot()
qgroup_account_snapshot()
btrfs_qgroup_inherit()
mutex_lock(fs_info->qgroup_ioctl_lock)
--> deadlock, task at CPU 0
has the mutex locked but it is waiting for us to finish the transaction commit

So fix this by setting the quota enabled flag in fs_info after committing the transaction at btrfs_quota_enable(). This ends up serializing quota enable and snapshot creation as if the snapshot creation happened just before the quota enable request. The quota rescan task, scheduled after committing the transaction in btrfs_quote_enable(), will do the accounting.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---

V2: Added second deadlock example to changelog and changed the fix to deal
with that case as well.

 fs/btrfs/qgroup.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

fs/btrfs/qgroup.c +0 -4

index d4917c0cddf5..ae1358253b7b 100644
@@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              btrfs_abort_transaction(trans, ret);
              goto out_free_path;
      }
-     spin_lock(&fs_info->qgroup_lock);
-     fs_info->quota_root = quota_root;
-     set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
-     spin_unlock(&fs_info->qgroup_lock);
 

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 1020

Quoted 4 lines

fs/btrfs/qgroup.c +0 -0

      ret = btrfs_commit_transaction(trans);
      trans = NULL;
      if (ret)
              goto out_free_path;

The main concern here is, if we don't set qgroup enabled bit before we commit transaction, there will be a window where new tree modification could happen before QGROUP_ENABLED bit set.

There may be some qgroup reserved space related problem in such case, but I'm not 100% sure to foresee such problem.

The best way to do this is, commit trans first, and before any other one trying to start transaction, we set the bit.
However I can't find such infrastructure now (I still remember we used to have a pending bit to change quota enabled flag, but removed later)

Thanks,
Qu

Quoted 15 lines

fs/btrfs/qgroup.c +10 -0

+     /*
+      * Set quota enabled flag after committing the transaction, to avoid
+      * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
+      * creation.
+      */
+     spin_lock(&fs_info->qgroup_lock);
+     fs_info->quota_root = quota_root;
+     set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
+     spin_unlock(&fs_info->qgroup_lock);
+
      ret = qgroup_rescan_init(fs_info, 0, 1);
      if (!ret) {
              qgroup_rescan_zero_tracking(fs_info);
 

Quoted 114 lines · Qu Wenruo

Quoted 110 lines · On 2018/11/19 下午10:15, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the quota enable and snapshot creation ioctls are called concurrently we can get into a deadlock where the task enabling quotas will deadlock on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it twice, or the task creating a snapshot tries to commit the transaction while the task enabling quota waits for the former task to commit the transaction while holding the mutex. The following time diagrams show how both cases happen.

First scenario:

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()

    mutex_lock(fs_info->qgroup_ioctl_lock)
    btrfs_start_transaction()

                                             btrfs_ioctl()
                                              btrfs_ioctl_snap_create_v2
                                               create_snapshot()
                                                --> adds snapshot to the
                                                    list pending_snapshots
                                                    of the current
                                                    transaction

    btrfs_commit_transaction()
     create_pending_snapshots()
       create_pending_snapshot()
        qgroup_account_snapshot()
         btrfs_qgroup_inherit()
         mutex_lock(fs_info->qgroup_ioctl_lock)
          --> deadlock, mutex already locked
              by this task at
              btrfs_quota_enable()

Second scenario:

CPU 0 CPU 1

btrfs_ioctl()
btrfs_ioctl_quota_ctl()
btrfs_quota_enable()
mutex_lock(fs_info->qgroup_ioctl_lock)
btrfs_start_transaction()

btrfs_ioctl()
btrfs_ioctl_snap_create_v2
create_snapshot()
--> adds snapshot to the
list pending_snapshots of the current transaction

btrfs_commit_transaction()
--> waits for task at
CPU 0 to release its transaction handle

btrfs_commit_transaction()
--> sees another task started
the transaction commit first
--> releases its transaction
handle
--> waits for the transaction
commit to be completed by
the task at CPU 1

create_pending_snapshot()
qgroup_account_snapshot()
btrfs_qgroup_inherit()
mutex_lock(fs_info->qgroup_ioctl_lock)
--> deadlock, task at CPU 0
has the mutex locked but it is waiting for us to finish the transaction commit

So fix this by setting the quota enabled flag in fs_info after committing the transaction at btrfs_quota_enable(). This ends up serializing quota enable and snapshot creation as if the snapshot creation happened just before the quota enable request. The quota rescan task, scheduled after committing the transaction in btrfs_quote_enable(), will do the accounting.

Fixes 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")

Signed-off-by Filipe Manana <[email protected]>

---

V2: Added second deadlock example to changelog and changed the fix to deal
with that case as well.

 fs/btrfs/qgroup.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

fs/btrfs/qgroup.c +0 -4

index d4917c0cddf5..ae1358253b7b 100644
@@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              btrfs_abort_transaction(trans, ret);
              goto out_free_path;
      }
-     spin_lock(&fs_info->qgroup_lock);
-     fs_info->quota_root = quota_root;
-     set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
-     spin_unlock(&fs_info->qgroup_lock);
 

Quoted 8 lines

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 1020

Quoted 4 lines

fs/btrfs/qgroup.c +0 -0

      ret = btrfs_commit_transaction(trans);
      trans = NULL;
      if (ret)
              goto out_free_path;

The main concern here is, if we don't set qgroup enabled bit before we commit transaction, there will be a window where new tree modification could happen before QGROUP_ENABLED bit set.

That doesn't seem to make much sense to me, if I understood correctly. Essentially you're saying stuff done to any tree in the the transaction we use to
enable quotas must be accounted for. In that case the quota enabled bit should be done as soon as the transaction is started, because before we set it and after
we started (or joined) a transaction, a lot could of modifications may have happened.
Nevertheless I don't think it's unexpected for anyone to have the accounting happening
only after the quota enable ioctl returns success.

Quoted 30 lines

There may be some qgroup reserved space related problem in such case, but I'm not 100% sure to foresee such problem.

The best way to do this is, commit trans first, and before any other one trying to start transaction, we set the bit.
However I can't find such infrastructure now (I still remember we used to have a pending bit to change quota enabled flag, but removed later)

Thanks,
Qu

Quoted 15 lines

fs/btrfs/qgroup.c +10 -0

+     /*
+      * Set quota enabled flag after committing the transaction, to avoid
+      * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
+      * creation.
+      */
+     spin_lock(&fs_info->qgroup_lock);
+     fs_info->quota_root = quota_root;
+     set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
+     spin_unlock(&fs_info->qgroup_lock);
+
      ret = qgroup_rescan_init(fs_info, 0, 1);
      if (!ret) {
              qgroup_rescan_zero_tracking(fs_info);
 

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

fs/btrfs/qgroup.c +0 -4

@@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              btrfs_abort_transaction(trans, ret);
              goto out_free_path;
      }
-     spin_lock(&fs_info->qgroup_lock);
-     fs_info->quota_root = quota_root;
-     set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
-     spin_unlock(&fs_info->qgroup_lock);
 

Quoted 8 lines

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 1020

Quoted 4 lines

fs/btrfs/qgroup.c +0 -0

      ret = btrfs_commit_transaction(trans);
      trans = NULL;
      if (ret)
              goto out_free_path;

The main concern here is, if we don't set qgroup enabled bit before we commit transaction, there will be a window where new tree modification could happen before QGROUP_ENABLED bit set.

Quoted 11 lines

That doesn't seem to make much sense to me, if I understood correctly. Essentially you're saying stuff done to any tree in the the transaction we use to
enable quotas must be accounted for. In that case the quota enabled bit should be done as soon as the transaction is started, because before we set it and after
we started (or joined) a transaction, a lot could of modifications may have happened.
Nevertheless I don't think it's unexpected for anyone to have the accounting happening
only after the quota enable ioctl returns success.

The problem is not accounting, the qgroup number won't cause problem.

It's the reserved space. Like some dirty pages are dirtied before quota enabled, but at run_dealloc() time quota is enabled.

For such case we have io_tree based method to avoid underflow so it should be OK.

So v2 patch looks OK.

Quoted 38 lines · Qu Wenruo

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

fs/btrfs/qgroup.c +0 -4

@@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
              btrfs_abort_transaction(trans, ret);
              goto out_free_path;
      }
-     spin_lock(&fs_info->qgroup_lock);
-     fs_info->quota_root = quota_root;
-     set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
-     spin_unlock(&fs_info->qgroup_lock);
 

Quoted 8 lines

fs/btrfs/qgroup.c int btrfs_quota_enable(struct btrfs_fs_info *fs_info) · line 1020

Quoted 4 lines

fs/btrfs/qgroup.c +0 -0

      ret = btrfs_commit_transaction(trans);
      trans = NULL;
      if (ret)
              goto out_free_path;

The main concern here is, if we don't set qgroup enabled bit before we commit transaction, there will be a window where new tree modification could happen before QGROUP_ENABLED bit set.

Quoted 11 lines

That doesn't seem to make much sense to me, if I understood correctly. Essentially you're saying stuff done to any tree in the the transaction we use to
enable quotas must be accounted for. In that case the quota enabled bit should be done as soon as the transaction is started, because before we set it and after
we started (or joined) a transaction, a lot could of modifications may have happened.
Nevertheless I don't think it's unexpected for anyone to have the accounting happening
only after the quota enable ioctl returns success.

The problem is not accounting, the qgroup number won't cause problem.

It's the reserved space. Like some dirty pages are dirtied before quota enabled, but at run_dealloc() time quota is enabled.

For such case we have io_tree based method to avoid underflow so it should be OK.

Quoted 1 line

So v2 patch looks OK.

Does that mean reviewed-by? In case there's a evolved discussion under a patch, a clear yes/no is appreciated and an explicit Reviewed-by even more. I'm about to add this patch to rc4 pull, thre's still some time to add the tag. Thanks.

Quoted 15 lines

Quoted 15 lines

Quoted 6 lines

it and after
we started (or joined) a transaction, a lot could of modifications may have happened.
Nevertheless I don't think it's unexpected for anyone to have the accounting happening
only after the quota enable ioctl returns success.

The problem is not accounting, the qgroup number won't cause problem.

It's the reserved space. Like some dirty pages are dirtied before quota enabled, but at run_dealloc() time quota is enabled.

For such case we have io_tree based method to avoid underflow so it should be OK.

Quoted 6 lines

Quoted 1 line

So v2 patch looks OK.

Does that mean reviewed-by? In case there's a evolved discussion under a patch, a clear yes/no is appreciated and an explicit Reviewed-by even more. I'm about to add this patch to rc4 pull, thre's still some time to add the tag. Thanks.

I'd like to add reviewed-by tab, but I'm still not 100% if this will cause extra qgroup reserved space related problem.

At least from my side, I can't directly see a case where it will cause problem.

Does such case mean a reviewed-by tag? Or something LGTM-but-uncertain?

There may be some qgroup reserved space related problem in such case, but I'm not 100% sure to foresee such problem.

The best way to do this is, commit trans first, and before any other one trying to start transaction, we set the bit.
However I can't find such infrastructure now (I still remember we used to have a pending bit to change quota enabled flag, but removed later)

Quoted 2 lines · Qu Wenruo

There may be some qgroup reserved space related problem in such case, but I'm not 100% sure to foresee such problem.

But why is this a problem - we always queue quota rescan following QUOTA enable, that should take care of proper accounting, no?

Quoted 7 lines

The best way to do this is, commit trans first, and before any other one trying to start transaction, we set the bit.
However I can't find such infrastructure now (I still remember we used to have a pending bit to change quota enabled flag, but removed later)