Fix generic/390 failure due to quota release after freeze

One of the paths quota writeback is called from is:

freeze_super()
sync_filesystem()

    ext4_sync_fs()
      dquot_writeback_dquots()

Since we currently don't always flush the quota_release_work queue in this path, we can end up with the following race:

1. dquot are added to releasing_dquots list during regular operations.
2. FS Freeze starts, however, this does not flush the quota_release_work queue.
3. Freeze completes.
4. Kernel eventually tries to flush the workqueue while FS is frozen which hits a WARN_ON since transaction gets started during frozen state:

  ext4_journal_check_start+0x28/0x110 [ext4] (unreliable)
  __ext4_journal_start_sb+0x64/0x1c0 [ext4]
  ext4_release_dquot+0x90/0x1d0 [ext4]
  quota_release_workfn+0x43c/0x4d0

Which is the following line:

  WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE);

Which ultimately results in generic/390 failing due to dmesg noise. This was detected on powerpc machine 15 cores.

To avoid this, make sure to flush the workqueue during dquot_writeback_dquots() so we dont have any pending workitems after freeze.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

One of the paths quota writeback is called from is:

freeze_super()
sync_filesystem()

     ext4_sync_fs()
       dquot_writeback_dquots()

Since we currently don't always flush the quota_release_work queue in this path, we can end up with the following race:

1. dquot are added to releasing_dquots list during regular operations.
2. FS Freeze starts, however, this does not flush the quota_release_work queue.
3. Freeze completes.
4. Kernel eventually tries to flush the workqueue while FS is frozen which hits a WARN_ON since transaction gets started during frozen state:

   ext4_journal_check_start+0x28/0x110 [ext4] (unreliable)
   __ext4_journal_start_sb+0x64/0x1c0 [ext4]
   ext4_release_dquot+0x90/0x1d0 [ext4]
   quota_release_workfn+0x43c/0x4d0

Which is the following line:

   WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE);

Which ultimately results in generic/390 failing due to dmesg noise. This was detected on powerpc machine 15 cores.

To avoid this, make sure to flush the workqueue during dquot_writeback_dquots() so we dont have any pending workitems after freeze.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  fs/quota/dquot.c | 2 ++
  1 file changed, 2 insertions(+)

fs/quota/dquot.c +2 -0

index 3dd8d6f27725..f9578918cfb2 100644
@@ -688,6 +688,8 @@ int dquot_writeback_dquots(struct super_block *sb, int type)
 
  	WARN_ON_ONCE(!rwsem_is_locked(&sb->s_umount));
 
+	flush_delayed_work(&quota_release_work);
+
  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
  		if (type != -1 && cnt != type)
  			continue;

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/super.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

fs/ext4/super.c +17 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
 {
 	int ret, err;
 	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
 
 	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
 				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
 	if (IS_ERR(handle)) {
 		/* Release dquot anyway to avoid endless cycle in dqput() */
 		dquot_release(dquot);
+		if (freeze_protected)
+			sb_end_intwrite(dquot->dq_sb);
 		return PTR_ERR(handle);
 	}
 	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
 	err = ext4_journal_stop(handle);
 	if (!ret)
 		ret = err;
+
+	if (freeze_protected)
+		sb_end_intwrite(dquot->dq_sb);
+
 	return ret;
 }
 

Signature · 1 line

2.43.5

Quoted 4 lines · Ojaswin Mujoo

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Quoted 8 lines

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Looks good to me (the 0-day reports seem to be due to wrong merge). Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 49 lines

---
 fs/ext4/super.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

fs/ext4/super.c +17 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
 {
 	int ret, err;
 	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
 
 	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
 				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
 	if (IS_ERR(handle)) {
 		/* Release dquot anyway to avoid endless cycle in dqput() */
 		dquot_release(dquot);
+		if (freeze_protected)
+			sb_end_intwrite(dquot->dq_sb);
 		return PTR_ERR(handle);
 	}
 	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
 	err = ext4_journal_stop(handle);
 	if (!ret)
 		ret = err;
+
+	if (freeze_protected)
+		sb_end_intwrite(dquot->dq_sb);
+
 	return ret;
 }
 

Signature · 1 line

2.43.5

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 41 lines · Ojaswin Mujoo

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  fs/ext4/super.c | 17 +++++++++++++++++
  1 file changed, 17 insertions(+)

fs/ext4/super.c +12 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
  {
  	int ret, err;
  	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
  	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
  				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
  	if (IS_ERR(handle)) {
  		/* Release dquot anyway to avoid endless cycle in dqput() */
  		dquot_release(dquot);
+		if (freeze_protected)

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot)

Quoted 5 lines

fs/ext4/super.c +1 -0

+			sb_end_intwrite(dquot->dq_sb);
  		return PTR_ERR(handle);
  	}
  	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)

The git am command looks for the following context code from line 6903 to apply the changes. But there are many functions in fs/ext4/super.c that have similar code, such as ext4_write_dquot() and ext4_acquire_dquot().

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot)

Quoted 1 line

fs/ext4/super.c +0 -0

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)

So when the code before ext4_release_dquot() is added, the first matching context found could be in ext4_write_dquot() or ext4_acquire_dquot().

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot) · line 6924

Quoted 9 lines

fs/ext4/super.c +4 -0

  	err = ext4_journal_stop(handle);
  	if (!ret)
  		ret = err;
+
+	if (freeze_protected)
+		sb_end_intwrite(dquot->dq_sb);
+
  	return ret;
  }

Thus this is actually a bug in git am, which can be avoided by increasing the number of context lines with git format-patch -U8 -1.

Otherwise it looks good. Feel free to add:

Reviewed-by Baokun Li <[email protected]>

Quoted 42 lines · Baokun Li

Quoted 41 lines · Ojaswin Mujoo

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
   fs/ext4/super.c | 17 +++++++++++++++++
   1 file changed, 17 insertions(+)

fs/ext4/super.c +12 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
   {
   	int ret, err;
   	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
   	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
   				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
   	if (IS_ERR(handle)) {
   		/* Release dquot anyway to avoid endless cycle in dqput() */
   		dquot_release(dquot);
+		if (freeze_protected)

Quoted 8 lines

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot)

Quoted 5 lines

fs/ext4/super.c +1 -0

+			sb_end_intwrite(dquot->dq_sb);
   		return PTR_ERR(handle);
   	}
   	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)

The git am command looks for the following context code from line 6903 to apply the changes. But there are many functions in fs/ext4/super.c that have similar code, such as ext4_write_dquot() and ext4_acquire_dquot().

Oh that's strange, shouldn't it match the complete line like:

Quoted 1 line

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot)

Quoted 1 line

fs/ext4/super.c +0 -0

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)

That should only have one occurence around line 6903? Or does it try to fuzzy match which ends up matching ext4_write_dquot etc?

Quoted 51 lines · Ojaswin Mujoo

Quoted 42 lines · Baokun Li

Quoted 41 lines · Ojaswin Mujoo

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
   fs/ext4/super.c | 17 +++++++++++++++++
   1 file changed, 17 insertions(+)

fs/ext4/super.c +12 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
   {
   	int ret, err;
   	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
   	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
   				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
   	if (IS_ERR(handle)) {
   		/* Release dquot anyway to avoid endless cycle in dqput() */
   		dquot_release(dquot);
+		if (freeze_protected)

Quoted 8 lines

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot)

Quoted 5 lines

fs/ext4/super.c +1 -0

+			sb_end_intwrite(dquot->dq_sb);
   		return PTR_ERR(handle);
   	}
   	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)

The git am command looks for the following context code from line 6903 to apply the changes. But there are many functions in fs/ext4/super.c that have similar code, such as ext4_write_dquot() and ext4_acquire_dquot().

Quoted 1 line

Oh that's strange, shouldn't it match the complete line like:

A rough look at the git am source code looks like it only focuses on line numbers between two ‘@@’.

am_run
 run_apply
  apply_all_patches
   apply_patch
    parse_chunk
     find_header
      parse_fragment_header
    check_patch_list
     check_patch
      apply_data
       load_preimage
       apply_fragments
        apply_one_fragment
         find_pos
          match_fragment

Quoted 3 lines

Quoted 1 line

fs/ext4/super.c static int ext4_release_dquot(struct dquot *dquot)

Quoted 1 line

fs/ext4/super.c +0 -0

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)

That should only have one occurence around line 6903? Or does it try to fuzzy match which ends up matching ext4_write_dquot etc?

In find_pos(), start from line 6903, compare the hash value of each line of code line by line in forward direction, if it can't match, then match the hash value of each line of code line by line in reverse direction from line 6903. Fuzzy matching is used if some whitespace characters should be ignored.

Regards,
Baokun

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/super.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

fs/ext4/super.c +17 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
 {
 	int ret, err;
 	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
 
 	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
 				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
 	if (IS_ERR(handle)) {
 		/* Release dquot anyway to avoid endless cycle in dqput() */
 		dquot_release(dquot);
+		if (freeze_protected)
+			sb_end_intwrite(dquot->dq_sb);
 		return PTR_ERR(handle);
 	}
 	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
 	err = ext4_journal_stop(handle);
 	if (!ret)
 		ret = err;
+
+	if (freeze_protected)
+		sb_end_intwrite(dquot->dq_sb);
+
 	return ret;
 }
 

Signature · 1 line

2.43.5

Quoted 4 lines · Ojaswin Mujoo

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Quoted 8 lines

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Sorry for being late on this. Ideally, shouldn't it be the responsibility of higher level FS (ext4) to make sure that FS never freezes while there is pending work for releasing dquot structures and that it should also prevent any context where such dquot structures gets added for release/delayed release.

e.g. this is what FS takes care during freeze path i.e.
freeze_super() -> sync_fs -> ext4_sync_fs()-> dquot_writeback_dquots() -> flush_delayed_work() (now fixed)

Now coming to iput() case which Jan mentioned [1] which could still be called after FS have frozen. As I see we have a protection from FS freeze in the ext4_evict_path() right? So ideally we should never see

dquot_drop() w/o fs freeze protection. And say, if the FS freezing immediately happened after we scheduled this delayed work (but before the work gets scheduled), then that will be taken care in the freeze_super() chain, where we will flush all the delayed work no? - which is what Patch-1 is fixing.

(There still might be an error handling path in ext4_evict_inode() -> ext4_clear_inode() which we don't freeze protect. I still need to take a closer look at that though).

So.. isn't this patch trying to hide the problem where FS failed to freeze protect some code path?

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/super.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

fs/ext4/super.c +17 -0

index 16a4ce704460..f7437a592359 100644
@@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
 {
 	int ret, err;
 	handle_t *handle;
+	bool freeze_protected = false;
+
+	/*
+	 * Trying to sb_start_intwrite() in a running transaction
+	 * can result in a deadlock. Further, running transactions
+	 * are already protected from freezing.
+	 */
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(dquot->dq_sb);
+		freeze_protected = true;
+	}
 
 	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
 				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
 	if (IS_ERR(handle)) {
 		/* Release dquot anyway to avoid endless cycle in dqput() */
 		dquot_release(dquot);
+		if (freeze_protected)
+			sb_end_intwrite(dquot->dq_sb);
 		return PTR_ERR(handle);
 	}
 	ret = dquot_release(dquot);

@@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
 	err = ext4_journal_stop(handle);
 	if (!ret)
 		ret = err;
+
+	if (freeze_protected)
+		sb_end_intwrite(dquot->dq_sb);
+
 	return ret;
 }
 

Signature · 1 line

2.43.5

Quoted 10 lines · Ojaswin Mujoo

Protect ext4_release_dquot against freezing so that we don't try to start a transaction when FS is frozen, leading to warnings.

Further, avoid taking the freeze protection if a transaction is already running so that we don't need end up in a deadlock as described in

46e294efc355 ext4: fix deadlock with fs freezing and EA inodes

Quoted 2 lines

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Hey Ted,

Just a ping, I think you might have missed this patch. Let me know if anything else is needed from my side.

Changes since v1:

* Patch 1: Move flush_delayed_work() to start of function
* Patch 2: Guard ext4_release_dquot against freeze

Regarding patch 2, as per my understanding of the journalling code, right now ext4_release_dquot() can only be called from the quota_realease_work workqueue and hence ideally should never have a journal open but to future-proof it we make sure the journal is not opened when calling sb_start_inwrite().

** Original Cover **

Recently we noticed generic/390 failing on powerpc systems. This test basically does a freeze-unfreeze loop in parallel with fsstress on the FS to detect any races in the code paths.

We noticed that the test started failing due to kernel WARN_ONs because quota_release_work workqueue started executing while the FS was frozen which led to creating new transactions in ext4_release_quota.

Most of the details are in the bug however I'd just like to add that I'm completely new to quota code so the patch, although fixing the issue, might be not be logically the right thing to do. So reviews and suggestions are welcome.

Also, I can only replicate this race on one of my machines reliably and does not appear on others. I've tested with with fstests -g quota and don't see any new failures.

Ojaswin Mujoo (2):
quota: flush quota_release_work upon quota writeback
ext4: protect ext4_release_dquot against freezing

 fs/ext4/super.c  | 17 +++++++++++++++++
 fs/quota/dquot.c |  2 ++
 2 files changed, 19 insertions(+)

Signature · 1 line

2.43.5