[PATCH v3 00/10] ext4: fix insufficient credits when writing back large folios

[PATCH v3 00/10] ext4: fix insufficient credits when writing back large folios

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Changes since v2:
 - Convert the processing of folios writeback in bytes instead of pages.
 - Refactor ext4_page_mkwrite() and ensure journal credits in
   ext4_block_write_begin() instead of in _ext4_get_block().
 - Enhance tracepoints in ext4_do_writepages().
 - Replace the outdated ext4_da_writepages_trans_blocks() and
   ext4_writepage_trans_blocks() with the new helper used to reserve
   credits for a single extent.
Changes since v1:
 - Make the write-back process supports writing a partial folio if it
   exits the mapping loop prematurely due to insufficient sapce or
   journal credits, it also fix the potential stale data and
   inconsistency issues.
 - Fix the same issue regarding the allocation of blocks in
   ext4_write_begin() and ext4_page_mkwrite() when delalloc is not
   enabled.

v2: https://lore.kernel.org/linux-ext4/20250611111625.1668035-1-yi.zhang@huaweicloud.com/
v1: https://lore.kernel.org/linux-ext4/20250530062858.458039-1-yi.zhang@huaweicloud.com/

Original Description

This series addresses the issue that Jan pointed out regarding large
folios support for ext4[1]. The problem is that the credits calculation
may insufficient in ext4_meta_trans_blocks() when allocating blocks
during write back a sufficiently large and discontinuous folio, it
doesn't involve the credits for updating bitmap and group descriptor
block. However, if we fix this issue, it may lead to significant
overestimation on the some filesystems with a lot of block groups.

The solution involves first ensure that the current journal transaction
has enough credits when we mapping an extent during allocating blocks.
Then if the credits reach the upper limit, exit the current mapping
loop, submit the partial folio and restart a new transaction. Finally,
fix the wrong credits calculation in ext4_meta_trans_blocks(). Please
see the following patches for details.

[1] https://lore.kernel.org/linux-ext4/ht54j6bvjmiqt62xmcveqlo7bmrunqs4ji7wikfteftdjijzek@7tz5gpejaoen/

Thanks,
Yi.


Zhang Yi (10):
  ext4: process folios writeback in bytes
  ext4: move the calculation of wbc->nr_to_write to mpage_folio_done()
  ext4: fix stale data if it bail out of the extents mapping loop
  ext4: refactor the block allocation process of ext4_page_mkwrite()
  ext4: restart handle if credits are insufficient during allocating
    blocks
  ext4: enhance tracepoints during the folios writeback
  ext4: correct the reserved credits for extent conversion
  ext4: reserved credits for one extent during the folio writeback
  ext4: replace ext4_writepage_trans_blocks()
  ext4: fix insufficient credits calculation in ext4_meta_trans_blocks()

 fs/ext4/ext4.h              |   2 +-
 fs/ext4/extents.c           |   6 +-
 fs/ext4/inline.c            |   6 +-
 fs/ext4/inode.c             | 324 ++++++++++++++++++++++--------------
 fs/ext4/move_extent.c       |   3 +-
 fs/ext4/xattr.c             |   2 +-
 include/trace/events/ext4.h |  47 +++++-
 7 files changed, 248 insertions(+), 142 deletions(-)

-- 
2.46.1

[PATCH v3 01/10] ext4: process folios writeback in bytes

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Since ext4 supports large folios, processing writebacks in pages is no
longer appropriate, it can be modified to process writebacks in bytes.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c             | 70 +++++++++++++++++++------------------
 include/trace/events/ext4.h | 13 ++++---
 2 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index be9a4cba35fd..ba81df0d87dd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1667,11 +1667,12 @@ struct mpage_da_data {
 	unsigned int can_map:1;	/* Can writepages call map blocks? */
 
 	/* These are internal state of ext4_do_writepages() */
-	pgoff_t first_page;	/* The first page to write */
-	pgoff_t next_page;	/* Current page to examine */
-	pgoff_t last_page;	/* Last page to examine */
+	loff_t start_pos;	/* The start pos to write */
+	loff_t next_pos;	/* Current pos to examine */
+	loff_t end_pos;		/* Last pos to examine */
+
 	/*
-	 * Extent to map - this can be after first_page because that can be
+	 * Extent to map - this can be after start_pos because that can be
 	 * fully mapped. We somewhat abuse m_flags to store whether the extent
 	 * is delalloc or unwritten.
 	 */
@@ -1691,38 +1692,38 @@ static void mpage_release_unused_pages(struct mpage_da_data *mpd,
 	struct inode *inode = mpd->inode;
 	struct address_space *mapping = inode->i_mapping;
 
-	/* This is necessary when next_page == 0. */
-	if (mpd->first_page >= mpd->next_page)
+	/* This is necessary when next_pos == 0. */
+	if (mpd->start_pos >= mpd->next_pos)
 		return;
 
 	mpd->scanned_until_end = 0;
-	index = mpd->first_page;
-	end   = mpd->next_page - 1;
 	if (invalidate) {
 		ext4_lblk_t start, last;
-		start = index << (PAGE_SHIFT - inode->i_blkbits);
-		last = end << (PAGE_SHIFT - inode->i_blkbits);
+		start = EXT4_B_TO_LBLK(inode, mpd->start_pos);
+		last = mpd->next_pos >> inode->i_blkbits;
 
 		/*
 		 * avoid racing with extent status tree scans made by
 		 * ext4_insert_delayed_block()
 		 */
 		down_write(&EXT4_I(inode)->i_data_sem);
-		ext4_es_remove_extent(inode, start, last - start + 1);
+		ext4_es_remove_extent(inode, start, last - start);
 		up_write(&EXT4_I(inode)->i_data_sem);
 	}
 
 	folio_batch_init(&fbatch);
-	while (index <= end) {
-		nr = filemap_get_folios(mapping, &index, end, &fbatch);
+	index = mpd->start_pos >> PAGE_SHIFT;
+	end = mpd->next_pos >> PAGE_SHIFT;
+	while (index < end) {
+		nr = filemap_get_folios(mapping, &index, end - 1, &fbatch);
 		if (nr == 0)
 			break;
 		for (i = 0; i < nr; i++) {
 			struct folio *folio = fbatch.folios[i];
 
-			if (folio->index < mpd->first_page)
+			if (folio_pos(folio) < mpd->start_pos)
 				continue;
-			if (folio_next_index(folio) - 1 > end)
+			if (folio_next_index(folio) > end)
 				continue;
 			BUG_ON(!folio_test_locked(folio));
 			BUG_ON(folio_test_writeback(folio));
@@ -2024,7 +2025,7 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
 
 static void mpage_folio_done(struct mpage_da_data *mpd, struct folio *folio)
 {
-	mpd->first_page += folio_nr_pages(folio);
+	mpd->start_pos += folio_size(folio);
 	folio_unlock(folio);
 }
 
@@ -2034,7 +2035,7 @@ static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
 	loff_t size;
 	int err;
 
-	BUG_ON(folio->index != mpd->first_page);
+	WARN_ON_ONCE(folio_pos(folio) != mpd->start_pos);
 	folio_clear_dirty_for_io(folio);
 	/*
 	 * We have to be very careful here!  Nothing protects writeback path
@@ -2446,7 +2447,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 	 * Update on-disk size after IO is submitted.  Races with
 	 * truncate are avoided by checking i_size under i_data_sem.
 	 */
-	disksize = ((loff_t)mpd->first_page) << PAGE_SHIFT;
+	disksize = mpd->start_pos;
 	if (disksize > READ_ONCE(EXT4_I(inode)->i_disksize)) {
 		int err2;
 		loff_t i_size;
@@ -2549,8 +2550,8 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 	struct address_space *mapping = mpd->inode->i_mapping;
 	struct folio_batch fbatch;
 	unsigned int nr_folios;
-	pgoff_t index = mpd->first_page;
-	pgoff_t end = mpd->last_page;
+	pgoff_t index = mpd->start_pos >> PAGE_SHIFT;
+	pgoff_t end = mpd->end_pos >> PAGE_SHIFT;
 	xa_mark_t tag;
 	int i, err = 0;
 	int blkbits = mpd->inode->i_blkbits;
@@ -2565,7 +2566,7 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 		tag = PAGECACHE_TAG_DIRTY;
 
 	mpd->map.m_len = 0;
-	mpd->next_page = index;
+	mpd->next_pos = mpd->start_pos;
 	if (ext4_should_journal_data(mpd->inode)) {
 		handle = ext4_journal_start(mpd->inode, EXT4_HT_WRITE_PAGE,
 					    bpp);
@@ -2596,7 +2597,8 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 				goto out;
 
 			/* If we can't merge this page, we are done. */
-			if (mpd->map.m_len > 0 && mpd->next_page != folio->index)
+			if (mpd->map.m_len > 0 &&
+			    mpd->next_pos != folio_pos(folio))
 				goto out;
 
 			if (handle) {
@@ -2642,8 +2644,8 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 			}
 
 			if (mpd->map.m_len == 0)
-				mpd->first_page = folio->index;
-			mpd->next_page = folio_next_index(folio);
+				mpd->start_pos = folio_pos(folio);
+			mpd->next_pos = folio_pos(folio) + folio_size(folio);
 			/*
 			 * Writeout when we cannot modify metadata is simple.
 			 * Just submit the page. For data=journal mode we
@@ -2786,18 +2788,18 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		writeback_index = mapping->writeback_index;
 		if (writeback_index)
 			cycled = 0;
-		mpd->first_page = writeback_index;
-		mpd->last_page = -1;
+		mpd->start_pos = writeback_index << PAGE_SHIFT;
+		mpd->end_pos = -1;
 	} else {
-		mpd->first_page = wbc->range_start >> PAGE_SHIFT;
-		mpd->last_page = wbc->range_end >> PAGE_SHIFT;
+		mpd->start_pos = wbc->range_start;
+		mpd->end_pos = wbc->range_end;
 	}
 
 	ext4_io_submit_init(&mpd->io_submit, wbc);
 retry:
 	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
-		tag_pages_for_writeback(mapping, mpd->first_page,
-					mpd->last_page);
+		tag_pages_for_writeback(mapping, mpd->start_pos >> PAGE_SHIFT,
+					mpd->end_pos >> PAGE_SHIFT);
 	blk_start_plug(&plug);
 
 	/*
@@ -2857,7 +2859,7 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		}
 		mpd->do_map = 1;
 
-		trace_ext4_da_write_pages(inode, mpd->first_page, wbc);
+		trace_ext4_da_write_pages(inode, mpd->start_pos, wbc);
 		ret = mpage_prepare_extent_to_map(mpd);
 		if (!ret && mpd->map.m_len)
 			ret = mpage_map_and_submit_extent(handle, mpd,
@@ -2914,8 +2916,8 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	blk_finish_plug(&plug);
 	if (!ret && !cycled && wbc->nr_to_write > 0) {
 		cycled = 1;
-		mpd->last_page = writeback_index - 1;
-		mpd->first_page = 0;
+		mpd->end_pos = (writeback_index << PAGE_SHIFT) - 1;
+		mpd->start_pos = 0;
 		goto retry;
 	}
 
@@ -2925,7 +2927,7 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		 * Set the writeback_index so that range_cyclic
 		 * mode will write it back later
 		 */
-		mapping->writeback_index = mpd->first_page;
+		mapping->writeback_index = mpd->start_pos >> PAGE_SHIFT;
 
 out_writepages:
 	trace_ext4_writepages_result(inode, wbc, ret,
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 156908641e68..62d52997b5c6 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -483,15 +483,15 @@ TRACE_EVENT(ext4_writepages,
 );
 
 TRACE_EVENT(ext4_da_write_pages,
-	TP_PROTO(struct inode *inode, pgoff_t first_page,
+	TP_PROTO(struct inode *inode, loff_t start_pos,
 		 struct writeback_control *wbc),
 
-	TP_ARGS(inode, first_page, wbc),
+	TP_ARGS(inode, start_pos, wbc),
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)
 		__field(	ino_t,	ino			)
-		__field(      pgoff_t,	first_page		)
+		__field(       loff_t,	start_pos		)
 		__field(	 long,	nr_to_write		)
 		__field(	  int,	sync_mode		)
 	),
@@ -499,15 +499,14 @@ TRACE_EVENT(ext4_da_write_pages,
 	TP_fast_assign(
 		__entry->dev		= inode->i_sb->s_dev;
 		__entry->ino		= inode->i_ino;
-		__entry->first_page	= first_page;
+		__entry->start_pos	= start_pos;
 		__entry->nr_to_write	= wbc->nr_to_write;
 		__entry->sync_mode	= wbc->sync_mode;
 	),
 
-	TP_printk("dev %d,%d ino %lu first_page %lu nr_to_write %ld "
-		  "sync_mode %d",
+	TP_printk("dev %d,%d ino %lu start_pos 0x%llx nr_to_write %ld sync_mode %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
-		  (unsigned long) __entry->ino, __entry->first_page,
+		  (unsigned long) __entry->ino, __entry->start_pos,
 		  __entry->nr_to_write, __entry->sync_mode)
 );
 
-- 
2.46.1

Re: [PATCH v3 01/10] ext4: process folios writeback in bytes

[Jan Kara]

On Tue 01-07-25 21:06:26, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Since ext4 supports large folios, processing writebacks in pages is no
> longer appropriate, it can be modified to process writebacks in bytes.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Just one small issue. With that fixed feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> @@ -2786,18 +2788,18 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>  		writeback_index = mapping->writeback_index;
>  		if (writeback_index)
>  			cycled = 0;
> -		mpd->first_page = writeback_index;
> -		mpd->last_page = -1;
> +		mpd->start_pos = writeback_index << PAGE_SHIFT;
> +		mpd->end_pos = -1;

Careful here. Previously last_page was unsigned long so -1 was fine but now
loff_t is signed. So we should rather store LLONG_MAX here.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 01/10] ext4: process folios writeback in bytes

[Zhang Yi]

On 2025/7/2 22:00, Jan Kara wrote:
> On Tue 01-07-25 21:06:26, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Since ext4 supports large folios, processing writebacks in pages is no
>> longer appropriate, it can be modified to process writebacks in bytes.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> Just one small issue. With that fixed feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
>> @@ -2786,18 +2788,18 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>>  		writeback_index = mapping->writeback_index;
>>  		if (writeback_index)
>>  			cycled = 0;
>> -		mpd->first_page = writeback_index;
>> -		mpd->last_page = -1;
>> +		mpd->start_pos = writeback_index << PAGE_SHIFT;
>> +		mpd->end_pos = -1;
> 
> Careful here. Previously last_page was unsigned long so -1 was fine but now
> loff_t is signed. So we should rather store LLONG_MAX here.
> 
> 									Honza

Ha, right! Sorry for missed this corner, will fix.

Thanks,
Yi.

[PATCH v3 02/10] ext4: move the calculation of wbc->nr_to_write to mpage_folio_done()

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

mpage_folio_done() should be a more appropriate place than
mpage_submit_folio() for updating the wbc->nr_to_write after we have
submitted a fully mapped folio. Preparing to make mpage_submit_folio()
allows to submit partially mapped folio that is still under processing.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ba81df0d87dd..38db1c186f76 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2026,6 +2026,7 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
 static void mpage_folio_done(struct mpage_da_data *mpd, struct folio *folio)
 {
 	mpd->start_pos += folio_size(folio);
+	mpd->wbc->nr_to_write -= folio_nr_pages(folio);
 	folio_unlock(folio);
 }
 
@@ -2056,8 +2057,6 @@ static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
 	    !ext4_verity_in_progress(mpd->inode))
 		len = size & (len - 1);
 	err = ext4_bio_write_folio(&mpd->io_submit, folio, len);
-	if (!err)
-		mpd->wbc->nr_to_write -= folio_nr_pages(folio);
 
 	return err;
 }
-- 
2.46.1

[PATCH v3 03/10] ext4: fix stale data if it bail out of the extents mapping loop

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

During the process of writing back folios, if
mpage_map_and_submit_extent() exits the extent mapping loop due to an
ENOSPC or ENOMEM error, it may result in stale data or filesystem
inconsistency in environments where the block size is smaller than the
folio size.

When mapping a discontinuous folio in mpage_map_and_submit_extent(),
some buffers may have already be mapped. If we exit the mapping loop
prematurely, the folio data within the mapped range will not be written
back, and the file's disk size will not be updated. Once the transaction
that includes this range of extents is committed, this can lead to stale
data or filesystem inconsistency.

Fix this by submitting the current processing partially mapped folio.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 38db1c186f76..62f1263d05da 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2361,6 +2361,47 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
 	return 0;
 }
 
+/*
+ * This is used to submit mapped buffers in a single folio that is not fully
+ * mapped for various reasons, such as insufficient space or journal credits.
+ */
+static int mpage_submit_buffers(struct mpage_da_data *mpd)
+{
+	struct inode *inode = mpd->inode;
+	struct folio *folio;
+	loff_t pos;
+	int ret;
+
+	folio = filemap_get_folio(inode->i_mapping,
+				  mpd->start_pos >> PAGE_SHIFT);
+	if (IS_ERR(folio))
+		return PTR_ERR(folio);
+	/*
+	 * The mapped position should be within the current processing folio
+	 * but must not be the folio start position.
+	 */
+	pos = mpd->map.m_lblk << inode->i_blkbits;
+	if (WARN_ON_ONCE((folio_pos(folio) == pos) ||
+			 !folio_contains(folio, pos >> PAGE_SHIFT)))
+		return -EINVAL;
+
+	ret = mpage_submit_folio(mpd, folio);
+	if (ret)
+		goto out;
+	/*
+	 * Update start_pos to prevent this folio from being released in
+	 * mpage_release_unused_pages(), it will be reset to the aligned folio
+	 * pos when this folio is written again in the next round. Additionally,
+	 * do not update wbc->nr_to_write here, as it will be updated once the
+	 * entire folio has finished processing.
+	 */
+	mpd->start_pos = pos;
+out:
+	folio_unlock(folio);
+	folio_put(folio);
+	return ret;
+}
+
 /*
  * mpage_map_and_submit_extent - map extent starting at mpd->lblk of length
  *				 mpd->len and submit pages underlying it for IO
@@ -2411,8 +2452,16 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 			 */
 			if ((err == -ENOMEM) ||
 			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
-				if (progress)
+				/*
+				 * We may have already allocated extents for
+				 * some bhs inside the folio, issue the
+				 * corresponding data to prevent stale data.
+				 */
+				if (progress) {
+					if (mpage_submit_buffers(mpd))
+						goto invalidate_dirty_pages;
 					goto update_disksize;
+				}
 				return err;
 			}
 			ext4_msg(sb, KERN_CRIT,
-- 
2.46.1

Re: [PATCH v3 03/10] ext4: fix stale data if it bail out of the extents mapping loop

[Jan Kara]

On Tue 01-07-25 21:06:28, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> During the process of writing back folios, if
> mpage_map_and_submit_extent() exits the extent mapping loop due to an
> ENOSPC or ENOMEM error, it may result in stale data or filesystem
> inconsistency in environments where the block size is smaller than the
> folio size.
> 
> When mapping a discontinuous folio in mpage_map_and_submit_extent(),
> some buffers may have already be mapped. If we exit the mapping loop
> prematurely, the folio data within the mapped range will not be written
> back, and the file's disk size will not be updated. Once the transaction
> that includes this range of extents is committed, this can lead to stale
> data or filesystem inconsistency.
> 
> Fix this by submitting the current processing partially mapped folio.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

Just one naming suggestion below:

> +/*
> + * This is used to submit mapped buffers in a single folio that is not fully
> + * mapped for various reasons, such as insufficient space or journal credits.
> + */
> +static int mpage_submit_buffers(struct mpage_da_data *mpd)

mpage_submit_buffers() sounds somewhat generic. How about
mpage_submit_partial_folio()?

								Honza

> +{
> +	struct inode *inode = mpd->inode;
> +	struct folio *folio;
> +	loff_t pos;
> +	int ret;
> +
> +	folio = filemap_get_folio(inode->i_mapping,
> +				  mpd->start_pos >> PAGE_SHIFT);
> +	if (IS_ERR(folio))
> +		return PTR_ERR(folio);
> +	/*
> +	 * The mapped position should be within the current processing folio
> +	 * but must not be the folio start position.
> +	 */
> +	pos = mpd->map.m_lblk << inode->i_blkbits;
> +	if (WARN_ON_ONCE((folio_pos(folio) == pos) ||
> +			 !folio_contains(folio, pos >> PAGE_SHIFT)))
> +		return -EINVAL;
> +
> +	ret = mpage_submit_folio(mpd, folio);
> +	if (ret)
> +		goto out;
> +	/*
> +	 * Update start_pos to prevent this folio from being released in
> +	 * mpage_release_unused_pages(), it will be reset to the aligned folio
> +	 * pos when this folio is written again in the next round. Additionally,
> +	 * do not update wbc->nr_to_write here, as it will be updated once the
> +	 * entire folio has finished processing.
> +	 */
> +	mpd->start_pos = pos;
> +out:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +	return ret;
> +}
> +
>  /*
>   * mpage_map_and_submit_extent - map extent starting at mpd->lblk of length
>   *				 mpd->len and submit pages underlying it for IO
> @@ -2411,8 +2452,16 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  			 */
>  			if ((err == -ENOMEM) ||
>  			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
> -				if (progress)
> +				/*
> +				 * We may have already allocated extents for
> +				 * some bhs inside the folio, issue the
> +				 * corresponding data to prevent stale data.
> +				 */
> +				if (progress) {
> +					if (mpage_submit_buffers(mpd))
> +						goto invalidate_dirty_pages;
>  					goto update_disksize;
> +				}
>  				return err;
>  			}
>  			ext4_msg(sb, KERN_CRIT,
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 03/10] ext4: fix stale data if it bail out of the extents mapping loop

[Zhang Yi]

On 2025/7/2 22:07, Jan Kara wrote:
> On Tue 01-07-25 21:06:28, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> During the process of writing back folios, if
>> mpage_map_and_submit_extent() exits the extent mapping loop due to an
>> ENOSPC or ENOMEM error, it may result in stale data or filesystem
>> inconsistency in environments where the block size is smaller than the
>> folio size.
>>
>> When mapping a discontinuous folio in mpage_map_and_submit_extent(),
>> some buffers may have already be mapped. If we exit the mapping loop
>> prematurely, the folio data within the mapped range will not be written
>> back, and the file's disk size will not be updated. Once the transaction
>> that includes this range of extents is committed, this can lead to stale
>> data or filesystem inconsistency.
>>
>> Fix this by submitting the current processing partially mapped folio.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Just one naming suggestion below:
> 
>> +/*
>> + * This is used to submit mapped buffers in a single folio that is not fully
>> + * mapped for various reasons, such as insufficient space or journal credits.
>> + */
>> +static int mpage_submit_buffers(struct mpage_da_data *mpd)
> 
> mpage_submit_buffers() sounds somewhat generic. How about
> mpage_submit_partial_folio()?
> 
> 								Honza
>

Yeah, mpage_submit_partial_folio looks better to me.

Thanks,
Yi.


>> +{
>> +	struct inode *inode = mpd->inode;
>> +	struct folio *folio;
>> +	loff_t pos;
>> +	int ret;
>> +
>> +	folio = filemap_get_folio(inode->i_mapping,
>> +				  mpd->start_pos >> PAGE_SHIFT);
>> +	if (IS_ERR(folio))
>> +		return PTR_ERR(folio);
>> +	/*
>> +	 * The mapped position should be within the current processing folio
>> +	 * but must not be the folio start position.
>> +	 */
>> +	pos = mpd->map.m_lblk << inode->i_blkbits;
>> +	if (WARN_ON_ONCE((folio_pos(folio) == pos) ||
>> +			 !folio_contains(folio, pos >> PAGE_SHIFT)))
>> +		return -EINVAL;
>> +
>> +	ret = mpage_submit_folio(mpd, folio);
>> +	if (ret)
>> +		goto out;
>> +	/*
>> +	 * Update start_pos to prevent this folio from being released in
>> +	 * mpage_release_unused_pages(), it will be reset to the aligned folio
>> +	 * pos when this folio is written again in the next round. Additionally,
>> +	 * do not update wbc->nr_to_write here, as it will be updated once the
>> +	 * entire folio has finished processing.
>> +	 */
>> +	mpd->start_pos = pos;
>> +out:
>> +	folio_unlock(folio);
>> +	folio_put(folio);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * mpage_map_and_submit_extent - map extent starting at mpd->lblk of length
>>   *				 mpd->len and submit pages underlying it for IO
>> @@ -2411,8 +2452,16 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>>  			 */
>>  			if ((err == -ENOMEM) ||
>>  			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
>> -				if (progress)
>> +				/*
>> +				 * We may have already allocated extents for
>> +				 * some bhs inside the folio, issue the
>> +				 * corresponding data to prevent stale data.
>> +				 */
>> +				if (progress) {
>> +					if (mpage_submit_buffers(mpd))
>> +						goto invalidate_dirty_pages;
>>  					goto update_disksize;
>> +				}
>>  				return err;
>>  			}
>>  			ext4_msg(sb, KERN_CRIT,
>> -- 
>> 2.46.1
>>

[PATCH v3 04/10] ext4: refactor the block allocation process of ext4_page_mkwrite()

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

The block allocation process and error handling in ext4_page_mkwrite()
is complex now. Refactor it by introducing a new helper function,
ext4_block_page_mkwrite(). It will call ext4_block_write_begin() to
allocate blocks instead of directly calling block_page_mkwrite().
Preparing to implement retry logic in a subsequent patch to address
situations where the reserved journal credits are insufficient.
Additionally, this modification will help prevent potential deadlocks
that may occur when waiting for folio writeback while holding the
transaction handle.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c | 95 ++++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 45 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 62f1263d05da..31731a732df2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6605,6 +6605,53 @@ static int ext4_bh_unmapped(handle_t *handle, struct inode *inode,
 	return !buffer_mapped(bh);
 }
 
+static int ext4_block_page_mkwrite(struct inode *inode, struct folio *folio,
+				   get_block_t get_block)
+{
+	handle_t *handle;
+	loff_t size;
+	unsigned long len;
+	int ret;
+
+	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
+				    ext4_writepage_trans_blocks(inode));
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
+
+	folio_lock(folio);
+	size = i_size_read(inode);
+	/* Page got truncated from under us? */
+	if (folio->mapping != inode->i_mapping || folio_pos(folio) > size) {
+		ret = -EFAULT;
+		goto out_error;
+	}
+
+	len = folio_size(folio);
+	if (folio_pos(folio) + len > size)
+		len = size - folio_pos(folio);
+
+	ret = ext4_block_write_begin(handle, folio, 0, len, get_block);
+	if (ret)
+		goto out_error;
+
+	if (!ext4_should_journal_data(inode)) {
+		block_commit_write(folio, 0, len);
+		folio_mark_dirty(folio);
+	} else {
+		ret = ext4_journal_folio_buffers(handle, folio, len);
+		if (ret)
+			goto out_error;
+	}
+	ext4_journal_stop(handle);
+	folio_wait_stable(folio);
+	return ret;
+
+out_error:
+	folio_unlock(folio);
+	ext4_journal_stop(handle);
+	return ret;
+}
+
 vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
@@ -6616,8 +6663,7 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 	struct file *file = vma->vm_file;
 	struct inode *inode = file_inode(file);
 	struct address_space *mapping = inode->i_mapping;
-	handle_t *handle;
-	get_block_t *get_block;
+	get_block_t *get_block = ext4_get_block;
 	int retries = 0;
 
 	if (unlikely(IS_IMMUTABLE(inode)))
@@ -6685,46 +6731,9 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 	/* OK, we need to fill the hole... */
 	if (ext4_should_dioread_nolock(inode))
 		get_block = ext4_get_block_unwritten;
-	else
-		get_block = ext4_get_block;
 retry_alloc:
-	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
-				    ext4_writepage_trans_blocks(inode));
-	if (IS_ERR(handle)) {
-		ret = VM_FAULT_SIGBUS;
-		goto out;
-	}
-	/*
-	 * Data journalling can't use block_page_mkwrite() because it
-	 * will set_buffer_dirty() before do_journal_get_write_access()
-	 * thus might hit warning messages for dirty metadata buffers.
-	 */
-	if (!ext4_should_journal_data(inode)) {
-		err = block_page_mkwrite(vma, vmf, get_block);
-	} else {
-		folio_lock(folio);
-		size = i_size_read(inode);
-		/* Page got truncated from under us? */
-		if (folio->mapping != mapping || folio_pos(folio) > size) {
-			ret = VM_FAULT_NOPAGE;
-			goto out_error;
-		}
-
-		len = folio_size(folio);
-		if (folio_pos(folio) + len > size)
-			len = size - folio_pos(folio);
-
-		err = ext4_block_write_begin(handle, folio, 0, len,
-					     ext4_get_block);
-		if (!err) {
-			ret = VM_FAULT_SIGBUS;
-			if (ext4_journal_folio_buffers(handle, folio, len))
-				goto out_error;
-		} else {
-			folio_unlock(folio);
-		}
-	}
-	ext4_journal_stop(handle);
+	/* Start jorunal and allocate blocks */
+	err = ext4_block_page_mkwrite(inode, folio, get_block);
 	if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry_alloc;
 out_ret:
@@ -6733,8 +6742,4 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 	filemap_invalidate_unlock_shared(mapping);
 	sb_end_pagefault(inode->i_sb);
 	return ret;
-out_error:
-	folio_unlock(folio);
-	ext4_journal_stop(handle);
-	goto out;
 }
-- 
2.46.1

Re: [PATCH v3 04/10] ext4: refactor the block allocation process of ext4_page_mkwrite()

[Jan Kara]

On Tue 01-07-25 21:06:29, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> The block allocation process and error handling in ext4_page_mkwrite()
> is complex now. Refactor it by introducing a new helper function,
> ext4_block_page_mkwrite(). It will call ext4_block_write_begin() to
> allocate blocks instead of directly calling block_page_mkwrite().
> Preparing to implement retry logic in a subsequent patch to address
> situations where the reserved journal credits are insufficient.
> Additionally, this modification will help prevent potential deadlocks
> that may occur when waiting for folio writeback while holding the
> transaction handle.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

One typo fix below:

> +	/* Start jorunal and allocate blocks */
		 ^^^ journal

> +	err = ext4_block_page_mkwrite(inode, folio, get_block);
>  	if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry_alloc;
>  out_ret:

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

After large folios are supported on ext4, writing back a sufficiently
large and discontinuous folio may consume a significant number of
journal credits, placing considerable strain on the journal. For
example, in a 20GB filesystem with 1K block size and 1MB journal size,
writing back a 2MB folio could require thousands of credits in the
worst-case scenario (when each block is discontinuous and distributed
across different block groups), potentially exceeding the journal size.
This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
when delalloc is not enabled.

Fix this by ensuring that there are sufficient journal credits before
allocating an extent in mpage_map_one_extent() and
ext4_block_write_begin(). If there are not enough credits, return
-EAGAIN, exit the current mapping loop, restart a new handle and a new
transaction, and allocating blocks on this folio again in the next
iteration.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 31731a732df2..efe778aaf74b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -877,6 +877,25 @@ static void ext4_update_bh_state(struct buffer_head *bh, unsigned long flags)
 	} while (unlikely(!try_cmpxchg(&bh->b_state, &old_state, new_state)));
 }
 
+/*
+ * Make sure that the current journal transaction has enough credits to map
+ * one extent. Return -EAGAIN if it cannot extend the current running
+ * transaction.
+ */
+static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
+						     struct inode *inode)
+{
+	int credits;
+	int ret;
+
+	if (!handle)
+		return 0;
+
+	credits = ext4_chunk_trans_blocks(inode, 1);
+	ret = __ext4_journal_ensure_credits(handle, credits, credits, 0);
+	return ret <= 0 ? ret : -EAGAIN;
+}
+
 static int _ext4_get_block(struct inode *inode, sector_t iblock,
 			   struct buffer_head *bh, int flags)
 {
@@ -1175,7 +1194,9 @@ int ext4_block_write_begin(handle_t *handle, struct folio *folio,
 			clear_buffer_new(bh);
 		if (!buffer_mapped(bh)) {
 			WARN_ON(bh->b_size != blocksize);
-			err = get_block(inode, block, bh, 1);
+			err = ext4_journal_ensure_extent_credits(handle, inode);
+			if (!err)
+				err = get_block(inode, block, bh, 1);
 			if (err)
 				break;
 			if (buffer_new(bh)) {
@@ -1374,8 +1395,9 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 				ext4_orphan_del(NULL, inode);
 		}
 
-		if (ret == -ENOSPC &&
-		    ext4_should_retry_alloc(inode->i_sb, &retries))
+		if (ret == -EAGAIN ||
+		    (ret == -ENOSPC &&
+		     ext4_should_retry_alloc(inode->i_sb, &retries)))
 			goto retry_journal;
 		folio_put(folio);
 		return ret;
@@ -2323,6 +2345,11 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
 	int get_blocks_flags;
 	int err, dioread_nolock;
 
+	/* Make sure transaction has enough credits for this extent */
+	err = ext4_journal_ensure_extent_credits(handle, inode);
+	if (err < 0)
+		return err;
+
 	trace_ext4_da_write_pages_extent(inode, map);
 	/*
 	 * Call ext4_map_blocks() to allocate any delayed allocation blocks, or
@@ -2450,7 +2477,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 			 * In the case of ENOSPC, if ext4_count_free_blocks()
 			 * is non-zero, a commit should free up blocks.
 			 */
-			if ((err == -ENOMEM) ||
+			if ((err == -ENOMEM) || (err == -EAGAIN) ||
 			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
 				/*
 				 * We may have already allocated extents for
@@ -2956,6 +2983,8 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 			ret = 0;
 			continue;
 		}
+		if (ret == -EAGAIN)
+			ret = 0;
 		/* Fatal error - ENOMEM, EIO... */
 		if (ret)
 			break;
@@ -6734,7 +6763,8 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 retry_alloc:
 	/* Start jorunal and allocate blocks */
 	err = ext4_block_page_mkwrite(inode, folio, get_block);
-	if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+	if (err == -EAGAIN ||
+	    (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries)))
 		goto retry_alloc;
 out_ret:
 	ret = vmf_fs_error(err);
-- 
2.46.1

Re: [PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Jan Kara]

On Tue 01-07-25 21:06:30, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> After large folios are supported on ext4, writing back a sufficiently
> large and discontinuous folio may consume a significant number of
> journal credits, placing considerable strain on the journal. For
> example, in a 20GB filesystem with 1K block size and 1MB journal size,
> writing back a 2MB folio could require thousands of credits in the
> worst-case scenario (when each block is discontinuous and distributed
> across different block groups), potentially exceeding the journal size.
> This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
> when delalloc is not enabled.
> 
> Fix this by ensuring that there are sufficient journal credits before
> allocating an extent in mpage_map_one_extent() and
> ext4_block_write_begin(). If there are not enough credits, return
> -EAGAIN, exit the current mapping loop, restart a new handle and a new
> transaction, and allocating blocks on this folio again in the next
> iteration.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Very nice. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

One small comment below:

> +/*
> + * Make sure that the current journal transaction has enough credits to map
> + * one extent. Return -EAGAIN if it cannot extend the current running
> + * transaction.
> + */
> +static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
> +						     struct inode *inode)
> +{
> +	int credits;
> +	int ret;
> +
> +	if (!handle)

Shouldn't this rather be ext4_handle_valid(handle) to catch nojournal mode
properly?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Zhang Yi]

On 2025/7/2 22:18, Jan Kara wrote:
> On Tue 01-07-25 21:06:30, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> After large folios are supported on ext4, writing back a sufficiently
>> large and discontinuous folio may consume a significant number of
>> journal credits, placing considerable strain on the journal. For
>> example, in a 20GB filesystem with 1K block size and 1MB journal size,
>> writing back a 2MB folio could require thousands of credits in the
>> worst-case scenario (when each block is discontinuous and distributed
>> across different block groups), potentially exceeding the journal size.
>> This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
>> when delalloc is not enabled.
>>
>> Fix this by ensuring that there are sufficient journal credits before
>> allocating an extent in mpage_map_one_extent() and
>> ext4_block_write_begin(). If there are not enough credits, return
>> -EAGAIN, exit the current mapping loop, restart a new handle and a new
>> transaction, and allocating blocks on this folio again in the next
>> iteration.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> Very nice. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> One small comment below:
> 
>> +/*
>> + * Make sure that the current journal transaction has enough credits to map
>> + * one extent. Return -EAGAIN if it cannot extend the current running
>> + * transaction.
>> + */
>> +static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
>> +						     struct inode *inode)
>> +{
>> +	int credits;
>> +	int ret;
>> +
>> +	if (!handle)
> 
> Shouldn't this rather be ext4_handle_valid(handle) to catch nojournal mode
> properly?
> 
__ext4_journal_ensure_credits() already calls ext4_handle_valid() to handle
nojournal mode, and the '!handle' check here is to handle the case where
ext4_block_write_begin() passes in a NULL 'handle'.

Thanks,
Yi.

Re: [PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Jan Kara]

On Thu 03-07-25 10:13:07, Zhang Yi wrote:
> On 2025/7/2 22:18, Jan Kara wrote:
> > On Tue 01-07-25 21:06:30, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@huawei.com>
> >>
> >> After large folios are supported on ext4, writing back a sufficiently
> >> large and discontinuous folio may consume a significant number of
> >> journal credits, placing considerable strain on the journal. For
> >> example, in a 20GB filesystem with 1K block size and 1MB journal size,
> >> writing back a 2MB folio could require thousands of credits in the
> >> worst-case scenario (when each block is discontinuous and distributed
> >> across different block groups), potentially exceeding the journal size.
> >> This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
> >> when delalloc is not enabled.
> >>
> >> Fix this by ensuring that there are sufficient journal credits before
> >> allocating an extent in mpage_map_one_extent() and
> >> ext4_block_write_begin(). If there are not enough credits, return
> >> -EAGAIN, exit the current mapping loop, restart a new handle and a new
> >> transaction, and allocating blocks on this folio again in the next
> >> iteration.
> >>
> >> Suggested-by: Jan Kara <jack@suse.cz>
> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> > 
> > Very nice. Feel free to add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > One small comment below:
> > 
> >> +/*
> >> + * Make sure that the current journal transaction has enough credits to map
> >> + * one extent. Return -EAGAIN if it cannot extend the current running
> >> + * transaction.
> >> + */
> >> +static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
> >> +						     struct inode *inode)
> >> +{
> >> +	int credits;
> >> +	int ret;
> >> +
> >> +	if (!handle)
> > 
> > Shouldn't this rather be ext4_handle_valid(handle) to catch nojournal mode
> > properly?
> > 
> __ext4_journal_ensure_credits() already calls ext4_handle_valid() to handle
> nojournal mode, and the '!handle' check here is to handle the case where
> ext4_block_write_begin() passes in a NULL 'handle'.

Ah, right. But then you don't need the test at all, do you? Anyway,
whatever you decide to do with this (or nothing) is fine by me.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Zhang Yi]

On 2025/7/4 0:27, Jan Kara wrote:
> On Thu 03-07-25 10:13:07, Zhang Yi wrote:
>> On 2025/7/2 22:18, Jan Kara wrote:
>>> On Tue 01-07-25 21:06:30, Zhang Yi wrote:
>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>
>>>> After large folios are supported on ext4, writing back a sufficiently
>>>> large and discontinuous folio may consume a significant number of
>>>> journal credits, placing considerable strain on the journal. For
>>>> example, in a 20GB filesystem with 1K block size and 1MB journal size,
>>>> writing back a 2MB folio could require thousands of credits in the
>>>> worst-case scenario (when each block is discontinuous and distributed
>>>> across different block groups), potentially exceeding the journal size.
>>>> This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
>>>> when delalloc is not enabled.
>>>>
>>>> Fix this by ensuring that there are sufficient journal credits before
>>>> allocating an extent in mpage_map_one_extent() and
>>>> ext4_block_write_begin(). If there are not enough credits, return
>>>> -EAGAIN, exit the current mapping loop, restart a new handle and a new
>>>> transaction, and allocating blocks on this folio again in the next
>>>> iteration.
>>>>
>>>> Suggested-by: Jan Kara <jack@suse.cz>
>>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> Very nice. Feel free to add:
>>>
>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>
>>> One small comment below:
>>>
>>>> +/*
>>>> + * Make sure that the current journal transaction has enough credits to map
>>>> + * one extent. Return -EAGAIN if it cannot extend the current running
>>>> + * transaction.
>>>> + */
>>>> +static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
>>>> +						     struct inode *inode)
>>>> +{
>>>> +	int credits;
>>>> +	int ret;
>>>> +
>>>> +	if (!handle)
>>>
>>> Shouldn't this rather be ext4_handle_valid(handle) to catch nojournal mode
>>> properly?
>>>
>> __ext4_journal_ensure_credits() already calls ext4_handle_valid() to handle
>> nojournal mode, and the '!handle' check here is to handle the case where
>> ext4_block_write_begin() passes in a NULL 'handle'.
> 
> Ah, right. But then you don't need the test at all, do you? Anyway,
> whatever you decide to do with this (or nothing) is fine by me.
> 

Yeah, remove this test is fine with me. I added this one is because the
comments in ext4_handle_valid() said "Do not use this for NULL handles."
I think it is best to follow this rule. :)

Best regards,
Yi.

Re: [PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Jan Kara]

On Fri 04-07-25 09:40:23, Zhang Yi wrote:
> On 2025/7/4 0:27, Jan Kara wrote:
> > On Thu 03-07-25 10:13:07, Zhang Yi wrote:
> >> On 2025/7/2 22:18, Jan Kara wrote:
> >>> On Tue 01-07-25 21:06:30, Zhang Yi wrote:
> >>>> From: Zhang Yi <yi.zhang@huawei.com>
> >>>>
> >>>> After large folios are supported on ext4, writing back a sufficiently
> >>>> large and discontinuous folio may consume a significant number of
> >>>> journal credits, placing considerable strain on the journal. For
> >>>> example, in a 20GB filesystem with 1K block size and 1MB journal size,
> >>>> writing back a 2MB folio could require thousands of credits in the
> >>>> worst-case scenario (when each block is discontinuous and distributed
> >>>> across different block groups), potentially exceeding the journal size.
> >>>> This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
> >>>> when delalloc is not enabled.
> >>>>
> >>>> Fix this by ensuring that there are sufficient journal credits before
> >>>> allocating an extent in mpage_map_one_extent() and
> >>>> ext4_block_write_begin(). If there are not enough credits, return
> >>>> -EAGAIN, exit the current mapping loop, restart a new handle and a new
> >>>> transaction, and allocating blocks on this folio again in the next
> >>>> iteration.
> >>>>
> >>>> Suggested-by: Jan Kara <jack@suse.cz>
> >>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> >>>
> >>> Very nice. Feel free to add:
> >>>
> >>> Reviewed-by: Jan Kara <jack@suse.cz>
> >>>
> >>> One small comment below:
> >>>
> >>>> +/*
> >>>> + * Make sure that the current journal transaction has enough credits to map
> >>>> + * one extent. Return -EAGAIN if it cannot extend the current running
> >>>> + * transaction.
> >>>> + */
> >>>> +static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
> >>>> +						     struct inode *inode)
> >>>> +{
> >>>> +	int credits;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (!handle)
> >>>
> >>> Shouldn't this rather be ext4_handle_valid(handle) to catch nojournal mode
> >>> properly?
> >>>
> >> __ext4_journal_ensure_credits() already calls ext4_handle_valid() to handle
> >> nojournal mode, and the '!handle' check here is to handle the case where
> >> ext4_block_write_begin() passes in a NULL 'handle'.
> > 
> > Ah, right. But then you don't need the test at all, do you? Anyway,
> > whatever you decide to do with this (or nothing) is fine by me.
> > 
> 
> Yeah, remove this test is fine with me. I added this one is because the
> comments in ext4_handle_valid() said "Do not use this for NULL handles."
> I think it is best to follow this rule. :)

Right, I didn't read that comment :) So maybe the best fix will be just
adding a comment before the test like:

	/* Called from ext4_da_write_begin() which has no handle started? */
	if (!handle)
		return 0;

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 05/10] ext4: restart handle if credits are insufficient during allocating blocks

[Zhang Yi]

On 2025/7/4 16:18, Jan Kara wrote:
> On Fri 04-07-25 09:40:23, Zhang Yi wrote:
>> On 2025/7/4 0:27, Jan Kara wrote:
>>> On Thu 03-07-25 10:13:07, Zhang Yi wrote:
>>>> On 2025/7/2 22:18, Jan Kara wrote:
>>>>> On Tue 01-07-25 21:06:30, Zhang Yi wrote:
>>>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>>>
>>>>>> After large folios are supported on ext4, writing back a sufficiently
>>>>>> large and discontinuous folio may consume a significant number of
>>>>>> journal credits, placing considerable strain on the journal. For
>>>>>> example, in a 20GB filesystem with 1K block size and 1MB journal size,
>>>>>> writing back a 2MB folio could require thousands of credits in the
>>>>>> worst-case scenario (when each block is discontinuous and distributed
>>>>>> across different block groups), potentially exceeding the journal size.
>>>>>> This issue can also occur in ext4_write_begin() and ext4_page_mkwrite()
>>>>>> when delalloc is not enabled.
>>>>>>
>>>>>> Fix this by ensuring that there are sufficient journal credits before
>>>>>> allocating an extent in mpage_map_one_extent() and
>>>>>> ext4_block_write_begin(). If there are not enough credits, return
>>>>>> -EAGAIN, exit the current mapping loop, restart a new handle and a new
>>>>>> transaction, and allocating blocks on this folio again in the next
>>>>>> iteration.
>>>>>>
>>>>>> Suggested-by: Jan Kara <jack@suse.cz>
>>>>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>>>>
>>>>> Very nice. Feel free to add:
>>>>>
>>>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>>>
>>>>> One small comment below:
>>>>>
>>>>>> +/*
>>>>>> + * Make sure that the current journal transaction has enough credits to map
>>>>>> + * one extent. Return -EAGAIN if it cannot extend the current running
>>>>>> + * transaction.
>>>>>> + */
>>>>>> +static inline int ext4_journal_ensure_extent_credits(handle_t *handle,
>>>>>> +						     struct inode *inode)
>>>>>> +{
>>>>>> +	int credits;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (!handle)
>>>>>
>>>>> Shouldn't this rather be ext4_handle_valid(handle) to catch nojournal mode
>>>>> properly?
>>>>>
>>>> __ext4_journal_ensure_credits() already calls ext4_handle_valid() to handle
>>>> nojournal mode, and the '!handle' check here is to handle the case where
>>>> ext4_block_write_begin() passes in a NULL 'handle'.
>>>
>>> Ah, right. But then you don't need the test at all, do you? Anyway,
>>> whatever you decide to do with this (or nothing) is fine by me.
>>>
>>
>> Yeah, remove this test is fine with me. I added this one is because the
>> comments in ext4_handle_valid() said "Do not use this for NULL handles."
>> I think it is best to follow this rule. :)
> 
> Right, I didn't read that comment :) So maybe the best fix will be just
> adding a comment before the test like:
> 
> 	/* Called from ext4_da_write_begin() which has no handle started? */
> 	if (!handle)
> 		return 0;
> 

Sure, it looks good, will do.

Thanks,
Yi.

[PATCH v3 06/10] ext4: enhance tracepoints during the folios writeback

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

After mpage_map_and_submit_extent() supports restarting handle if
credits are insufficient during allocating blocks, it is more likely to
exit the current mapping iteration and continue to process the current
processing partially mapped folio again. The existing tracepoints are
not sufficient to track this situation, so enhance the tracepoints to
track the writeback position and the return value before and after
submitting the folios.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c             |  5 ++++-
 include/trace/events/ext4.h | 42 ++++++++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index efe778aaf74b..79389874d35f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2934,7 +2934,8 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		}
 		mpd->do_map = 1;
 
-		trace_ext4_da_write_pages(inode, mpd->start_pos, wbc);
+		trace_ext4_da_write_folios_start(inode, mpd->start_pos,
+				mpd->next_pos, wbc);
 		ret = mpage_prepare_extent_to_map(mpd);
 		if (!ret && mpd->map.m_len)
 			ret = mpage_map_and_submit_extent(handle, mpd,
@@ -2972,6 +2973,8 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		} else
 			ext4_put_io_end(mpd->io_submit.io_end);
 		mpd->io_submit.io_end = NULL;
+		trace_ext4_da_write_folios_end(inode, mpd->start_pos,
+				mpd->next_pos, wbc, ret);
 
 		if (ret == -ENOSPC && sbi->s_journal) {
 			/*
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 62d52997b5c6..845451077c41 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -482,16 +482,17 @@ TRACE_EVENT(ext4_writepages,
 		  (unsigned long) __entry->writeback_index)
 );
 
-TRACE_EVENT(ext4_da_write_pages,
-	TP_PROTO(struct inode *inode, loff_t start_pos,
+TRACE_EVENT(ext4_da_write_folios_start,
+	TP_PROTO(struct inode *inode, loff_t start_pos, loff_t next_pos,
 		 struct writeback_control *wbc),
 
-	TP_ARGS(inode, start_pos, wbc),
+	TP_ARGS(inode, start_pos, next_pos, wbc),
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)
 		__field(	ino_t,	ino			)
 		__field(       loff_t,	start_pos		)
+		__field(       loff_t,	next_pos		)
 		__field(	 long,	nr_to_write		)
 		__field(	  int,	sync_mode		)
 	),
@@ -500,16 +501,47 @@ TRACE_EVENT(ext4_da_write_pages,
 		__entry->dev		= inode->i_sb->s_dev;
 		__entry->ino		= inode->i_ino;
 		__entry->start_pos	= start_pos;
+		__entry->next_pos	= next_pos;
 		__entry->nr_to_write	= wbc->nr_to_write;
 		__entry->sync_mode	= wbc->sync_mode;
 	),
 
-	TP_printk("dev %d,%d ino %lu start_pos 0x%llx nr_to_write %ld sync_mode %d",
+	TP_printk("dev %d,%d ino %lu start_pos 0x%llx next_pos 0x%llx nr_to_write %ld sync_mode %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
-		  (unsigned long) __entry->ino, __entry->start_pos,
+		  (unsigned long) __entry->ino, __entry->start_pos, __entry->next_pos,
 		  __entry->nr_to_write, __entry->sync_mode)
 );
 
+TRACE_EVENT(ext4_da_write_folios_end,
+	TP_PROTO(struct inode *inode, loff_t start_pos, loff_t next_pos,
+		 struct writeback_control *wbc, int ret),
+
+	TP_ARGS(inode, start_pos, next_pos, wbc, ret),
+
+	TP_STRUCT__entry(
+		__field(	dev_t,	dev			)
+		__field(	ino_t,	ino			)
+		__field(       loff_t,	start_pos		)
+		__field(       loff_t,	next_pos		)
+		__field(	 long,	nr_to_write		)
+		__field(	  int,	ret			)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->start_pos	= start_pos;
+		__entry->next_pos	= next_pos;
+		__entry->nr_to_write	= wbc->nr_to_write;
+		__entry->ret		= ret;
+	),
+
+	TP_printk("dev %d,%d ino %lu start_pos 0x%llx next_pos 0x%llx nr_to_write %ld ret %d",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  (unsigned long) __entry->ino, __entry->start_pos, __entry->next_pos,
+		  __entry->nr_to_write, __entry->ret)
+);
+
 TRACE_EVENT(ext4_da_write_pages_extent,
 	TP_PROTO(struct inode *inode, struct ext4_map_blocks *map),
 
-- 
2.46.1

Re: [PATCH v3 06/10] ext4: enhance tracepoints during the folios writeback

[Jan Kara]

On Tue 01-07-25 21:06:31, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> After mpage_map_and_submit_extent() supports restarting handle if
> credits are insufficient during allocating blocks, it is more likely to
> exit the current mapping iteration and continue to process the current
> processing partially mapped folio again. The existing tracepoints are
> not sufficient to track this situation, so enhance the tracepoints to
> track the writeback position and the return value before and after
> submitting the folios.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 07/10] ext4: correct the reserved credits for extent conversion

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Now, we reserve journal credits for converting extents in only one page
to written state when the I/O operation is complete. This is
insufficient when large folio is enabled.

Fix this by reserving credits for converting up to one extent per block in
the largest 2MB folio, this calculation should only involve extents index
and leaf blocks, so it should not estimate too many credits.

Fixes: 7ac67301e82f ("ext4: enable large folio for regular file")
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 79389874d35f..3230734a3014 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2848,12 +2848,12 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	mpd->journalled_more_data = 0;
 
 	if (ext4_should_dioread_nolock(inode)) {
+		int bpf = ext4_journal_blocks_per_folio(inode);
 		/*
 		 * We may need to convert up to one extent per block in
-		 * the page and we may dirty the inode.
+		 * the folio and we may dirty the inode.
 		 */
-		rsv_blocks = 1 + ext4_chunk_trans_blocks(inode,
-						PAGE_SIZE >> inode->i_blkbits);
+		rsv_blocks = 1 + ext4_ext_index_trans_blocks(inode, bpf);
 	}
 
 	if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX)
-- 
2.46.1

[PATCH v3 08/10] ext4: reserved credits for one extent during the folio writeback

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

After ext4 supports large folios, reserving journal credits for one
maximum-ordered folio based on the worst case cenario during the
writeback process can easily exceed the maximum transaction credits.
Additionally, reserving journal credits for one page is also no
longer appropriate.

Currently, the folio writeback process can either extend the journal
credits or initiate a new transaction if the currently reserved journal
credits are insufficient. Therefore, it can be modified to reserve
credits for only one extent at the outset. In most cases involving
continuous mapping, these credits are generally adequate, and we may
only need to perform some basic credit expansion. However, in extreme
cases where the block size and folio size differ significantly, or when
the folios are sufficiently discontinuous, it may be necessary to
restart a new transaction and resubmit the folios.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3230734a3014..ceaede80d791 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2546,21 +2546,6 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 	return err;
 }
 
-/*
- * Calculate the total number of credits to reserve for one writepages
- * iteration. This is called from ext4_writepages(). We map an extent of
- * up to MAX_WRITEPAGES_EXTENT_LEN blocks and then we go on and finish mapping
- * the last partial page. So in total we can map MAX_WRITEPAGES_EXTENT_LEN +
- * bpp - 1 blocks in bpp different extents.
- */
-static int ext4_da_writepages_trans_blocks(struct inode *inode)
-{
-	int bpp = ext4_journal_blocks_per_folio(inode);
-
-	return ext4_meta_trans_blocks(inode,
-				MAX_WRITEPAGES_EXTENT_LEN + bpp - 1, bpp);
-}
-
 static int ext4_journal_folio_buffers(handle_t *handle, struct folio *folio,
 				     size_t len)
 {
@@ -2917,8 +2902,14 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		 * not supported by delalloc.
 		 */
 		BUG_ON(ext4_should_journal_data(inode));
-		needed_blocks = ext4_da_writepages_trans_blocks(inode);
-
+		/*
+		 * Calculate the number of credits needed to reserve for one
+		 * extent of up to MAX_WRITEPAGES_EXTENT_LEN blocks. It will
+		 * attempt to extend the transaction or start a new iteration
+		 * if the reserved credits are insufficient.
+		 */
+		needed_blocks = ext4_chunk_trans_blocks(inode,
+						MAX_WRITEPAGES_EXTENT_LEN);
 		/* start a new transaction */
 		handle = ext4_journal_start_with_reserve(inode,
 				EXT4_HT_WRITE_PAGE, needed_blocks, rsv_blocks);
-- 
2.46.1

Re: [PATCH v3 08/10] ext4: reserved credits for one extent during the folio writeback

[Jan Kara]

On Tue 01-07-25 21:06:33, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> After ext4 supports large folios, reserving journal credits for one
> maximum-ordered folio based on the worst case cenario during the
> writeback process can easily exceed the maximum transaction credits.
> Additionally, reserving journal credits for one page is also no
> longer appropriate.
> 
> Currently, the folio writeback process can either extend the journal
> credits or initiate a new transaction if the currently reserved journal
> credits are insufficient. Therefore, it can be modified to reserve
> credits for only one extent at the outset. In most cases involving
> continuous mapping, these credits are generally adequate, and we may
> only need to perform some basic credit expansion. However, in extreme
> cases where the block size and folio size differ significantly, or when
> the folios are sufficiently discontinuous, it may be necessary to
> restart a new transaction and resubmit the folios.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/inode.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3230734a3014..ceaede80d791 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2546,21 +2546,6 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  	return err;
>  }
>  
> -/*
> - * Calculate the total number of credits to reserve for one writepages
> - * iteration. This is called from ext4_writepages(). We map an extent of
> - * up to MAX_WRITEPAGES_EXTENT_LEN blocks and then we go on and finish mapping
> - * the last partial page. So in total we can map MAX_WRITEPAGES_EXTENT_LEN +
> - * bpp - 1 blocks in bpp different extents.
> - */
> -static int ext4_da_writepages_trans_blocks(struct inode *inode)
> -{
> -	int bpp = ext4_journal_blocks_per_folio(inode);
> -
> -	return ext4_meta_trans_blocks(inode,
> -				MAX_WRITEPAGES_EXTENT_LEN + bpp - 1, bpp);
> -}
> -
>  static int ext4_journal_folio_buffers(handle_t *handle, struct folio *folio,
>  				     size_t len)
>  {
> @@ -2917,8 +2902,14 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>  		 * not supported by delalloc.
>  		 */
>  		BUG_ON(ext4_should_journal_data(inode));
> -		needed_blocks = ext4_da_writepages_trans_blocks(inode);
> -
> +		/*
> +		 * Calculate the number of credits needed to reserve for one
> +		 * extent of up to MAX_WRITEPAGES_EXTENT_LEN blocks. It will
> +		 * attempt to extend the transaction or start a new iteration
> +		 * if the reserved credits are insufficient.
> +		 */
> +		needed_blocks = ext4_chunk_trans_blocks(inode,
> +						MAX_WRITEPAGES_EXTENT_LEN);
>  		/* start a new transaction */
>  		handle = ext4_journal_start_with_reserve(inode,
>  				EXT4_HT_WRITE_PAGE, needed_blocks, rsv_blocks);
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 09/10] ext4: replace ext4_writepage_trans_blocks()

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

After ext4 supports large folios, the semantics of reserving credits in
pages is no longer applicable. In most scenarios, reserving credits in
extents is sufficient. Therefore, introduce ext4_chunk_trans_extent()
to replace ext4_writepage_trans_blocks(). move_extent_per_page() is the
only remaining location where we are still processing extents in pages.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h        |  2 +-
 fs/ext4/extents.c     |  6 +++---
 fs/ext4/inline.c      |  6 +++---
 fs/ext4/inode.c       | 33 +++++++++++++++------------------
 fs/ext4/move_extent.c |  3 ++-
 fs/ext4/xattr.c       |  2 +-
 6 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 18373de980f2..f705046ba6c6 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3064,9 +3064,9 @@ extern int ext4_punch_hole(struct file *file, loff_t offset, loff_t length);
 extern void ext4_set_inode_flags(struct inode *, bool init);
 extern int ext4_alloc_da_blocks(struct inode *inode);
 extern void ext4_set_aops(struct inode *inode);
-extern int ext4_writepage_trans_blocks(struct inode *);
 extern int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode);
 extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
+extern int ext4_chunk_trans_extent(struct inode *inode, int nrblocks);
 extern int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
 				  int pextents);
 extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index b543a46fc809..f0f155458697 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5171,7 +5171,7 @@ ext4_ext_shift_path_extents(struct ext4_ext_path *path, ext4_lblk_t shift,
 				credits = depth + 2;
 			}
 
-			restart_credits = ext4_writepage_trans_blocks(inode);
+			restart_credits = ext4_chunk_trans_extent(inode, 0);
 			err = ext4_datasem_ensure_credits(handle, inode, credits,
 					restart_credits, 0);
 			if (err) {
@@ -5431,7 +5431,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
 
 	truncate_pagecache(inode, start);
 
-	credits = ext4_writepage_trans_blocks(inode);
+	credits = ext4_chunk_trans_extent(inode, 0);
 	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
@@ -5527,7 +5527,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
 
 	truncate_pagecache(inode, start);
 
-	credits = ext4_writepage_trans_blocks(inode);
+	credits = ext4_chunk_trans_extent(inode, 0);
 	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index a1bbcdf40824..d5b32d242495 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -562,7 +562,7 @@ static int ext4_convert_inline_data_to_extent(struct address_space *mapping,
 		return 0;
 	}
 
-	needed_blocks = ext4_writepage_trans_blocks(inode);
+	needed_blocks = ext4_chunk_trans_extent(inode, 1);
 
 	ret = ext4_get_inode_loc(inode, &iloc);
 	if (ret)
@@ -1864,7 +1864,7 @@ int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
 	};
 
 
-	needed_blocks = ext4_writepage_trans_blocks(inode);
+	needed_blocks = ext4_chunk_trans_extent(inode, 1);
 	handle = ext4_journal_start(inode, EXT4_HT_INODE, needed_blocks);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
@@ -1979,7 +1979,7 @@ int ext4_convert_inline_data(struct inode *inode)
 			return 0;
 	}
 
-	needed_blocks = ext4_writepage_trans_blocks(inode);
+	needed_blocks = ext4_chunk_trans_extent(inode, 1);
 
 	iloc.bh = NULL;
 	error = ext4_get_inode_loc(inode, &iloc);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ceaede80d791..572a70b6a934 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1295,7 +1295,8 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 	 * Reserve one block more for addition to orphan list in case
 	 * we allocate blocks but write fails for some reason
 	 */
-	needed_blocks = ext4_writepage_trans_blocks(inode) + 1;
+	needed_blocks = ext4_chunk_trans_extent(inode,
+			ext4_journal_blocks_per_folio(inode)) + 1;
 	index = pos >> PAGE_SHIFT;
 
 	if (ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
@@ -4462,7 +4463,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 		return ret;
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		credits = ext4_writepage_trans_blocks(inode);
+		credits = ext4_chunk_trans_extent(inode, 2);
 	else
 		credits = ext4_blocks_for_truncate(inode);
 	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
@@ -4611,7 +4612,7 @@ int ext4_truncate(struct inode *inode)
 	}
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		credits = ext4_writepage_trans_blocks(inode);
+		credits = ext4_chunk_trans_extent(inode, 1);
 	else
 		credits = ext4_blocks_for_truncate(inode);
 
@@ -6238,25 +6239,19 @@ int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents)
 }
 
 /*
- * Calculate the total number of credits to reserve to fit
- * the modification of a single pages into a single transaction,
- * which may include multiple chunks of block allocations.
- *
- * This could be called via ext4_write_begin()
- *
- * We need to consider the worse case, when
- * one new block per extent.
+ * Calculate the journal credits for modifying the number of blocks
+ * in a single extent within one transaction. 'nrblocks' is used only
+ * for non-extent inodes. For extent type inodes, 'nrblocks' can be
+ * zero if the exact number of blocks is unknown.
  */
-int ext4_writepage_trans_blocks(struct inode *inode)
+int ext4_chunk_trans_extent(struct inode *inode, int nrblocks)
 {
-	int bpp = ext4_journal_blocks_per_folio(inode);
 	int ret;
 
-	ret = ext4_meta_trans_blocks(inode, bpp, bpp);
-
+	ret = ext4_meta_trans_blocks(inode, nrblocks, 1);
 	/* Account for data blocks for journalled mode */
 	if (ext4_should_journal_data(inode))
-		ret += bpp;
+		ret += nrblocks;
 	return ret;
 }
 
@@ -6634,10 +6629,12 @@ static int ext4_block_page_mkwrite(struct inode *inode, struct folio *folio,
 	handle_t *handle;
 	loff_t size;
 	unsigned long len;
+	int credits;
 	int ret;
 
-	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
-				    ext4_writepage_trans_blocks(inode));
+	credits = ext4_chunk_trans_extent(inode,
+			ext4_journal_blocks_per_folio(inode));
+	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, credits);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 1f8493a56e8f..adae3caf175a 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -280,7 +280,8 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
 	 */
 again:
 	*err = 0;
-	jblocks = ext4_writepage_trans_blocks(orig_inode) * 2;
+	jblocks = ext4_meta_trans_blocks(orig_inode, block_len_in_page,
+					 block_len_in_page) * 2;
 	handle = ext4_journal_start(orig_inode, EXT4_HT_MOVE_EXTENTS, jblocks);
 	if (IS_ERR(handle)) {
 		*err = PTR_ERR(handle);
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 8d15acbacc20..3fb93247330d 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -962,7 +962,7 @@ int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
 	 * so we need to reserve credits for this eventuality
 	 */
 	if (inode && ext4_has_inline_data(inode))
-		credits += ext4_writepage_trans_blocks(inode) + 1;
+		credits += ext4_chunk_trans_extent(inode, 1) + 1;
 
 	/* We are done if ea_inode feature is not enabled. */
 	if (!ext4_has_feature_ea_inode(sb))
-- 
2.46.1

Re: [PATCH v3 09/10] ext4: replace ext4_writepage_trans_blocks()

[Jan Kara]

On Tue 01-07-25 21:06:34, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> After ext4 supports large folios, the semantics of reserving credits in
> pages is no longer applicable. In most scenarios, reserving credits in
> extents is sufficient. Therefore, introduce ext4_chunk_trans_extent()
> to replace ext4_writepage_trans_blocks(). move_extent_per_page() is the
> only remaining location where we are still processing extents in pages.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ext4.h        |  2 +-
>  fs/ext4/extents.c     |  6 +++---
>  fs/ext4/inline.c      |  6 +++---
>  fs/ext4/inode.c       | 33 +++++++++++++++------------------
>  fs/ext4/move_extent.c |  3 ++-
>  fs/ext4/xattr.c       |  2 +-
>  6 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 18373de980f2..f705046ba6c6 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3064,9 +3064,9 @@ extern int ext4_punch_hole(struct file *file, loff_t offset, loff_t length);
>  extern void ext4_set_inode_flags(struct inode *, bool init);
>  extern int ext4_alloc_da_blocks(struct inode *inode);
>  extern void ext4_set_aops(struct inode *inode);
> -extern int ext4_writepage_trans_blocks(struct inode *);
>  extern int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode);
>  extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
> +extern int ext4_chunk_trans_extent(struct inode *inode, int nrblocks);
>  extern int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
>  				  int pextents);
>  extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index b543a46fc809..f0f155458697 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5171,7 +5171,7 @@ ext4_ext_shift_path_extents(struct ext4_ext_path *path, ext4_lblk_t shift,
>  				credits = depth + 2;
>  			}
>  
> -			restart_credits = ext4_writepage_trans_blocks(inode);
> +			restart_credits = ext4_chunk_trans_extent(inode, 0);
>  			err = ext4_datasem_ensure_credits(handle, inode, credits,
>  					restart_credits, 0);
>  			if (err) {
> @@ -5431,7 +5431,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
>  
>  	truncate_pagecache(inode, start);
>  
> -	credits = ext4_writepage_trans_blocks(inode);
> +	credits = ext4_chunk_trans_extent(inode, 0);
>  	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
> @@ -5527,7 +5527,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
>  
>  	truncate_pagecache(inode, start);
>  
> -	credits = ext4_writepage_trans_blocks(inode);
> +	credits = ext4_chunk_trans_extent(inode, 0);
>  	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index a1bbcdf40824..d5b32d242495 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -562,7 +562,7 @@ static int ext4_convert_inline_data_to_extent(struct address_space *mapping,
>  		return 0;
>  	}
>  
> -	needed_blocks = ext4_writepage_trans_blocks(inode);
> +	needed_blocks = ext4_chunk_trans_extent(inode, 1);
>  
>  	ret = ext4_get_inode_loc(inode, &iloc);
>  	if (ret)
> @@ -1864,7 +1864,7 @@ int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
>  	};
>  
>  
> -	needed_blocks = ext4_writepage_trans_blocks(inode);
> +	needed_blocks = ext4_chunk_trans_extent(inode, 1);
>  	handle = ext4_journal_start(inode, EXT4_HT_INODE, needed_blocks);
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
> @@ -1979,7 +1979,7 @@ int ext4_convert_inline_data(struct inode *inode)
>  			return 0;
>  	}
>  
> -	needed_blocks = ext4_writepage_trans_blocks(inode);
> +	needed_blocks = ext4_chunk_trans_extent(inode, 1);
>  
>  	iloc.bh = NULL;
>  	error = ext4_get_inode_loc(inode, &iloc);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ceaede80d791..572a70b6a934 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1295,7 +1295,8 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>  	 * Reserve one block more for addition to orphan list in case
>  	 * we allocate blocks but write fails for some reason
>  	 */
> -	needed_blocks = ext4_writepage_trans_blocks(inode) + 1;
> +	needed_blocks = ext4_chunk_trans_extent(inode,
> +			ext4_journal_blocks_per_folio(inode)) + 1;
>  	index = pos >> PAGE_SHIFT;
>  
>  	if (ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
> @@ -4462,7 +4463,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>  		return ret;
>  
>  	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> -		credits = ext4_writepage_trans_blocks(inode);
> +		credits = ext4_chunk_trans_extent(inode, 2);
>  	else
>  		credits = ext4_blocks_for_truncate(inode);
>  	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
> @@ -4611,7 +4612,7 @@ int ext4_truncate(struct inode *inode)
>  	}
>  
>  	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> -		credits = ext4_writepage_trans_blocks(inode);
> +		credits = ext4_chunk_trans_extent(inode, 1);
>  	else
>  		credits = ext4_blocks_for_truncate(inode);
>  
> @@ -6238,25 +6239,19 @@ int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents)
>  }
>  
>  /*
> - * Calculate the total number of credits to reserve to fit
> - * the modification of a single pages into a single transaction,
> - * which may include multiple chunks of block allocations.
> - *
> - * This could be called via ext4_write_begin()
> - *
> - * We need to consider the worse case, when
> - * one new block per extent.
> + * Calculate the journal credits for modifying the number of blocks
> + * in a single extent within one transaction. 'nrblocks' is used only
> + * for non-extent inodes. For extent type inodes, 'nrblocks' can be
> + * zero if the exact number of blocks is unknown.
>   */
> -int ext4_writepage_trans_blocks(struct inode *inode)
> +int ext4_chunk_trans_extent(struct inode *inode, int nrblocks)
>  {
> -	int bpp = ext4_journal_blocks_per_folio(inode);
>  	int ret;
>  
> -	ret = ext4_meta_trans_blocks(inode, bpp, bpp);
> -
> +	ret = ext4_meta_trans_blocks(inode, nrblocks, 1);
>  	/* Account for data blocks for journalled mode */
>  	if (ext4_should_journal_data(inode))
> -		ret += bpp;
> +		ret += nrblocks;
>  	return ret;
>  }
>  
> @@ -6634,10 +6629,12 @@ static int ext4_block_page_mkwrite(struct inode *inode, struct folio *folio,
>  	handle_t *handle;
>  	loff_t size;
>  	unsigned long len;
> +	int credits;
>  	int ret;
>  
> -	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
> -				    ext4_writepage_trans_blocks(inode));
> +	credits = ext4_chunk_trans_extent(inode,
> +			ext4_journal_blocks_per_folio(inode));
> +	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, credits);
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
>  
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 1f8493a56e8f..adae3caf175a 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -280,7 +280,8 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	 */
>  again:
>  	*err = 0;
> -	jblocks = ext4_writepage_trans_blocks(orig_inode) * 2;
> +	jblocks = ext4_meta_trans_blocks(orig_inode, block_len_in_page,
> +					 block_len_in_page) * 2;
>  	handle = ext4_journal_start(orig_inode, EXT4_HT_MOVE_EXTENTS, jblocks);
>  	if (IS_ERR(handle)) {
>  		*err = PTR_ERR(handle);
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 8d15acbacc20..3fb93247330d 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -962,7 +962,7 @@ int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
>  	 * so we need to reserve credits for this eventuality
>  	 */
>  	if (inode && ext4_has_inline_data(inode))
> -		credits += ext4_writepage_trans_blocks(inode) + 1;
> +		credits += ext4_chunk_trans_extent(inode, 1) + 1;
>  
>  	/* We are done if ea_inode feature is not enabled. */
>  	if (!ext4_has_feature_ea_inode(sb))
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 10/10] ext4: fix insufficient credits calculation in ext4_meta_trans_blocks()

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

The calculation of journal credits in ext4_meta_trans_blocks() should
include pextents, as each extent separately may be allocated from a
different group and thus need to update different bitmap and group
descriptor block.

Fixes: 0e32d8617012 ("ext4: correct the journal credits calculations of allocating blocks")
Reported-by: Jan Kara <jack@suse.cz>
Closes: https://lore.kernel.org/linux-ext4/nhxfuu53wyacsrq7xqgxvgzcggyscu2tbabginahcygvmc45hy@t4fvmyeky33e/
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 572a70b6a934..a75279cceec4 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6213,7 +6213,7 @@ int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents)
 	int ret;
 
 	/*
-	 * How many index and lead blocks need to touch to map @lblocks
+	 * How many index and leaf blocks need to touch to map @lblocks
 	 * logical blocks to @pextents physical extents?
 	 */
 	idxblocks = ext4_index_trans_blocks(inode, lblocks, pextents);
@@ -6222,7 +6222,7 @@ int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents)
 	 * Now let's see how many group bitmaps and group descriptors need
 	 * to account
 	 */
-	groups = idxblocks;
+	groups = idxblocks + pextents;
 	gdpblocks = groups;
 	if (groups > ngroups)
 		groups = ngroups;
-- 
2.46.1