[PATCH 0/2] GFS2: Fix wrong call of new trans for jdata

[PATCH 0/2] GFS2: Fix wrong call of new trans for jdata

[Su Yue]

Patch 1 is trivial.
Please see commit message of patch 2/2.

Su Yue (2):
  gfs2: rename gfs2_write_jdata_folio to gfs2_write_jdata_full_folio
  gfs2: do not call gfs2_jdata_writepages for jdata

 fs/gfs2/aops.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
 fs/gfs2/aops.h |  1 +
 fs/gfs2/log.c  |  6 +++++-
 3 files changed, 49 insertions(+), 4 deletions(-)

-- 
2.48.1

[PATCH 1/2] gfs2: rename gfs2_write_jdata_folio to gfs2_write_jdata_full_folio

[Su Yue]

As the functions calls block_write_full_folio(), gfs2_write_jdata_full_folio
is more proper. Reserve name gfs2_write_jdata_folio for futher use.

Signed-off-by: Su Yue <glass.su@suse.com>
---
 fs/gfs2/aops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 68fc8af14700..1a9e4ed33911 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -82,14 +82,14 @@ static int gfs2_get_block_noalloc(struct inode *inode, sector_t lblock,
 }
 
 /**
- * gfs2_write_jdata_folio - gfs2 jdata-specific version of block_write_full_folio
+ * gfs2_write_jdata_full_folio - gfs2 jdata-specific version of block_write_full_folio
  * @folio: The folio to write
  * @wbc: The writeback control
  *
  * This is the same as calling block_write_full_folio, but it also
  * writes pages outside of i_size
  */
-static int gfs2_write_jdata_folio(struct folio *folio,
+static int gfs2_write_jdata_full_folio(struct folio *folio,
 				 struct writeback_control *wbc)
 {
 	struct inode * const inode = folio->mapping->host;
@@ -135,7 +135,7 @@ static int __gfs2_jdata_write_folio(struct folio *folio,
 		}
 		gfs2_trans_add_databufs(ip, folio, 0, folio_size(folio));
 	}
-	return gfs2_write_jdata_folio(folio, wbc);
+	return gfs2_write_jdata_full_folio(folio, wbc);
 }
 
 /**
-- 
2.48.1

[PATCH 2/2] gfs2: do not call gfs2_jdata_writepages for jdata

[Su Yue]

The script can triger hang of umount:

==========================================================================
  dev="/dev/vdb2"
  mnt="/mnt"
  user=root

  yes | mkfs.gfs2 $dev -p lock_nolock
  mount -o quota=account $dev $mnt
  echo 1 > /sys/kernel/debug/tracing/tracing_on
  quotacheck -ug $mnt
  quotaon -ug $mnt
  quotaon --project $mnt
  chmod ugo+rwx $mnt
  setquota -u $user $((250 * 4096 / 1024)) $((500 * 4096 / 1024)) 3 5 $mnt
  umount $mnt # hangs
==========================================================================

  [  247.018171 ] INFO: task umount:1004 blocked for more than 122 seconds.
  [  247.018398 ]       Not tainted 6.11.0-09378-gfbb86b0d5f38 #235
  [  247.018598 ] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  [  247.018866 ] task:umount          state:D stack:0     pid:1004  tgid:1004  ppid:988    flags:0x00004006
  [  247.019191 ] Call Trace:
  [  247.019282 ]  <TASK>
  [  247.019358 ]  __schedule+0x404/0x1410
  [  247.019485 ]  schedule+0x27/0xf0
  [  247.019600 ]  schedule_preempt_disabled+0x15/0x30
  [  247.019761 ]  rwsem_down_read_slowpath+0x26f/0x4e0
  [  247.019925 ]  ? sched_balance_find_src_group+0x51/0x580
  [  247.020106 ]  down_read+0x48/0xa0
  [  247.020232 ]  __gfs2_trans_begin+0x119/0x290   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.020535 ]  gfs2_trans_begin+0x4b/0x90   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.020816 ]  gfs2_write_cache_jdata+0x163/0x680   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.021138 ]  gfs2_jdata_writepages+0x29/0x70   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.021449 ]  gfs2_ail1_flush+0x1c9/0x4c0   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.021731 ]  ? psi_group_change+0x12a/0x300
  [  247.021882 ]  gfs2_ail1_start+0x4b/0x70   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.022175 ]  gfs2_log_flush+0x9fd/0xb70   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.022458 ]  gfs2_kill_sb+0x39/0x170   [gfs2 0d63e36c3a51bc921f68927c362d00048655daca]
  [  247.022731 ]  deactivate_locked_super+0x33/0xb0
  [  247.022890 ]  cleanup_mnt+0xba/0x150
  [  247.023018 ]  task_work_run+0x5c/0x90
  [  247.023148 ]  syscall_exit_to_user_mode+0x1b2/0x1c0
  [  247.023316 ]  do_syscall_64+0x8e/0x190
  [  247.023449 ]  ? syscall_exit_to_user_mode+0x37/0x1c0
  [  247.023618 ]  ? do_syscall_64+0x8e/0x190
  [  247.023751 ]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
  [  247.023931 ] RIP: 0033:0x7f1730c2756b
  [  247.024068 ] RSP: 002b:00007fff3db03018 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
  [  247.024334 ] RAX: 0000000000000000 RBX: 0000562517583560 RCX: 00007f1730c2756b
  [  247.024582 ] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000562517583970
  [  247.024828 ] RBP: 00007fff3db030f0 R08: 0000000000000000 R09: 0000000000000000
  [  247.025073 ] R10: 0000000000000800 R11: 0000000000000246 R12: 0000562517583668
  [  247.025320 ] R13: 0000000000000000 R14: 0000562517583970 R15: 0000000000000000
  [  247.025568 ]  </TASK>
==========================================================================

While in gfs2_ail1_flush(),  gfs2_log_flush() has already down_write sdp->sd_log_flush_lock
but gfs2_jdata_writepages() stil calls gfs2_trans_begin() to down_read the semaphore.

The commit 8d391972ae2d ("gfs2: Remove __gfs2_writepage()") overlooked differences between
gfs2_jdata_writepage() and gfs2_jdata_writepages(). The later calls gfs2_write_cache_jdata()
which 'start transactions before we grab page locks'.

The commit partly revert commit 8d391972ae2d ("gfs2: Remove __gfs2_writepage()").
__gfs2_writepage() is back as gfs2_write_jdata_folio() which is called by
exported gfs2_write_jdata_folios().
In gfs2_log_shutdown(), if the inode is jdata then call gfs2_write_jdata_folios()
to write all folios without new transaction started just like old behavior
before 8d391972ae2d.

Fixes: 8d391972ae2d ("gfs2: Remove __gfs2_writepage()")
Signed-off-by: Su Yue <glass.su@suse.com>
---
 fs/gfs2/aops.c | 40 ++++++++++++++++++++++++++++++++++++++++
 fs/gfs2/aops.h |  1 +
 fs/gfs2/log.c  |  6 +++++-
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 1a9e4ed33911..26ed68e9529a 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -138,6 +138,46 @@ static int __gfs2_jdata_write_folio(struct folio *folio,
 	return gfs2_write_jdata_full_folio(folio, wbc);
 }
 
+/**
+ * gfs2_write_jdata_folio - Write folio
+ * @folio: folio to write
+ * @wbc: The writeback control
+ *
+ * Returns: errno
+ *
+ */
+
+static int gfs2_write_jdata_folio(struct folio *folio, struct writeback_control *wbc)
+{
+	struct inode *inode = folio->mapping->host;
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
+
+	if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == LM_ST_EXCLUSIVE))
+		goto out;
+	if (folio_test_checked(folio) || current->journal_info)
+		goto out_ignore;
+	return __gfs2_jdata_write_folio(folio, wbc);
+
+out_ignore:
+	folio_redirty_for_writepage(wbc, folio);
+out:
+	folio_unlock(folio);
+	return 0;
+}
+
+int gfs2_write_jdata_folios(struct address_space *mapping, struct writeback_control *wbc)
+{
+	struct folio *folio = NULL;
+	int error;
+
+	while ((folio = writeback_iter(mapping, wbc, folio, &error)))
+		error = gfs2_write_jdata_folio(folio, wbc);
+
+	return error;
+}
+
+
 /**
  * gfs2_writepages - Write a bunch of dirty pages back to disk
  * @mapping: The mapping to write
diff --git a/fs/gfs2/aops.h b/fs/gfs2/aops.h
index a10c4334d248..4364693486ab 100644
--- a/fs/gfs2/aops.h
+++ b/fs/gfs2/aops.h
@@ -11,5 +11,6 @@
 void adjust_fs_space(struct inode *inode);
 void gfs2_trans_add_databufs(struct gfs2_inode *ip, struct folio *folio,
 			     size_t from, size_t len);
+int gfs2_write_jdata_folios(struct address_space *mapping, struct writeback_control *wbc);
 
 #endif /* __AOPS_DOT_H__ */
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index f9c5089783d2..586f103f01c2 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -31,6 +31,7 @@
 #include "dir.h"
 #include "trace_gfs2.h"
 #include "trans.h"
+#include "aops.h"
 
 static void gfs2_log_shutdown(struct gfs2_sbd *sdp);
 
@@ -131,7 +132,10 @@ __acquires(&sdp->sd_ail_lock)
 		if (!mapping)
 			continue;
 		spin_unlock(&sdp->sd_ail_lock);
-		ret = mapping->a_ops->writepages(mapping, wbc);
+		if (gfs2_is_jdata(GFS2_I(mapping->host)))
+			ret = gfs2_write_jdata_folios(mapping, wbc);
+		else
+			ret = mapping->a_ops->writepages(mapping, wbc);
 		if (need_resched()) {
 			blk_finish_plug(plug);
 			cond_resched();
-- 
2.48.1

Re: [PATCH 2/2] gfs2: do not call gfs2_jdata_writepages for jdata

[Matthew Wilcox]

On Fri, Jun 06, 2025 at 05:47:56PM +0800, Su Yue wrote:
> +++ b/fs/gfs2/aops.c
> @@ -138,6 +138,46 @@ static int __gfs2_jdata_write_folio(struct folio *folio,
>  	return gfs2_write_jdata_full_folio(folio, wbc);
>  }
>  
> +/**
> + * gfs2_write_jdata_folio - Write folio
> + * @folio: folio to write
> + * @wbc: The writeback control
> + *
> + * Returns: errno
> + *
> + */
> +
> +static int gfs2_write_jdata_folio(struct folio *folio, struct writeback_control *wbc)
> +{
> +	struct inode *inode = folio->mapping->host;
> +	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_sbd *sdp = GFS2_SB(inode);
> +
> +	if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == LM_ST_EXCLUSIVE))
> +		goto out;
> +	if (folio_test_checked(folio) || current->journal_info)
> +		goto out_ignore;
> +	return __gfs2_jdata_write_folio(folio, wbc);
> +
> +out_ignore:
> +	folio_redirty_for_writepage(wbc, folio);
> +out:
> +	folio_unlock(folio);
> +	return 0;
> +}
> +
> +int gfs2_write_jdata_folios(struct address_space *mapping, struct writeback_control *wbc)
> +{
> +	struct folio *folio = NULL;
> +	int error;
> +
> +	while ((folio = writeback_iter(mapping, wbc, folio, &error)))
> +		error = gfs2_write_jdata_folio(folio, wbc);
> +
> +	return error;
> +}

The great thing about using an iterator pattern rather than a callback
pattern is that we don't need this stupid extra function.  So you can
drop patch 1 and just do this:

+	struct inode *inode = mapping->host;
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
+
+	if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == LM_ST_EXCLUSIVE))
+		return -Ewhatever;
+
+	while ((folio = writeback_iter(mapping, wbc, folio, &error))) {
+		if (folio_test_checked(folio) || current->journal_info)
+			goto ignore;
+		error = __gfs2_jdata_write_folio(folio, wbc);
+		continue;
+
+ignore:
+		folio_redirty_for_writepage(wbc, folio);
+		folio_unlock(folio);
+	}
+
+	return error;

Re: [PATCH 2/2] gfs2: do not call gfs2_jdata_writepages for jdata

[Su Yue]

On Fri 06 Jun 2025 at 13:27, Matthew Wilcox <willy@infradead.org> 
wrote:

> On Fri, Jun 06, 2025 at 05:47:56PM +0800, Su Yue wrote:
>> +++ b/fs/gfs2/aops.c
>> @@ -138,6 +138,46 @@ static int __gfs2_jdata_write_folio(struct 
>> folio *folio,
>>  	return gfs2_write_jdata_full_folio(folio, wbc);
>>  }
>>
>> +/**
>> + * gfs2_write_jdata_folio - Write folio
>> + * @folio: folio to write
>> + * @wbc: The writeback control
>> + *
>> + * Returns: errno
>> + *
>> + */
>> +
>> +static int gfs2_write_jdata_folio(struct folio *folio, struct 
>> writeback_control *wbc)
>> +{
>> +	struct inode *inode = folio->mapping->host;
>> +	struct gfs2_inode *ip = GFS2_I(inode);
>> +	struct gfs2_sbd *sdp = GFS2_SB(inode);
>> +
>> +	if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == 
>> LM_ST_EXCLUSIVE))
>> +		goto out;
>> +	if (folio_test_checked(folio) || current->journal_info)
>> +		goto out_ignore;
>> +	return __gfs2_jdata_write_folio(folio, wbc);
>> +
>> +out_ignore:
>> +	folio_redirty_for_writepage(wbc, folio);
>> +out:
>> +	folio_unlock(folio);
>> +	return 0;
>> +}
>> +
>> +int gfs2_write_jdata_folios(struct address_space *mapping, 
>> struct writeback_control *wbc)
>> +{
>> +	struct folio *folio = NULL;
>> +	int error;
>> +
>> +	while ((folio = writeback_iter(mapping, wbc, folio, 
>> &error)))
>> +		error = gfs2_write_jdata_folio(folio, wbc);
>> +
>> +	return error;
>> +}
>
> The great thing about using an iterator pattern rather than a 
> callback
> pattern is that we don't need this stupid extra function.  So 
> you can
> drop patch 1 and just do this:
>
Thanks. That's what Andreas wrote in his patch 5a90f8d49922  :)

--
Su
> +	struct inode *inode = mapping->host;
> +	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_sbd *sdp = GFS2_SB(inode);
> +
> +	if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == 
> LM_ST_EXCLUSIVE))
> +		return -Ewhatever;
> +
> +	while ((folio = writeback_iter(mapping, wbc, folio, &error))) 
> {
> +		if (folio_test_checked(folio) || current->journal_info)
> +			goto ignore;
> +		error = __gfs2_jdata_write_folio(folio, wbc);
> +		continue;
> +
> +ignore:
> +		folio_redirty_for_writepage(wbc, folio);
> +		folio_unlock(folio);
> +	}
> +
> +	return error;

Re: [PATCH 0/2] GFS2: Fix wrong call of new trans for jdata

[Su Yue]

On Fri 06 Jun 2025 at 17:47, Su Yue <glass.su@suse.com> wrote:

> Patch 1 is trivial.
> Please see commit message of patch 2/2.
>
Please ignore the patchset. The issue was fixed by
commit 5a90f8d49922 ("gfs2: Don't start unnecessary transactions 
during log flush").

--
Su
> Su Yue (2):
>   gfs2: rename gfs2_write_jdata_folio to 
>   gfs2_write_jdata_full_folio
>   gfs2: do not call gfs2_jdata_writepages for jdata
>
>  fs/gfs2/aops.c | 46 
>  +++++++++++++++++++++++++++++++++++++++++++---
>  fs/gfs2/aops.h |  1 +
>  fs/gfs2/log.c  |  6 +++++-
>  3 files changed, 49 insertions(+), 4 deletions(-)