* Re: [PATCH v4 02/23] ext4: factor out ext4_truncate_[up|down]()
From: Ojaswin Mujoo @ 2026-05-19 6:05 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <20260511072344.191271-3-yi.zhang@huaweicloud.com>
On Mon, May 11, 2026 at 03:23:22PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> Refactor ext4_setattr() by introducing two helper functions,
> ext4_truncate_up() and ext4_truncate_down(), to handle size changes. The
> current ATTR_SIZE processing consolidates checks for both shrinking and
> non-shrinking cases, leading to cluttered code. Separating the
> truncation paths improves readability.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Looks good to me Zhang:
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
> fs/ext4/inode.c | 199 +++++++++++++++++++++++++++---------------------
> 1 file changed, 112 insertions(+), 87 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 0751dc55e94f..35e958f89bd5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5855,6 +5855,112 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
> }
> }
>
> +/*
> + * Set i_size and i_disksize to 'newsize'.
> + *
> + * Both i_rwsem and i_data_sem are required here to avoid races between
> + * generic append writeback and concurrent truncate that also modify
> + * i_size and i_disksize.
> + */
> +static inline void ext4_set_inode_size(struct inode *inode, loff_t newsize)
> +{
> + WARN_ON_ONCE(S_ISREG(inode->i_mode) && !inode_is_locked(inode));
> +
> + down_write(&EXT4_I(inode)->i_data_sem);
> + i_size_write(inode, newsize);
> + EXT4_I(inode)->i_disksize = newsize;
> + up_write(&EXT4_I(inode)->i_data_sem);
> +}
> +
> +static int ext4_truncate_up(struct inode *inode, loff_t oldsize, loff_t newsize)
> +{
> + ext4_lblk_t old_lblk, new_lblk;
> + handle_t *handle;
> + int ret;
> +
> + if (!IS_ALIGNED(oldsize | newsize, i_blocksize(inode))) {
> + ret = ext4_inode_attach_jinode(inode);
> + if (ret)
> + return ret;
> + }
> +
> + inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
> + if (!IS_ALIGNED(oldsize, i_blocksize(inode))) {
> + ret = ext4_block_zero_eof(inode, oldsize, LLONG_MAX);
> + if (ret)
> + return ret;
> + }
> +
> + handle = ext4_journal_start(inode, EXT4_HT_INODE, 3);
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> +
> + old_lblk = oldsize > 0 ? (oldsize - 1) >> inode->i_blkbits : 0;
> + new_lblk = newsize > 0 ? (newsize - 1) >> inode->i_blkbits : 0;
> + ext4_fc_track_range(handle, inode, old_lblk, new_lblk);
> +
> + ext4_set_inode_size(inode, newsize);
> +
> + ret = ext4_mark_inode_dirty(handle, inode);
> + ext4_journal_stop(handle);
> + if (ret)
> + return ret;
> + /*
> + * isize extend must be called outside an active handle due to
> + * the lock ordering of transaction start and folio lock in the
> + * iomap buffered I/O path (folio lock -> transaction start).
> + */
> + pagecache_isize_extended(inode, oldsize, newsize);
> + return 0;
> +}
> +
> +static int ext4_truncate_down(struct inode *inode, loff_t oldsize,
> + loff_t newsize, int *orphan)
> +{
> + ext4_lblk_t start_lblk;
> + handle_t *handle;
> + int ret;
> +
> + /* Do not change i_size. */
> + if (newsize == oldsize)
> + goto truncate;
> +
> + /* Shrink. */
> + handle = ext4_journal_start(inode, EXT4_HT_INODE, 3);
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> +
> + if (ext4_handle_valid(handle)) {
> + ret = ext4_orphan_add(handle, inode);
> + *orphan = 1;
> + if (ret) {
> + ext4_journal_stop(handle);
> + return ret;
> + }
> + }
> +
> + start_lblk = newsize > 0 ? (newsize - 1) >> inode->i_blkbits : 0;
> + ext4_fc_track_range(handle, inode, start_lblk, EXT_MAX_BLOCKS - 1);
> +
> + ext4_set_inode_size(inode, newsize);
> +
> + ret = ext4_mark_inode_dirty(handle, inode);
> + ext4_journal_stop(handle);
> + if (ret)
> + return ret;
> +
> + if (ext4_should_journal_data(inode))
> + ext4_wait_for_tail_page_commit(inode);
> +truncate:
> + /*
> + * Truncate pagecache after we've waited for commit in data=journal
> + * mode to make pages freeable. Call ext4_truncate() even if
> + * i_size didn't change to truncatea possible preallocated blocks.
> + */
> + truncate_pagecache(inode, newsize);
> + return ext4_truncate(inode);
> +}
> +
> /*
> * ext4_setattr()
> *
> @@ -5951,7 +6057,6 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> }
>
> if (attr->ia_valid & ATTR_SIZE) {
> - handle_t *handle;
> loff_t oldsize = inode->i_size;
> int shrink = (attr->ia_size < inode->i_size);
>
> @@ -6003,94 +6108,14 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> goto err_out;
> }
>
> - if (attr->ia_size != inode->i_size) {
> - /* attach jbd2 jinode for EOF folio tail zeroing */
> - if (attr->ia_size & (inode->i_sb->s_blocksize - 1) ||
> - oldsize & (inode->i_sb->s_blocksize - 1)) {
> - error = ext4_inode_attach_jinode(inode);
> - if (error)
> - goto out_mmap_sem;
> - }
> -
> - /*
> - * Update c/mtime and tail zero the EOF folio on
> - * truncate up. ext4_truncate() handles the shrink case
> - * below.
> - */
> - if (!shrink) {
> - inode_set_mtime_to_ts(inode,
> - inode_set_ctime_current(inode));
> - if (oldsize & (inode->i_sb->s_blocksize - 1)) {
> - error = ext4_block_zero_eof(inode,
> - oldsize, LLONG_MAX);
> - if (error)
> - goto out_mmap_sem;
> - }
> - }
> -
> - handle = ext4_journal_start(inode, EXT4_HT_INODE, 3);
> - if (IS_ERR(handle)) {
> - error = PTR_ERR(handle);
> - goto out_mmap_sem;
> - }
> - if (ext4_handle_valid(handle) && shrink) {
> - error = ext4_orphan_add(handle, inode);
> - orphan = 1;
> - if (error)
> - goto out_handle;
> - }
> -
> - if (shrink)
> - ext4_fc_track_range(handle, inode,
> - (attr->ia_size > 0 ? attr->ia_size - 1 : 0) >>
> - inode->i_sb->s_blocksize_bits,
> - EXT_MAX_BLOCKS - 1);
> - else
> - ext4_fc_track_range(
> - handle, inode,
> - (oldsize > 0 ? oldsize - 1 : oldsize) >>
> - inode->i_sb->s_blocksize_bits,
> - (attr->ia_size > 0 ? attr->ia_size - 1 : 0) >>
> - inode->i_sb->s_blocksize_bits);
> -
> - /*
> - * We have to update i_size under i_data_sem together
> - * with i_disksize to avoid races with writeback code
> - * updating disksize in mpage_map_and_submit_extent().
> - */
> - down_write(&EXT4_I(inode)->i_data_sem);
> - i_size_write(inode, attr->ia_size);
> - EXT4_I(inode)->i_disksize = attr->ia_size;
> - up_write(&EXT4_I(inode)->i_data_sem);
> -
> - error = ext4_mark_inode_dirty(handle, inode);
> -out_handle:
> - ext4_journal_stop(handle);
> - if (error)
> - goto out_mmap_sem;
> - if (!shrink) {
> - pagecache_isize_extended(inode, oldsize,
> - inode->i_size);
> - } else if (ext4_should_journal_data(inode)) {
> - ext4_wait_for_tail_page_commit(inode);
> - }
> + if (attr->ia_size > oldsize)
> + error = ext4_truncate_up(inode, oldsize, attr->ia_size);
> + else {
> + /* Shrink or do not change i_size. */
> + error = ext4_truncate_down(inode, oldsize,
> + attr->ia_size, &orphan);
> }
>
> - /*
> - * Truncate pagecache after we've waited for commit
> - * in data=journal mode to make pages freeable.
> - */
> - truncate_pagecache(inode, inode->i_size);
> - /*
> - * Call ext4_truncate() even if i_size didn't change to
> - * truncate possible preallocated blocks.
> - */
> - if (attr->ia_size <= oldsize) {
> - rc = ext4_truncate(inode);
> - if (rc)
> - error = rc;
> - }
> -out_mmap_sem:
> filemap_invalidate_unlock(inode->i_mapping);
> }
>
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v4 03/23] ext4: simplify error handling in ext4_setattr()
From: Ojaswin Mujoo @ 2026-05-19 6:08 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <20260511072344.191271-4-yi.zhang@huaweicloud.com>
On Mon, May 11, 2026 at 03:23:23PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> Refactor the error handling in ext4_setattr() for better clarity:
>
> - Return directly on ext4_break_layouts() failure.
> - Propagate ext4_truncate() errors using the existing error variable
> and jump to the common 'err_out' label.
> - Propagate posix_acl_chmod() errors also through the error variable,
> as it theoretically does not return a non-fatal error.
>
> With these changes, every error path either returns immediately or jumps
> to err_out. Consequently, the "if (!error)" condition guarding
> setattr_copy() and mark_inode_dirty() becomes unreachable for error
> cases. Remove this redundant check and the unused rc variable can be
> removed as well.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Looks good to me:
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
> fs/ext4/inode.c | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 35e958f89bd5..b1ef706987c3 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5989,7 +5989,7 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> struct iattr *attr)
> {
> struct inode *inode = d_inode(dentry);
> - int error, rc = 0;
> + int error;
> int orphan = 0;
> const unsigned int ia_valid = attr->ia_valid;
> bool inc_ivers = true;
> @@ -6102,10 +6102,10 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>
> filemap_invalidate_lock(inode->i_mapping);
>
> - rc = ext4_break_layouts(inode);
> - if (rc) {
> + error = ext4_break_layouts(inode);
> + if (error) {
> filemap_invalidate_unlock(inode->i_mapping);
> - goto err_out;
> + return error;
> }
>
> if (attr->ia_size > oldsize)
> @@ -6117,15 +6117,19 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> }
>
> filemap_invalidate_unlock(inode->i_mapping);
> + if (error)
> + goto err_out;
> }
>
> - if (!error) {
> - if (inc_ivers)
> - inode_inc_iversion(inode);
> - setattr_copy(idmap, inode, attr);
> - mark_inode_dirty(inode);
> - }
> + if (inc_ivers)
> + inode_inc_iversion(inode);
> + setattr_copy(idmap, inode, attr);
> + mark_inode_dirty(inode);
>
> + if (ia_valid & ATTR_MODE)
> + error = posix_acl_chmod(idmap, dentry, inode->i_mode);
> +
> +err_out:
> /*
> * If the call to ext4_truncate failed to get a transaction handle at
> * all, we need to clean up the in-core orphan list manually.
> @@ -6133,14 +6137,8 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> if (orphan && inode->i_nlink)
> ext4_orphan_del(NULL, inode);
>
> - if (!error && (ia_valid & ATTR_MODE))
> - rc = posix_acl_chmod(idmap, dentry, inode->i_mode);
> -
> -err_out:
> - if (error)
> + if (error)
> ext4_std_error(inode->i_sb, error);
> - if (!error)
> - error = rc;
> return error;
> }
>
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v2] mm: do not install PMD mappings when handling a COW fault
From: Zhang Yi @ 2026-05-19 8:36 UTC (permalink / raw)
To: linux-mm
Cc: linux-ext4, linux-kernel, david, yi.zhang, karol.wachowski,
wangkefeng.wang, yangerkun, liuyongqiang13
In-Reply-To: <20251024102237.3332200-1-yi.zhang@huaweicloud.com>
Gentle ping – could anyone take this patch?
Thanks,
Yi.
On 10/24/2025 6:22 PM, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> When pinning a page with FOLL_LONGTERM in a CoW VMA and a PMD-aligned
> (2MB on x86) large folio follow_page_mask() failed to obtain a valid
> anonymous page, resulting in an infinite loop issue. The specific
> triggering process is as follows:
>
> 1. User call mmap with a 2MB size in MAP_PRIVATE mode for a file that
> has a 2MB large folio installed in the page cache.
>
> addr = mmap(NULL, 2*1024*1024, PROT_READ, MAP_PRIVATE, file_fd, 0);
>
> 2. The kernel driver pass this mapped address to pin_user_pages_fast()
> in FOLL_LONGTERM mode.
>
> pin_user_pages_fast(addr, 512, FOLL_LONGTERM, pages);
>
> -> pin_user_pages_fast()
> | gup_fast_fallback()
> | __gup_longterm_locked()
> | __get_user_pages_locked()
> | __get_user_pages()
> | follow_page_mask()
> | follow_p4d_mask()
> | follow_pud_mask()
> | follow_pmd_mask() //pmd_leaf(pmdval) is true because the
> | //huge PMD is installed. This is normal
> | //in the first round, but it shouldn't
> | //happen in the second round.
> | follow_huge_pmd() //require an anonymous page
> | return -EMLINK;
> | faultin_page()
> | handle_mm_fault()
> | wp_huge_pmd() //remove PMD and fall back to PTE
> | handle_pte_fault()
> | do_pte_missing()
> | do_fault()
> | do_read_fault() //FAULT_FLAG_WRITE is not set
> | finish_fault()
> | do_set_pmd() //install a huge PMD again, this is wrong!!!
> | do_wp_page() //create private anonymous pages
> <- goto retry;
>
> Due to an incorrectly large PMD set in do_read_fault(),
> follow_pmd_mask() always returns -EMLINK, causing an infinite loop.
>
> David pointed out that we can preallocate a page table and remap the PMD
> to be mapped by a PTE table in wp_huge_pmd() in the future. But now we
> can avoid this issue by not installing PMD mappings when handling a COW
> and unshare fault in do_set_pmd().
>
> Fixes: a7f226604170 ("mm/gup: trigger FAULT_FLAG_UNSHARE when R/O-pinning a possibly shared anonymous page")
> Reported-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> Closes: https://lore.kernel.org/linux-ext4/844e5cd4-462e-4b88-b3b5-816465a3b7e3@linux.intel.com/
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
> mm/memory.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 0ba4f6b71847..0748a31367df 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5212,6 +5212,11 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct folio *folio, struct page *pa
> if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
> return ret;
>
> + /* We're about to trigger CoW, so never map it through a PMD. */
> + if (is_cow_mapping(vma->vm_flags) &&
> + (vmf->flags & (FAULT_FLAG_WRITE|FAULT_FLAG_UNSHARE)))
> + return ret;
> +
> if (folio_order(folio) != HPAGE_PMD_ORDER)
> return ret;
> page = &folio->page;
^ permalink raw reply
* Re: [PATCH v2] mm: do not install PMD mappings when handling a COW fault
From: William Kucharski @ 2026-05-19 9:02 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-mm, linux-ext4, linux-kernel, david, yi.zhang,
karol.wachowski, wangkefeng.wang, yangerkun, liuyongqiang13
In-Reply-To: <a3c8038a-43e9-4427-92dd-8fed619001e8@huaweicloud.com>
> On May 19, 2026, at 02:36, Zhang Yi <yi.zhang@huaweicloud.com> wrote:
>
> Gentle ping – could anyone take this patch?
>
> Thanks,
> Yi.
Could you update the comment to clarify why you shouldn't install PMD mappings
while doing CoW rather than just state it should never be done?
>
> On 10/24/2025 6:22 PM, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> When pinning a page with FOLL_LONGTERM in a CoW VMA and a PMD-aligned
>> (2MB on x86) large folio follow_page_mask() failed to obtain a valid
>> anonymous page, resulting in an infinite loop issue. The specific
>> triggering process is as follows:
>>
>> 1. User call mmap with a 2MB size in MAP_PRIVATE mode for a file that
>> has a 2MB large folio installed in the page cache.
>>
>> addr = mmap(NULL, 2*1024*1024, PROT_READ, MAP_PRIVATE, file_fd, 0);
>>
>> 2. The kernel driver pass this mapped address to pin_user_pages_fast()
>> in FOLL_LONGTERM mode.
>>
>> pin_user_pages_fast(addr, 512, FOLL_LONGTERM, pages);
>>
>> -> pin_user_pages_fast()
>> | gup_fast_fallback()
>> | __gup_longterm_locked()
>> | __get_user_pages_locked()
>> | __get_user_pages()
>> | follow_page_mask()
>> | follow_p4d_mask()
>> | follow_pud_mask()
>> | follow_pmd_mask() //pmd_leaf(pmdval) is true because the
>> | //huge PMD is installed. This is normal
>> | //in the first round, but it shouldn't
>> | //happen in the second round.
>> | follow_huge_pmd() //require an anonymous page
>> | return -EMLINK;
>> | faultin_page()
>> | handle_mm_fault()
>> | wp_huge_pmd() //remove PMD and fall back to PTE
>> | handle_pte_fault()
>> | do_pte_missing()
>> | do_fault()
>> | do_read_fault() //FAULT_FLAG_WRITE is not set
>> | finish_fault()
>> | do_set_pmd() //install a huge PMD again, this is wrong!!!
>> | do_wp_page() //create private anonymous pages
>> <- goto retry;
>>
>> Due to an incorrectly large PMD set in do_read_fault(),
>> follow_pmd_mask() always returns -EMLINK, causing an infinite loop.
>>
>> David pointed out that we can preallocate a page table and remap the PMD
>> to be mapped by a PTE table in wp_huge_pmd() in the future. But now we
>> can avoid this issue by not installing PMD mappings when handling a COW
>> and unshare fault in do_set_pmd().
>>
>> Fixes: a7f226604170 ("mm/gup: trigger FAULT_FLAG_UNSHARE when R/O-pinning a possibly shared anonymous page")
>> Reported-by: Karol Wachowski <karol.wachowski@linux.intel.com>
>> Closes: https://lore.kernel.org/linux-ext4/844e5cd4-462e-4b88-b3b5-816465a3b7e3@linux.intel.com/
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> ---
>> mm/memory.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 0ba4f6b71847..0748a31367df 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5212,6 +5212,11 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct folio *folio, struct page *pa
>> if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
>> return ret;
>>
>> + /* We're about to trigger CoW, so never map it through a PMD. */
>> + if (is_cow_mapping(vma->vm_flags) &&
>> + (vmf->flags & (FAULT_FLAG_WRITE|FAULT_FLAG_UNSHARE)))
>> + return ret;
>> +
>> if (folio_order(folio) != HPAGE_PMD_ORDER)
>> return ret;
>> page = &folio->page;
>
>
^ permalink raw reply
* Re: [PATCH v4 04/23] ext4: add iomap address space operations for buffered I/O
From: Ojaswin Mujoo @ 2026-05-19 9:28 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <20260511072344.191271-5-yi.zhang@huaweicloud.com>
On Mon, May 11, 2026 at 03:23:24PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> Introduce initial support for iomap in the buffered I/O path for regular
> files on ext4.
>
> - Add a new inode state flag EXT4_STATE_BUFFERED_IOMAP to indicate the
> inode uses iomap instead of buffer_head for buffered I/O
> - Add helper ext4_inode_buffered_iomap() to check the flag
> - Add new address space operations ext4_iomap_aops with callbacks that
> will use generic iomap implementations
> - Add ext4_iomap_aops to ext4_set_aops() when the flag is set
>
> The following callbacks(read_folio(), readahead(), writepages()) are
> provided as placeholders and will be implemented in later patches.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Hi Zhang, looks good to me. Just a questions below:
> ---
> fs/ext4/ext4.h | 7 +++++++
> fs/ext4/inode.c | 32 ++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 94283a991e5c..1e27d73d7427 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1972,6 +1972,7 @@ enum {
> EXT4_STATE_FC_COMMITTING, /* Fast commit ongoing */
> EXT4_STATE_FC_FLUSHING_DATA, /* Fast commit flushing data */
> EXT4_STATE_ORPHAN_FILE, /* Inode orphaned in orphan file */
> + EXT4_STATE_BUFFERED_IOMAP, /* Inode use iomap for buffered IO */
> };
>
> #define EXT4_INODE_BIT_FNS(name, field, offset) \
> @@ -2040,6 +2041,12 @@ static inline bool ext4_inode_orphan_tracked(struct inode *inode)
> !list_empty(&EXT4_I(inode)->i_orphan);
> }
>
> +/* Whether the inode pass through the iomap infrastructure for buffered I/O */
> +static inline bool ext4_inode_buffered_iomap(struct inode *inode)
> +{
> + return ext4_test_inode_state(inode, EXT4_STATE_BUFFERED_IOMAP);
> +}
> +
> /*
> * Codes for operating systems
> */
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b1ef706987c3..178ac2be37b7 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3908,6 +3908,22 @@ const struct iomap_ops ext4_iomap_report_ops = {
> .iomap_begin = ext4_iomap_begin_report,
> };
>
> +static int ext4_iomap_read_folio(struct file *file, struct folio *folio)
> +{
> + return 0;
> +}
> +
> +static void ext4_iomap_readahead(struct readahead_control *rac)
> +{
> +
> +}
> +
> +static int ext4_iomap_writepages(struct address_space *mapping,
> + struct writeback_control *wbc)
> +{
> + return 0;
> +}
> +
> /*
> * For data=journal mode, folio should be marked dirty only when it was
> * writeably mapped. When that happens, it was already attached to the
> @@ -3994,6 +4010,20 @@ static const struct address_space_operations ext4_da_aops = {
> .swap_activate = ext4_iomap_swap_activate,
> };
>
> +static const struct address_space_operations ext4_iomap_aops = {
> + .read_folio = ext4_iomap_read_folio,
> + .readahead = ext4_iomap_readahead,
> + .writepages = ext4_iomap_writepages,
> + .dirty_folio = iomap_dirty_folio,
> + .bmap = ext4_bmap,
> + .invalidate_folio = iomap_invalidate_folio,
> + .release_folio = iomap_release_folio,
> + .migrate_folio = filemap_migrate_folio,
> + .is_partially_uptodate = iomap_is_partially_uptodate,
> + .error_remove_folio = generic_error_remove_folio,
> + .swap_activate = ext4_iomap_swap_activate,
> +};
So one question, for ->release_folio() we are using
iomap_release_folio() instead of ext4_release_folio() here which doesnt
make the jbd2_journal_try_to_free_bufferes() call. IIUC this function
seems to be trying to clean up already checkpointed buffers.
I wanted to check if ->release_folio() can be called for folios with
ext4 metadata buffers? (from my limited understanding of
shrink_folio_list() -> filemap_release_folio() it seems we can) And if
it can be called, is it okay to skip the
jbd2_journal_try_to_free_buffers call?
Regards,
ojaswin
> +
> static const struct address_space_operations ext4_dax_aops = {
> .writepages = ext4_dax_writepages,
> .dirty_folio = noop_dirty_folio,
> @@ -4015,6 +4045,8 @@ void ext4_set_aops(struct inode *inode)
> }
> if (IS_DAX(inode))
> inode->i_mapping->a_ops = &ext4_dax_aops;
> + else if (ext4_inode_buffered_iomap(inode))
> + inode->i_mapping->a_ops = &ext4_iomap_aops;
> else if (test_opt(inode->i_sb, DELALLOC))
> inode->i_mapping->a_ops = &ext4_da_aops;
> else
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v4 06/23] ext4: pass out extent seq counter when mapping da blocks
From: Ojaswin Mujoo @ 2026-05-19 10:02 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <20260511072344.191271-7-yi.zhang@huaweicloud.com>
On Mon, May 11, 2026 at 03:23:26PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> The iomap buffered write path does not hold any locks between querying
> inode extent mapping information and performing buffered writes. It
> relies on the sequence counter saved in the inode to detect stale
> mappings.
>
> Commit 07c440e8da8f ("ext4: pass out extent seq counter when mapping
> blocks") added the m_seq field to ext4_map_blocks to pass out extent
> sequence numbers, but it missed two callsites within
> ext4_da_map_blocks(). These callsites are on the delayed allocation
> path, which is also used in the iomap buffered write path. Pass out the
> sequence counter to ensure stale mappings can be detected.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good,
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Regards,
Ojaswin
> ---
> 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 6c4d9137b279..39577a6b65b9 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1929,7 +1929,7 @@ static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map)
> ext4_check_map_extents_env(inode);
>
> /* Lookup extent status tree firstly */
> - if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es, NULL)) {
> + if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es, &map->m_seq)) {
> map->m_len = min_t(unsigned int, map->m_len,
> es.es_len - (map->m_lblk - es.es_lblk));
>
> @@ -1982,7 +1982,7 @@ static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map)
> * is held in write mode, before inserting a new da entry in
> * the extent status tree.
> */
> - if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es, NULL)) {
> + if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es, &map->m_seq)) {
> map->m_len = min_t(unsigned int, map->m_len,
> es.es_len - (map->m_lblk - es.es_lblk));
>
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v4 05/23] ext4: implement buffered read path using iomap
From: Ojaswin Mujoo @ 2026-05-19 10:00 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <20260511072344.191271-6-yi.zhang@huaweicloud.com>
On Mon, May 11, 2026 at 03:23:25PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> Implement the iomap read path for ext4 by introducing a new
> ext4_iomap_buffered_read_ops instance. This provides the read_folio()
> and readahead() callbacks for ext4_iomap_aops. The implementation
> introduces:
>
> - ext4_iomap_map_blocks(): Helper function to query extent mappings for
> a given read range using ext4_map_blocks() and convert the mapping
> information to iomap type
> - ext4_iomap_buffered_read_begin(): The iomap_begin callback that maps
> blocks, validates filesystem state, and populates the iomap. It
> returns -ERANGE for inline data which is not yet supported.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Regards,
Ojaswin
> ---
> fs/ext4/inode.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 178ac2be37b7..6c4d9137b279 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3908,14 +3908,57 @@ const struct iomap_ops ext4_iomap_report_ops = {
> .iomap_begin = ext4_iomap_begin_report,
> };
>
> +static int ext4_iomap_map_blocks(struct inode *inode, loff_t offset,
> + loff_t length, struct ext4_map_blocks *map)
> +{
> + u8 blkbits = inode->i_blkbits;
> +
> + if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
> + return -EINVAL;
> +
> + /* Calculate the first and last logical blocks respectively. */
> + map->m_lblk = offset >> blkbits;
> + map->m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
> + EXT4_MAX_LOGICAL_BLOCK) - map->m_lblk + 1;
> +
> + return ext4_map_blocks(NULL, inode, map, 0);
> +}
> +
> +static int ext4_iomap_buffered_read_begin(struct inode *inode, loff_t offset,
> + loff_t length, unsigned int flags, struct iomap *iomap,
> + struct iomap *srcmap)
> +{
> + struct ext4_map_blocks map;
> + int ret;
> +
> + if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> + return -EIO;
> +
> + /* Inline data support is not yet available. */
> + if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
> + return -ERANGE;
> +
> + ret = ext4_iomap_map_blocks(inode, offset, length, &map);
> + if (ret < 0)
> + return ret;
> +
> + ext4_set_iomap(inode, iomap, &map, offset, length, flags);
> + return 0;
> +}
> +
> +const struct iomap_ops ext4_iomap_buffered_read_ops = {
> + .iomap_begin = ext4_iomap_buffered_read_begin,
> +};
> +
> static int ext4_iomap_read_folio(struct file *file, struct folio *folio)
> {
> + iomap_bio_read_folio(folio, &ext4_iomap_buffered_read_ops);
> return 0;
> }
>
> static void ext4_iomap_readahead(struct readahead_control *rac)
> {
> -
> + iomap_bio_readahead(rac, &ext4_iomap_buffered_read_ops);
> }
>
> static int ext4_iomap_writepages(struct address_space *mapping,
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v4 07/23] ext4: do not use data=ordered mode for inodes using buffered iomap path
From: Ojaswin Mujoo @ 2026-05-19 10:41 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <20260511072344.191271-8-yi.zhang@huaweicloud.com>
On Mon, May 11, 2026 at 03:23:27PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> The data=ordered mode introduces two fundamental conflicts with the
> iomap buffered write path, leading to potential deadlocks.
>
> 1) Lock ordering conflict
> In the iomap writeback path, each folio is processed sequentially:
> the folio lock is acquired first, followed by starting a transaction
> to create block mappings. In data=ordered mode, writeback triggered
> by the journal commit process may attempt to acquire a folio lock
> that is already held by iomap. Meanwhile, iomap, under that same
> folio lock, may start a new transaction and wait for the currently
> committing transaction to finish, resulting in a deadlock.
Right, makes sense.
>
> 2) Partial folio submission not supported
> When block size is smaller than folio size, a folio may contain both
> mapped and unmapped blocks. In data=ordered mode, if the journal
> waits for such a folio to be written back while the regular writeback
> process has already started committing it (with the writeback flag
> set), mapping the remaining unmapped blocks can deadlock. This is
> because the writeback flag is cleared only after the entire folio is
> processed and committed.
Okay so IIUC, if we do end up using iomap with ordered data, there are 2
codepaths with issues here:
txn_commit
ordered data writeback (say it goes via iomap)
folio_lock
iomap_writeback_folio
folio_start_writeback
iomap_writeback_range
ext4_map_block
txn_start
wait for tnx commit - DEADLOCK
Currently we avoid this by having ext4_normal_submit_inode_buffers()
pass can_map = 0 so journal flush makese sure not to start any txn.
Then we have
txn_commit background writeback (via iomap)
folio_lock()
ordered data writeback
folio_lock
iomap_writeback_folio
folio_start_writeback
iomap_writeback_range
ext4_map_block
txn_start
wait for txn commit - DEADLOCK
Currently, this is taken care because we try to start the txn before
taking any folio locks/starting writeback, and hence we cannot deadlock.
If the above description makes sense, do you think it'd be good to add
them to the commit message. The reason is that although these paths seem
obvious when we look at them a lot, it took me a good bit of time to
understand what deadlocks you are talking about here :p
Having the code traces like above makes it very clear.
>
> To support data=ordered mode, the iomap core would need two invasive
> changes:
> - Acquire the transaction handle before locking any folio for
> writeback.
> - Support partial folio submission.
>
> Both changes are complicated and risk performance regressions.
> Therefore, we must avoid using data=ordered mode when converting to the
> iomap path.
>
> Currently, data=ordered mode is used in three scenarios:
> - Append write
> - Post-EOF partial block truncate-up followed by append write
> - Online defragmentation
>
> We can address the first two without data=ordered mode:
> - For append write: always allocate unwritten blocks (i.e. always
> enable dioread_nolock), preserving the behavior of current
> extent-type inodes.
> - For post-EOF truncate-up + append write: postpone updating i_disksize
> until after the zeroed partial block has been written back.
I'm still going through how we are addressing no data=ordered so will
get back on this in some time.
Thanks,
Ojaswin
>
> Online defragmentation does not yet support iomap; this can be resolved
> separately in the future.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/ext4/ext4_jbd2.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 63d17c5201b5..26999f173870 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -383,7 +383,12 @@ static inline int ext4_should_journal_data(struct inode *inode)
>
> static inline int ext4_should_order_data(struct inode *inode)
> {
> - return ext4_inode_journal_mode(inode) & EXT4_INODE_ORDERED_DATA_MODE;
> + /*
> + * inodes using the iomap buffered I/O path do not use the
> + * data=ordered mode.
> + */
> + return !ext4_inode_buffered_iomap(inode) &&
> + (ext4_inode_journal_mode(inode) & EXT4_INODE_ORDERED_DATA_MODE);
> }
>
> static inline int ext4_should_writeback_data(struct inode *inode)
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v4 04/23] ext4: add iomap address space operations for buffered I/O
From: Zhang Yi @ 2026-05-19 12:35 UTC (permalink / raw)
To: Ojaswin Mujoo
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <agwtJq5-B2E-t7zT@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On 5/19/2026 5:28 PM, Ojaswin Mujoo wrote:
> On Mon, May 11, 2026 at 03:23:24PM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Introduce initial support for iomap in the buffered I/O path for regular
>> files on ext4.
>>
>> - Add a new inode state flag EXT4_STATE_BUFFERED_IOMAP to indicate the
>> inode uses iomap instead of buffer_head for buffered I/O
>> - Add helper ext4_inode_buffered_iomap() to check the flag
>> - Add new address space operations ext4_iomap_aops with callbacks that
>> will use generic iomap implementations
>> - Add ext4_iomap_aops to ext4_set_aops() when the flag is set
>>
>> The following callbacks(read_folio(), readahead(), writepages()) are
>> provided as placeholders and will be implemented in later patches.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>
> Hi Zhang, looks good to me. Just a questions below:
Hi, Ojaswin! Thank you for the review of this series.
>> ---
>> fs/ext4/ext4.h | 7 +++++++
>> fs/ext4/inode.c | 32 ++++++++++++++++++++++++++++++++
>> 2 files changed, 39 insertions(+)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 94283a991e5c..1e27d73d7427 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -1972,6 +1972,7 @@ enum {
>> EXT4_STATE_FC_COMMITTING, /* Fast commit ongoing */
>> EXT4_STATE_FC_FLUSHING_DATA, /* Fast commit flushing data */
>> EXT4_STATE_ORPHAN_FILE, /* Inode orphaned in orphan file */
>> + EXT4_STATE_BUFFERED_IOMAP, /* Inode use iomap for buffered IO */
>> };
>>
>> #define EXT4_INODE_BIT_FNS(name, field, offset) \
>> @@ -2040,6 +2041,12 @@ static inline bool ext4_inode_orphan_tracked(struct inode *inode)
>> !list_empty(&EXT4_I(inode)->i_orphan);
>> }
>>
>> +/* Whether the inode pass through the iomap infrastructure for buffered I/O */
>> +static inline bool ext4_inode_buffered_iomap(struct inode *inode)
>> +{
>> + return ext4_test_inode_state(inode, EXT4_STATE_BUFFERED_IOMAP);
>> +}
>> +
>> /*
>> * Codes for operating systems
>> */
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index b1ef706987c3..178ac2be37b7 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3908,6 +3908,22 @@ const struct iomap_ops ext4_iomap_report_ops = {
>> .iomap_begin = ext4_iomap_begin_report,
>> };
>>
>> +static int ext4_iomap_read_folio(struct file *file, struct folio *folio)
>> +{
>> + return 0;
>> +}
>> +
>> +static void ext4_iomap_readahead(struct readahead_control *rac)
>> +{
>> +
>> +}
>> +
>> +static int ext4_iomap_writepages(struct address_space *mapping,
>> + struct writeback_control *wbc)
>> +{
>> + return 0;
>> +}
>> +
>> /*
>> * For data=journal mode, folio should be marked dirty only when it was
>> * writeably mapped. When that happens, it was already attached to the
>> @@ -3994,6 +4010,20 @@ static const struct address_space_operations ext4_da_aops = {
>> .swap_activate = ext4_iomap_swap_activate,
>> };
>>
>> +static const struct address_space_operations ext4_iomap_aops = {
>> + .read_folio = ext4_iomap_read_folio,
>> + .readahead = ext4_iomap_readahead,
>> + .writepages = ext4_iomap_writepages,
>> + .dirty_folio = iomap_dirty_folio,
>> + .bmap = ext4_bmap,
>> + .invalidate_folio = iomap_invalidate_folio,
>> + .release_folio = iomap_release_folio,
>> + .migrate_folio = filemap_migrate_folio,
>> + .is_partially_uptodate = iomap_is_partially_uptodate,
>> + .error_remove_folio = generic_error_remove_folio,
>> + .swap_activate = ext4_iomap_swap_activate,
>> +};
>
> So one question, for ->release_folio() we are using
> iomap_release_folio() instead of ext4_release_folio() here which doesnt
> make the jbd2_journal_try_to_free_bufferes() call. IIUC this function
> seems to be trying to clean up already checkpointed buffers.
>
> I wanted to check if ->release_folio() can be called for folios with
> ext4 metadata buffers? (from my limited understanding of
> shrink_folio_list() -> filemap_release_folio() it seems we can) And if
> it can be called, is it okay to skip the
> jbd2_journal_try_to_free_buffers call?
Here, in ->release_folio(), folio->mapping points to inode->i_data (the
file's pagecache), not the block device's pagecache. ext4 metadata
resides in the block device's pagecache, which is at a different layer
than this release_folio callback. So we don't need to call
jbd2_journal_try_to_free_buffers() in the iomap path here.
Thanks,
Yi.
>
> Regards,
> ojaswin
>
>> +
>> static const struct address_space_operations ext4_dax_aops = {
>> .writepages = ext4_dax_writepages,
>> .dirty_folio = noop_dirty_folio,
>> @@ -4015,6 +4045,8 @@ void ext4_set_aops(struct inode *inode)
>> }
>> if (IS_DAX(inode))
>> inode->i_mapping->a_ops = &ext4_dax_aops;
>> + else if (ext4_inode_buffered_iomap(inode))
>> + inode->i_mapping->a_ops = &ext4_iomap_aops;
>> else if (test_opt(inode->i_sb, DELALLOC))
>> inode->i_mapping->a_ops = &ext4_da_aops;
>> else
>> --
>> 2.52.0
>>
^ permalink raw reply
* Re: [PATCH v4 07/23] ext4: do not use data=ordered mode for inodes using buffered iomap path
From: Ojaswin Mujoo @ 2026-05-19 13:31 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <agw-Wt4c4Fwevezk@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On Tue, May 19, 2026 at 04:11:30PM +0530, Ojaswin Mujoo wrote:
> On Mon, May 11, 2026 at 03:23:27PM +0800, Zhang Yi wrote:
> > From: Zhang Yi <yi.zhang@huawei.com>
> >
> > The data=ordered mode introduces two fundamental conflicts with the
> > iomap buffered write path, leading to potential deadlocks.
> >
> > 1) Lock ordering conflict
> > In the iomap writeback path, each folio is processed sequentially:
> > the folio lock is acquired first, followed by starting a transaction
> > to create block mappings. In data=ordered mode, writeback triggered
> > by the journal commit process may attempt to acquire a folio lock
> > that is already held by iomap. Meanwhile, iomap, under that same
> > folio lock, may start a new transaction and wait for the currently
> > committing transaction to finish, resulting in a deadlock.
>
> Right, makes sense.
> >
> > 2) Partial folio submission not supported
> > When block size is smaller than folio size, a folio may contain both
> > mapped and unmapped blocks. In data=ordered mode, if the journal
> > waits for such a folio to be written back while the regular writeback
> > process has already started committing it (with the writeback flag
> > set), mapping the remaining unmapped blocks can deadlock. This is
> > because the writeback flag is cleared only after the entire folio is
> > processed and committed.
>
> Okay so IIUC, if we do end up using iomap with ordered data, there are 2
> codepaths with issues here:
>
> txn_commit
> ordered data writeback (say it goes via iomap)
> folio_lock
> iomap_writeback_folio
> folio_start_writeback
> iomap_writeback_range
> ext4_map_block
> txn_start
> wait for tnx commit - DEADLOCK
>
> Currently we avoid this by having ext4_normal_submit_inode_buffers()
> pass can_map = 0 so journal flush makese sure not to start any txn.
>
> Then we have
>
> txn_commit background writeback (via iomap)
>
> folio_lock()
> ordered data writeback
> folio_lock
>
> iomap_writeback_folio
> folio_start_writeback
> iomap_writeback_range
> ext4_map_block
> txn_start
> wait for txn commit - DEADLOCK
Sorry I forget to remove tabs
this is what I meant:
txn_commit
ordered data writeback (say it goes via iomap)
folio_lock
iomap_writeback_folio
folio_start_writeback
iomap_writeback_range
ext4_map_block
txn_start
wait for tnx commit - DEADLOCK
Currently we avoid this by having ext4_normal_submit_inode_buffers()
pass can_map = 0 so journal flush makese sure not to start any txn.
Then we have
txn_commit background writeback (via iomap)
folio_lock()
ordered data writeback
folio_lock
iomap_writeback_folio
folio_start_writeback
iomap_writeback_range
ext4_map_block
txn_start
wait for txn commit - DEADLOCK
>
> Currently, this is taken care because we try to start the txn before
> taking any folio locks/starting writeback, and hence we cannot deadlock.
>
> If the above description makes sense, do you think it'd be good to add
> them to the commit message. The reason is that although these paths seem
> obvious when we look at them a lot, it took me a good bit of time to
> understand what deadlocks you are talking about here :p
>
> Having the code traces like above makes it very clear.
> >
> > To support data=ordered mode, the iomap core would need two invasive
> > changes:
> > - Acquire the transaction handle before locking any folio for
> > writeback.
> > - Support partial folio submission.
> >
> > Both changes are complicated and risk performance regressions.
> > Therefore, we must avoid using data=ordered mode when converting to the
> > iomap path.
> >
> > Currently, data=ordered mode is used in three scenarios:
> > - Append write
> > - Post-EOF partial block truncate-up followed by append write
> > - Online defragmentation
> >
> > We can address the first two without data=ordered mode:
> > - For append write: always allocate unwritten blocks (i.e. always
> > enable dioread_nolock), preserving the behavior of current
> > extent-type inodes.
> > - For post-EOF truncate-up + append write: postpone updating i_disksize
> > until after the zeroed partial block has been written back.
>
> I'm still going through how we are addressing no data=ordered so will
> get back on this in some time.
>
> Thanks,
> Ojaswin
>
> >
> > Online defragmentation does not yet support iomap; this can be resolved
> > separately in the future.
> >
> > Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> > ---
> > fs/ext4/ext4_jbd2.h | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> > index 63d17c5201b5..26999f173870 100644
> > --- a/fs/ext4/ext4_jbd2.h
> > +++ b/fs/ext4/ext4_jbd2.h
> > @@ -383,7 +383,12 @@ static inline int ext4_should_journal_data(struct inode *inode)
> >
> > static inline int ext4_should_order_data(struct inode *inode)
> > {
> > - return ext4_inode_journal_mode(inode) & EXT4_INODE_ORDERED_DATA_MODE;
> > + /*
> > + * inodes using the iomap buffered I/O path do not use the
> > + * data=ordered mode.
> > + */
> > + return !ext4_inode_buffered_iomap(inode) &&
> > + (ext4_inode_journal_mode(inode) & EXT4_INODE_ORDERED_DATA_MODE);
> > }
> >
> > static inline int ext4_should_writeback_data(struct inode *inode)
> > --
> > 2.52.0
> >
^ permalink raw reply
* Re: [PATCH v4 04/23] ext4: add iomap address space operations for buffered I/O
From: Ojaswin Mujoo @ 2026-05-19 16:53 UTC (permalink / raw)
To: Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <236657df-71f2-446d-b44b-39865219a850@huaweicloud.com>
On Tue, May 19, 2026 at 08:35:51PM +0800, Zhang Yi wrote:
> On 5/19/2026 5:28 PM, Ojaswin Mujoo wrote:
> > On Mon, May 11, 2026 at 03:23:24PM +0800, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@huawei.com>
> >>
> >> Introduce initial support for iomap in the buffered I/O path for regular
> >> files on ext4.
> >>
> >> - Add a new inode state flag EXT4_STATE_BUFFERED_IOMAP to indicate the
> >> inode uses iomap instead of buffer_head for buffered I/O
> >> - Add helper ext4_inode_buffered_iomap() to check the flag
> >> - Add new address space operations ext4_iomap_aops with callbacks that
> >> will use generic iomap implementations
> >> - Add ext4_iomap_aops to ext4_set_aops() when the flag is set
> >>
> >> The following callbacks(read_folio(), readahead(), writepages()) are
> >> provided as placeholders and will be implemented in later patches.
> >>
> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> >> Reviewed-by: Jan Kara <jack@suse.cz>
> >
> > Hi Zhang, looks good to me. Just a questions below:
>
> Hi, Ojaswin! Thank you for the review of this series.
>
> >> ---
> >> fs/ext4/ext4.h | 7 +++++++
> >> fs/ext4/inode.c | 32 ++++++++++++++++++++++++++++++++
> >> 2 files changed, 39 insertions(+)
> >>
> >> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> >> index 94283a991e5c..1e27d73d7427 100644
> >> --- a/fs/ext4/ext4.h
> >> +++ b/fs/ext4/ext4.h
> >> @@ -1972,6 +1972,7 @@ enum {
> >> EXT4_STATE_FC_COMMITTING, /* Fast commit ongoing */
> >> EXT4_STATE_FC_FLUSHING_DATA, /* Fast commit flushing data */
> >> EXT4_STATE_ORPHAN_FILE, /* Inode orphaned in orphan file */
> >> + EXT4_STATE_BUFFERED_IOMAP, /* Inode use iomap for buffered IO */
> >> };
> >>
> >> #define EXT4_INODE_BIT_FNS(name, field, offset) \
> >> @@ -2040,6 +2041,12 @@ static inline bool ext4_inode_orphan_tracked(struct inode *inode)
> >> !list_empty(&EXT4_I(inode)->i_orphan);
> >> }
> >>
> >> +/* Whether the inode pass through the iomap infrastructure for buffered I/O */
> >> +static inline bool ext4_inode_buffered_iomap(struct inode *inode)
> >> +{
> >> + return ext4_test_inode_state(inode, EXT4_STATE_BUFFERED_IOMAP);
> >> +}
> >> +
> >> /*
> >> * Codes for operating systems
> >> */
> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> >> index b1ef706987c3..178ac2be37b7 100644
> >> --- a/fs/ext4/inode.c
> >> +++ b/fs/ext4/inode.c
> >> @@ -3908,6 +3908,22 @@ const struct iomap_ops ext4_iomap_report_ops = {
> >> .iomap_begin = ext4_iomap_begin_report,
> >> };
> >>
> >> +static int ext4_iomap_read_folio(struct file *file, struct folio *folio)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static void ext4_iomap_readahead(struct readahead_control *rac)
> >> +{
> >> +
> >> +}
> >> +
> >> +static int ext4_iomap_writepages(struct address_space *mapping,
> >> + struct writeback_control *wbc)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> /*
> >> * For data=journal mode, folio should be marked dirty only when it was
> >> * writeably mapped. When that happens, it was already attached to the
> >> @@ -3994,6 +4010,20 @@ static const struct address_space_operations ext4_da_aops = {
> >> .swap_activate = ext4_iomap_swap_activate,
> >> };
> >>
> >> +static const struct address_space_operations ext4_iomap_aops = {
> >> + .read_folio = ext4_iomap_read_folio,
> >> + .readahead = ext4_iomap_readahead,
> >> + .writepages = ext4_iomap_writepages,
> >> + .dirty_folio = iomap_dirty_folio,
> >> + .bmap = ext4_bmap,
> >> + .invalidate_folio = iomap_invalidate_folio,
> >> + .release_folio = iomap_release_folio,
> >> + .migrate_folio = filemap_migrate_folio,
> >> + .is_partially_uptodate = iomap_is_partially_uptodate,
> >> + .error_remove_folio = generic_error_remove_folio,
> >> + .swap_activate = ext4_iomap_swap_activate,
> >> +};
> >
> > So one question, for ->release_folio() we are using
> > iomap_release_folio() instead of ext4_release_folio() here which doesnt
> > make the jbd2_journal_try_to_free_bufferes() call. IIUC this function
> > seems to be trying to clean up already checkpointed buffers.
> >
> > I wanted to check if ->release_folio() can be called for folios with
> > ext4 metadata buffers? (from my limited understanding of
> > shrink_folio_list() -> filemap_release_folio() it seems we can) And if
> > it can be called, is it okay to skip the
> > jbd2_journal_try_to_free_buffers call?
>
> Here, in ->release_folio(), folio->mapping points to inode->i_data (the
> file's pagecache), not the block device's pagecache. ext4 metadata
> resides in the block device's pagecache, which is at a different layer
> than this release_folio callback. So we don't need to call
> jbd2_journal_try_to_free_buffers() in the iomap path here.
Hi Yi,
Thanks for clarify and yes, thats what I was missing! So this
->release_folio() is only for data folios. So I guess the
jbd2_journal_try_to_free_buffers() is mostly to handle data=journal
case?
Regardless, with that clarification feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Regards,
ojaswin
>
> Thanks,
> Yi.
>
> >
> > Regards,
> > ojaswin
> >
> >> +
> >> static const struct address_space_operations ext4_dax_aops = {
> >> .writepages = ext4_dax_writepages,
> >> .dirty_folio = noop_dirty_folio,
> >> @@ -4015,6 +4045,8 @@ void ext4_set_aops(struct inode *inode)
> >> }
> >> if (IS_DAX(inode))
> >> inode->i_mapping->a_ops = &ext4_dax_aops;
> >> + else if (ext4_inode_buffered_iomap(inode))
> >> + inode->i_mapping->a_ops = &ext4_iomap_aops;
> >> else if (test_opt(inode->i_sb, DELALLOC))
> >> inode->i_mapping->a_ops = &ext4_da_aops;
> >> else
> >> --
> >> 2.52.0
> >>
>
^ permalink raw reply
* [PATCHBOMB v9] fuse/libfuse/e2fsprogs: faster file IO for containerized ext4 servers
From: Darrick J. Wong @ 2026-05-19 22:22 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, fuse-devel
Cc: Miklos Szeredi, Bernd Schubert, Joanne Koong, Theodore Ts'o,
Neal Gompa, Amir Goldstein, Christian Brauner, john
Hi everyone,
This is the ninth public draft of a prototype to connect the Linux
fuse driver to fs-iomap for regular file IO operations to and from files
whose contents persist to locally attached storage devices. With this
release, I show that it's possible to build a fuse server for a real
filesystem (ext4) that runs entirely in userspace yet maintains most of
its performance.
This effort is now separate from the one to run fuse servers in a
constrained environment via systemd. Putting fuse servers in a
container gets you all the blast radii reduction advantages and provides
a pathway to removing less popular filesystem drivers to reduce
maintenance work in the kernel; now we want trade relaxation of that
isolation for better performance.
The fuse command plumbing is very simple -- the ->iomap_begin,
->iomap_end, and iomap ->ioend calls within iomap are turned into
upcalls to the fuse server via a trio of new fuse commands. Pagecache
writeback is now a directio write. The fuse server can upsert mappings
into the kernel for cached access (== zero upcalls for rereads and pure
overwrites!) and the iomap cache revalidation code works.
At this stage I still get about 95% of the kernel ext4 driver's
streaming directio performance on streaming IO, and 110% of its
streaming buffered IO performance. Random buffered IO is about 85% as
fast as the kernel. Random direct IO is about 80% as fast as the
kernel; see the cover letter for the fuse2fs iomap changes for more
details. Unwritten extent conversions on random direct writes are
especially painful for fuse+iomap (~90% more overhead) due to upcall
overhead. And that's with (now dynamic) debugging turned on!
This series has been rebased to 7.1-rc4 since the eighth RFC, with
the following kernel changes:
1. The BPF stuff has been replaced with a filesystem striping mechanism.
This is my first attempt ever to implement raid0.
2. Much tightening of the validation code based on Codex reviews so that
we don't expose more "ABI" than we feel like getting yelled at for
in 2031.
3. Refactored iomap writeback mapping so that you can use the standard
iomap_begin functions for that.
4. Better userspace helpers so that fuse server authors don't have to
know quite so much detail of the innards.
5. The libfuse changes are based off the WIP fuse-service-container
branch.
There are some questions remaining:
a. fuse2fs doesn't support the ext4 journal. Urk.
b. I've dropped everything but the kernel patches for basic plumbing and
file IO paths because frankly they weren't getting looked at.
c. How on earth am I going to separate out the file_operations?
Will it actually work to say that fuse-iomap only supports local
filesystems initially? How many of the "is_iomap?" predicates are
actually for local filesystems and not the IO path???
I would like to any part of this submission reviewed for 7.2 now that
this has been collecting comments and tweaks in non-rfc status for 6
months.
Kernel:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fuse-iomap-striping
libfuse:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/libfuse.git/log/?h=fuse-iomap-striping
e2fsprogs:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/e2fsprogs.git/log/?h=fuse4fs-memory-reclaim
fstests:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fuse2fs
--Darrick
^ permalink raw reply
* Re: [PATCH v2] mm: do not install PMD mappings when handling a COW fault
From: Zhang Yi @ 2026-05-20 1:01 UTC (permalink / raw)
To: William Kucharski
Cc: linux-mm, linux-ext4, linux-kernel, david, yi.zhang,
karol.wachowski, wangkefeng.wang, yangerkun, liuyongqiang13
In-Reply-To: <2381B9B8-FD4D-48FE-BBF3-00D3455A8197@linux.dev>
On 5/19/2026 5:02 PM, William Kucharski wrote:
>
>
>> On May 19, 2026, at 02:36, Zhang Yi <yi.zhang@huaweicloud.com> wrote:
>>
>> Gentle ping – could anyone take this patch?
>>
>> Thanks,
>> Yi.
>
> Could you update the comment to clarify why you shouldn't install PMD mappings
> while doing CoW rather than just state it should never be done?
OK, will do.
Thanks,
Yi.
>
>>
>> On 10/24/2025 6:22 PM, Zhang Yi wrote:
>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> When pinning a page with FOLL_LONGTERM in a CoW VMA and a PMD-aligned
>>> (2MB on x86) large folio follow_page_mask() failed to obtain a valid
>>> anonymous page, resulting in an infinite loop issue. The specific
>>> triggering process is as follows:
>>>
>>> 1. User call mmap with a 2MB size in MAP_PRIVATE mode for a file that
>>> has a 2MB large folio installed in the page cache.
>>>
>>> addr = mmap(NULL, 2*1024*1024, PROT_READ, MAP_PRIVATE, file_fd, 0);
>>>
>>> 2. The kernel driver pass this mapped address to pin_user_pages_fast()
>>> in FOLL_LONGTERM mode.
>>>
>>> pin_user_pages_fast(addr, 512, FOLL_LONGTERM, pages);
>>>
>>> -> pin_user_pages_fast()
>>> | gup_fast_fallback()
>>> | __gup_longterm_locked()
>>> | __get_user_pages_locked()
>>> | __get_user_pages()
>>> | follow_page_mask()
>>> | follow_p4d_mask()
>>> | follow_pud_mask()
>>> | follow_pmd_mask() //pmd_leaf(pmdval) is true because the
>>> | //huge PMD is installed. This is normal
>>> | //in the first round, but it shouldn't
>>> | //happen in the second round.
>>> | follow_huge_pmd() //require an anonymous page
>>> | return -EMLINK;
>>> | faultin_page()
>>> | handle_mm_fault()
>>> | wp_huge_pmd() //remove PMD and fall back to PTE
>>> | handle_pte_fault()
>>> | do_pte_missing()
>>> | do_fault()
>>> | do_read_fault() //FAULT_FLAG_WRITE is not set
>>> | finish_fault()
>>> | do_set_pmd() //install a huge PMD again, this is wrong!!!
>>> | do_wp_page() //create private anonymous pages
>>> <- goto retry;
>>>
>>> Due to an incorrectly large PMD set in do_read_fault(),
>>> follow_pmd_mask() always returns -EMLINK, causing an infinite loop.
>>>
>>> David pointed out that we can preallocate a page table and remap the PMD
>>> to be mapped by a PTE table in wp_huge_pmd() in the future. But now we
>>> can avoid this issue by not installing PMD mappings when handling a COW
>>> and unshare fault in do_set_pmd().
>>>
>>> Fixes: a7f226604170 ("mm/gup: trigger FAULT_FLAG_UNSHARE when R/O-pinning a possibly shared anonymous page")
>>> Reported-by: Karol Wachowski <karol.wachowski@linux.intel.com>
>>> Closes: https://lore.kernel.org/linux-ext4/844e5cd4-462e-4b88-b3b5-816465a3b7e3@linux.intel.com/
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> mm/memory.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 0ba4f6b71847..0748a31367df 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -5212,6 +5212,11 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct folio *folio, struct page *pa
>>> if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
>>> return ret;
>>>
>>> + /* We're about to trigger CoW, so never map it through a PMD. */
>>> + if (is_cow_mapping(vma->vm_flags) &&
>>> + (vmf->flags & (FAULT_FLAG_WRITE|FAULT_FLAG_UNSHARE)))
>>> + return ret;
>>> +
>>> if (folio_order(folio) != HPAGE_PMD_ORDER)
>>> return ret;
>>> page = &folio->page;
>>
>>
^ permalink raw reply
* Re: [PATCH v4 04/23] ext4: add iomap address space operations for buffered I/O
From: Zhang Yi @ 2026-05-20 2:49 UTC (permalink / raw)
To: Ojaswin Mujoo, Zhang Yi
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yangerkun,
yukuai
In-Reply-To: <agyVb1U0US8PVgqo@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On 5/20/2026 12:53 AM, Ojaswin Mujoo wrote:
> On Tue, May 19, 2026 at 08:35:51PM +0800, Zhang Yi wrote:
>> On 5/19/2026 5:28 PM, Ojaswin Mujoo wrote:
>>> On Mon, May 11, 2026 at 03:23:24PM +0800, Zhang Yi wrote:
>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>
>>>> Introduce initial support for iomap in the buffered I/O path for regular
>>>> files on ext4.
>>>>
>>>> - Add a new inode state flag EXT4_STATE_BUFFERED_IOMAP to indicate the
>>>> inode uses iomap instead of buffer_head for buffered I/O
>>>> - Add helper ext4_inode_buffered_iomap() to check the flag
>>>> - Add new address space operations ext4_iomap_aops with callbacks that
>>>> will use generic iomap implementations
>>>> - Add ext4_iomap_aops to ext4_set_aops() when the flag is set
>>>>
>>>> The following callbacks(read_folio(), readahead(), writepages()) are
>>>> provided as placeholders and will be implemented in later patches.
>>>>
>>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>
>>> Hi Zhang, looks good to me. Just a questions below:
>>
>> Hi, Ojaswin! Thank you for the review of this series.
>>
>>>> ---
>>>> fs/ext4/ext4.h | 7 +++++++
>>>> fs/ext4/inode.c | 32 ++++++++++++++++++++++++++++++++
>>>> 2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>>>> index 94283a991e5c..1e27d73d7427 100644
>>>> --- a/fs/ext4/ext4.h
>>>> +++ b/fs/ext4/ext4.h
>>>> @@ -1972,6 +1972,7 @@ enum {
>>>> EXT4_STATE_FC_COMMITTING, /* Fast commit ongoing */
>>>> EXT4_STATE_FC_FLUSHING_DATA, /* Fast commit flushing data */
>>>> EXT4_STATE_ORPHAN_FILE, /* Inode orphaned in orphan file */
>>>> + EXT4_STATE_BUFFERED_IOMAP, /* Inode use iomap for buffered IO */
>>>> };
>>>>
>>>> #define EXT4_INODE_BIT_FNS(name, field, offset) \
>>>> @@ -2040,6 +2041,12 @@ static inline bool ext4_inode_orphan_tracked(struct inode *inode)
>>>> !list_empty(&EXT4_I(inode)->i_orphan);
>>>> }
>>>>
>>>> +/* Whether the inode pass through the iomap infrastructure for buffered I/O */
>>>> +static inline bool ext4_inode_buffered_iomap(struct inode *inode)
>>>> +{
>>>> + return ext4_test_inode_state(inode, EXT4_STATE_BUFFERED_IOMAP);
>>>> +}
>>>> +
>>>> /*
>>>> * Codes for operating systems
>>>> */
>>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>>> index b1ef706987c3..178ac2be37b7 100644
>>>> --- a/fs/ext4/inode.c
>>>> +++ b/fs/ext4/inode.c
>>>> @@ -3908,6 +3908,22 @@ const struct iomap_ops ext4_iomap_report_ops = {
>>>> .iomap_begin = ext4_iomap_begin_report,
>>>> };
>>>>
>>>> +static int ext4_iomap_read_folio(struct file *file, struct folio *folio)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void ext4_iomap_readahead(struct readahead_control *rac)
>>>> +{
>>>> +
>>>> +}
>>>> +
>>>> +static int ext4_iomap_writepages(struct address_space *mapping,
>>>> + struct writeback_control *wbc)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> /*
>>>> * For data=journal mode, folio should be marked dirty only when it was
>>>> * writeably mapped. When that happens, it was already attached to the
>>>> @@ -3994,6 +4010,20 @@ static const struct address_space_operations ext4_da_aops = {
>>>> .swap_activate = ext4_iomap_swap_activate,
>>>> };
>>>>
>>>> +static const struct address_space_operations ext4_iomap_aops = {
>>>> + .read_folio = ext4_iomap_read_folio,
>>>> + .readahead = ext4_iomap_readahead,
>>>> + .writepages = ext4_iomap_writepages,
>>>> + .dirty_folio = iomap_dirty_folio,
>>>> + .bmap = ext4_bmap,
>>>> + .invalidate_folio = iomap_invalidate_folio,
>>>> + .release_folio = iomap_release_folio,
>>>> + .migrate_folio = filemap_migrate_folio,
>>>> + .is_partially_uptodate = iomap_is_partially_uptodate,
>>>> + .error_remove_folio = generic_error_remove_folio,
>>>> + .swap_activate = ext4_iomap_swap_activate,
>>>> +};
>>>
>>> So one question, for ->release_folio() we are using
>>> iomap_release_folio() instead of ext4_release_folio() here which doesnt
>>> make the jbd2_journal_try_to_free_bufferes() call. IIUC this function
>>> seems to be trying to clean up already checkpointed buffers.
>>>
>>> I wanted to check if ->release_folio() can be called for folios with
>>> ext4 metadata buffers? (from my limited understanding of
>>> shrink_folio_list() -> filemap_release_folio() it seems we can) And if
>>> it can be called, is it okay to skip the
>>> jbd2_journal_try_to_free_buffers call?
>>
>> Here, in ->release_folio(), folio->mapping points to inode->i_data (the
>> file's pagecache), not the block device's pagecache. ext4 metadata
>> resides in the block device's pagecache, which is at a different layer
>> than this release_folio callback. So we don't need to call
>> jbd2_journal_try_to_free_buffers() in the iomap path here.
>
> Hi Yi,
>
> Thanks for clarify and yes, thats what I was missing! So this
> ->release_folio() is only for data folios. So I guess the
> jbd2_journal_try_to_free_buffers() is mostly to handle data=journal
> case?
Yes, that's my understanding as well. Meanwhile, the comment for the
jbd2_journal_try_to_free_buffers() function looks quite outdated and
needs to be updated.
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 4885903bbd10..239bcf88ed1c 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -2139,38 +2139,23 @@ static void __jbd2_journal_unfile_buffer(struct
journal_head *jh)
}
/**
- * jbd2_journal_try_to_free_buffers() - try to free page buffers.
+ * jbd2_journal_try_to_free_buffers() - try to free folio buffers.
* @journal: journal for operation
* @folio: Folio to detach data from.
*
- * For all the buffers on this page,
- * if they are fully written out ordered data, move them onto BUF_CLEAN
- * so try_to_free_buffers() can reap them.
+ * For each buffer_head on @folio, if the buffer has a journal_head but
+ * is not attached to a running or committing transaction, try to remove
+ * it from the checkpoint list. This is needed for data=journal mode
+ * where data buffers are journaled: once they are checkpointed, the
+ * journal_head can be detached and the buffer freed. If any buffer is
+ * still attached to a transaction, the folio cannot be released and we
+ * bail out. Otherwise we call try_to_free_buffers() to detach all
+ * buffer_heads from the folio.
*
- * This function returns non-zero if we wish try_to_free_buffers()
- * to be called. We do this if the page is releasable by
try_to_free_buffers().
- * We also do it if the page has locked or dirty buffers and the caller
wants
- * us to perform sync or async writeout.
+ * For data=ordered and writeback modes, data buffers never have
+ * journal_heads, so this degenerates to a plain try_to_free_buffers().
*
- * This complicates JBD locking somewhat. We aren't protected by the
- * BKL here. We wish to remove the buffer from its committing or
- * running transaction's ->t_datalist via __jbd2_journal_unfile_buffer.
- *
- * This may *change* the value of transaction_t->t_datalist, so anyone
- * who looks at t_datalist needs to lock against this function.
- *
- * Even worse, someone may be doing a jbd2_journal_dirty_data on this
- * buffer. So we need to lock against that. jbd2_journal_dirty_data()
- * will come out of the lock with the buffer dirty, which makes it
- * ineligible for release here.
- *
- * Who else is affected by this? hmm... Really the only contender
- * is do_get_write_access() - it could be looking at the buffer while
- * journal_try_to_free_buffer() is changing its state. But that
- * cannot happen because we never reallocate freed data as metadata
- * while the data is part of a transaction. Yes?
- *
- * Return false on failure, true on success
+ * Return: true if the folio's buffers were freed, false otherwise
*/
bool jbd2_journal_try_to_free_buffers(journal_t *journal, struct folio
*folio)
{
Thanks,
Yi.
^ permalink raw reply related
* [PATCH v2 3/5] iomap: fix incorrect did_zero setting in iomap_zero_iter()
From: Zhang Yi @ 2026-05-20 3:03 UTC (permalink / raw)
To: linux-fsdevel, linux-xfs
Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
yi.zhang, yizhang089, yangerkun, yukuai
In-Reply-To: <20260520030357.679687-1-yi.zhang@huaweicloud.com>
From: Zhang Yi <yi.zhang@huawei.com>
The did_zero output parameter was unconditionally set after the loop,
which is incorrect. It should only be set when the zeroing operation
actually completes, not when IOMAP_F_STALE is set or when
IOMAP_F_FOLIO_BATCH is set but !folio causes the loop to break early,
or when iomap_iter_advance() returns an error.
This causes did_zero to be incorrectly set when zeroing a clean
unwritten extent because the loop exits early without actually zeroing
any data.
Fix it by using a local variable to track whether any folio was actually
zeroed, and only set did_zero after the loop if zeroing happened.
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 876c2f507f58..27ab33edbdee 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1542,6 +1542,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
const struct iomap_write_ops *write_ops)
{
u64 bytes = iomap_length(iter);
+ bool zeroed = false;
int status;
do {
@@ -1560,6 +1561,8 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
/* a NULL folio means we're done with a folio batch */
if (!folio) {
status = iomap_iter_advance_full(iter);
+ if (status)
+ return status;
break;
}
@@ -1570,6 +1573,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
bytes);
folio_zero_range(folio, offset, bytes);
+ zeroed = true;
folio_mark_accessed(folio);
ret = iomap_write_end(iter, bytes, bytes, folio);
@@ -1579,10 +1583,10 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
status = iomap_iter_advance(iter, bytes);
if (status)
- break;
+ return status;
} while ((bytes = iomap_length(iter)) > 0);
- if (did_zero)
+ if (did_zero && zeroed)
*did_zero = true;
return status;
}
--
2.52.0
^ permalink raw reply related
* [PATCH v2 5/5] iomap: add comments for ifs_clear/set_range_dirty()
From: Zhang Yi @ 2026-05-20 3:03 UTC (permalink / raw)
To: linux-fsdevel, linux-xfs
Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
yi.zhang, yizhang089, yangerkun, yukuai
In-Reply-To: <20260520030357.679687-1-yi.zhang@huaweicloud.com>
From: Zhang Yi <yi.zhang@huawei.com>
The range alignment strategy differs between ifs_clear_range_dirty() and
ifs_set_range_dirty(). The former rounds inwards to clear only
fully-covered blocks, while the latter rounds outwards to mark any
partially-touched block as dirty. Add comments to document this
asymmetry in block range calculation.
Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
fs/iomap/buffered-io.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 76f9a43e283c..b1d917504d5d 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -173,6 +173,13 @@ static unsigned iomap_find_dirty_range(struct folio *folio, u64 *range_start,
return range_end - *range_start;
}
+/*
+ * Clear the per-block dirty bits for the range [@off, @off + @len) within a
+ * folio. The range is rounded inwards so that only blocks fully covered by
+ * the range are cleared. This is required for operations like folio
+ * invalidation, where we must ensure a block is fully clean before discarding
+ * it.
+ */
static void ifs_clear_range_dirty(struct folio *folio,
struct iomap_folio_state *ifs, size_t off, size_t len)
{
@@ -200,6 +207,13 @@ static void iomap_clear_range_dirty(struct folio *folio, size_t off, size_t len)
ifs_clear_range_dirty(folio, ifs, off, len);
}
+/*
+ * Set the per-block dirty bits for the range [@off, @off + @len) within a
+ * folio. The range is rounded outwards so that any block partially touched
+ * by the range is marked dirty. This ensures blocks containing even a
+ * single dirty byte will be included in subsequent writeback, preventing
+ * data loss when partial blocks are written.
+ */
static void ifs_set_range_dirty(struct folio *folio,
struct iomap_folio_state *ifs, size_t off, size_t len)
{
--
2.52.0
^ permalink raw reply related
* [PATCH v2 2/5] iomap: support invalidating partial folios
From: Zhang Yi @ 2026-05-20 3:03 UTC (permalink / raw)
To: linux-fsdevel, linux-xfs
Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
yi.zhang, yizhang089, yangerkun, yukuai
In-Reply-To: <20260520030357.679687-1-yi.zhang@huaweicloud.com>
From: Zhang Yi <yi.zhang@huawei.com>
Current iomap_invalidate_folio() can only invalidate an entire folio. If
we truncate a partial folio on a filesystem where the block size is
smaller than the folio size, it will leave behind dirty bits for the
truncated or punched blocks. During the write-back process, it will
attempt to map the invalid hole range. Fortunately, this has not caused
any real problems so far because the ->writeback_range() function
corrects the length.
However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
the support for invalidating partial folios. When ext4 partially zeroes
out a dirty and unwritten folio, it does not perform a flush first like
XFS. Therefore, if the dirty bits of the corresponding area cannot be
cleared, the zeroed area after writeback remains in the written state
rather than reverting to the unwritten state. Fix this by supporting
invalidation of partial folios.
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 64351a448a8b..876c2f507f58 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -761,6 +761,8 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len)
WARN_ON_ONCE(folio_test_writeback(folio));
folio_cancel_dirty(folio);
ifs_free(folio);
+ } else {
+ iomap_clear_range_dirty(folio, offset, len);
}
}
EXPORT_SYMBOL_GPL(iomap_invalidate_folio);
--
2.52.0
^ permalink raw reply related
* [PATCH v2 0/5] iomap: trivial fixes for ext4 conversion
From: Zhang Yi @ 2026-05-20 3:03 UTC (permalink / raw)
To: linux-fsdevel, linux-xfs
Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
yi.zhang, yizhang089, yangerkun, yukuai
Changes since v1:
- Add fix tags to patch 01 and 04.
- In patch 04, change ifs_set_range_uptodate() to always fall through
to ifs_is_fully_uptodate(), preventing a false-positive uptodate
mask.
- Add patch 05, add comments for ifs_clear/set_range_dirty().
v1: https://lore.kernel.org/linux-fsdevel/20260514062955.1183976-1-yi.zhang@huaweicloud.com/
Original Cover-letter:
This patch series contains a few trivial iomap-related fixes in
preparation for converting ext4 buffered I/O to use iomap.
The first three patches are taken from my ext4 conversion series [1], as
suggested by Christoph. The last patch fixes a bug originally reported
by Sashiko during review of my series; although unrelated to the ext4
conversion, it is worth fixing on its own. Please see the following
patches for detail.
Thanks,
Yi.
[1] https://lore.kernel.org/linux-ext4/20260511072344.191271-1-yi.zhang@huaweicloud.com/
Zhang Yi (5):
iomap: correct the range of a partial dirty clear
iomap: support invalidating partial folios
iomap: fix incorrect did_zero setting in iomap_zero_iter()
iomap: fix out-of-bounds bitmap_set() with zero-length range
iomap: add comments for ifs_clear/set_range_dirty()
fs/iomap/buffered-io.c | 58 ++++++++++++++++++++++++++++++++----------
1 file changed, 44 insertions(+), 14 deletions(-)
--
2.52.0
^ permalink raw reply
* [PATCH v2 4/5] iomap: fix out-of-bounds bitmap_set() with zero-length range
From: Zhang Yi @ 2026-05-20 3:03 UTC (permalink / raw)
To: linux-fsdevel, linux-xfs
Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
yi.zhang, yizhang089, yangerkun, yukuai
In-Reply-To: <20260520030357.679687-1-yi.zhang@huaweicloud.com>
From: Zhang Yi <yi.zhang@huawei.com>
ifs_set_range_dirty() and ifs_set_range_uptodate() compute last_blk
as (off + len - 1) >> i_blkbits. When off is 0 and len is 0, the
unsigned subtraction underflows to SIZE_MAX, producing a huge
last_blk and nr_blks value that causes bitmap_set() to write far
beyond the ifs->state allocation.
Regarding ifs_set_range_uptodate(), it is temporarily safe because len
cannot be passed in as 0. However, for ifs_set_range_dirty() this is
reachable from __iomap_write_end(): when copy_folio_from_iter_atomic()
returns 0 (e.g. user buffer fault) and the folio is already uptodate,
the guard at the top of __iomap_write_end() does not trigger because
!folio_test_uptodate() is false, and iomap_set_range_dirty() is called
with copied == 0.
Add a !len guard to both functions before the computation, so that a
zero-length range is a no-op.
Fixes: 4ce02c679722 ("iomap: Add per-block dirty state tracking to improve performance")
Cc: <stable@vger.kernel.org> # v6.6
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
fs/iomap/buffered-io.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 27ab33edbdee..76f9a43e283c 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -67,11 +67,13 @@ static bool ifs_set_range_uptodate(struct folio *folio,
struct iomap_folio_state *ifs, size_t off, size_t len)
{
struct inode *inode = folio->mapping->host;
- unsigned int first_blk = off >> inode->i_blkbits;
- unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
- unsigned int nr_blks = last_blk - first_blk + 1;
+ unsigned int first_blk, last_blk;
- bitmap_set(ifs->state, first_blk, nr_blks);
+ if (len) {
+ first_blk = off >> inode->i_blkbits;
+ last_blk = (off + len - 1) >> inode->i_blkbits;
+ bitmap_set(ifs->state, first_blk, last_blk - first_blk + 1);
+ }
return ifs_is_fully_uptodate(folio, ifs);
}
@@ -203,13 +205,17 @@ static void ifs_set_range_dirty(struct folio *folio,
{
struct inode *inode = folio->mapping->host;
unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
- unsigned int first_blk = (off >> inode->i_blkbits);
- unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
- unsigned int nr_blks = last_blk - first_blk + 1;
+ unsigned int first_blk, last_blk;
unsigned long flags;
+ if (!len)
+ return;
+
+ first_blk = off >> inode->i_blkbits;
+ last_blk = (off + len - 1) >> inode->i_blkbits;
spin_lock_irqsave(&ifs->state_lock, flags);
- bitmap_set(ifs->state, first_blk + blks_per_folio, nr_blks);
+ bitmap_set(ifs->state, first_blk + blks_per_folio,
+ last_blk - first_blk + 1);
spin_unlock_irqrestore(&ifs->state_lock, flags);
}
--
2.52.0
^ permalink raw reply related
* [PATCH v2 1/5] iomap: correct the range of a partial dirty clear
From: Zhang Yi @ 2026-05-20 3:03 UTC (permalink / raw)
To: linux-fsdevel, linux-xfs
Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
yi.zhang, yizhang089, yangerkun, yukuai
In-Reply-To: <20260520030357.679687-1-yi.zhang@huaweicloud.com>
From: Zhang Yi <yi.zhang@huawei.com>
The block range calculation in ifs_clear_range_dirty() is incorrect when
partially clearing a range in a folio. We cannot clear the dirty bit of
the first block or the last block if the start or end offset is not
blocksize-aligned. This has not yet caused any issues since we always
clear a whole folio in iomap_writeback_folio().
Fix this by rounding up the first block to blocksize alignment, and
calculate the last block by rounding down (using truncation). Correct
the nr_blks calculation accordingly.
Fixes: 4ce02c679722 ("iomap: Add per-block dirty state tracking to improve performance")
Cc: <stable@vger.kernel.org> # v6.6
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
fs/iomap/buffered-io.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d7b648421a70..64351a448a8b 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -176,13 +176,17 @@ static void ifs_clear_range_dirty(struct folio *folio,
{
struct inode *inode = folio->mapping->host;
unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
- unsigned int first_blk = (off >> inode->i_blkbits);
- unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
- unsigned int nr_blks = last_blk - first_blk + 1;
+ unsigned int first_blk = round_up(off, i_blocksize(inode)) >>
+ inode->i_blkbits;
+ unsigned int last_blk = (off + len) >> inode->i_blkbits;
unsigned long flags;
+ if (first_blk >= last_blk)
+ return;
+
spin_lock_irqsave(&ifs->state_lock, flags);
- bitmap_clear(ifs->state, first_blk + blks_per_folio, nr_blks);
+ bitmap_clear(ifs->state, first_blk + blks_per_folio,
+ last_blk - first_blk);
spin_unlock_irqrestore(&ifs->state_lock, flags);
}
--
2.52.0
^ permalink raw reply related
* Re: WARN_ON_ONCE in ext4_journal_check_start() triggered during graceful shutdown
From: Jan Kara @ 2026-05-20 6:54 UTC (permalink / raw)
To: Sanman Pradhan
Cc: linux-ext4@vger.kernel.org, tytso@mit.edu, jack@suse.cz,
adilger.kernel@dilger.ca
In-Reply-To: <SJ0PR05MB870981E19060D579A839B4AFBA002@SJ0PR05MB8709.namprd05.prod.outlook.com>
Hi!
On Tue 19-05-26 22:08:03, Sanman Pradhan wrote:
> We're seeing the following warning on v6.12 during graceful system shutdown:
>
> WARNING: CPU: 3 PID: 215 at fs/ext4/ext4_jbd2.c:73 ext4_journal_check_start+0x57/0xa0
> Workqueue: writeback wb_workfn (flush-259:0)
> Call Trace:
> __ext4_journal_start_sb+0x4b/0x190
> mpage_prepare_extent_to_map+0x41d/0x4c0
> ext4_do_writepages+0x26d/0xd40
> ext4_writepages+0xad/0x170
> do_writepages+0xe2/0x280
> __writeback_single_inode+0x4a/0x380
> writeback_sb_inodes+0x220/0x4f0
> wb_writeback+0x1cb/0x350
> wb_workfn+0x259/0x440
>
> This appears to be a race: during remount-ro, sync_filesystem() completes
> and SB_RDONLY is set, but a writeback kworker that was already scheduled
> before the remount still calls ext4_do_writepages(), which attempts to
> start a journal transaction.
>
> The WARN_ON_ONCE was added by commit e7fc2b31e04c ("ext4: warn on
> read-only filesystem in ext4_journal_check_start()") with the rationale
> that EXT4_FLAGS_SHUTDOWN should catch all cases first. However, normal
> admin-initiated remount-ro (shutdown path) does not set
> EXT4_FLAGS_SHUTDOWN, so the sb_rdonly() check is still reachable via
> in-flight writeback.
>
> The warning is not a functional issue — the -EROFS return correctly
> declines the journal start — but it produces a noisy stack trace on every
> affected shutdown.
Right. I think f4a2b42e7891 ("ext4: fix stale xarray tags after writeback")
should fix your issue. See the changelog for explanation.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH] ext4: Fix ERR_PTR(0) in ext4_mkdir()
From: Hongling Zeng @ 2026-05-20 7:46 UTC (permalink / raw)
To: tytso, adilger.kernel, libaokun, jack, ojaswin, ritesh.list,
yi.zhang, neil, brauner, jlayton
Cc: linux-ext4, linux-kernel, zhongling0719, Hongling Zeng
When mkdir succeeds, ext4_mkdir() returns ERR_PTR(0) which is incorrect.
It should return NULL instead for success and ERR_PTR() only with
negative error codes for failure.
Fixes: 88d5baf69082 ("Change inode_operations.mkdir to return struct dentry *")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
fs/ext4/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 4a47fbd8dd30..8cadaeb15b2b 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3054,7 +3054,7 @@ static struct dentry *ext4_mkdir(struct mnt_idmap *idmap, struct inode *dir,
out_retry:
if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries))
goto retry;
- return ERR_PTR(err);
+ return err ? ERR_PTR(err) : NULL;
}
/*
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v4 07/23] ext4: do not use data=ordered mode for inodes using buffered iomap path
From: Zhang Yi @ 2026-05-20 8:18 UTC (permalink / raw)
To: Ojaswin Mujoo
Cc: linux-ext4, linux-fsdevel, linux-kernel, tytso, adilger.kernel,
libaokun, jack, ritesh.list, djwong, hch, yi.zhang, yizhang089,
yangerkun, yukuai
In-Reply-To: <agxmG0MhIywySPaA@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On 5/19/2026 9:31 PM, Ojaswin Mujoo wrote:
> On Tue, May 19, 2026 at 04:11:30PM +0530, Ojaswin Mujoo wrote:
>> On Mon, May 11, 2026 at 03:23:27PM +0800, Zhang Yi wrote:
>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> The data=ordered mode introduces two fundamental conflicts with the
>>> iomap buffered write path, leading to potential deadlocks.
>>>
>>> 1) Lock ordering conflict
>>> In the iomap writeback path, each folio is processed sequentially:
>>> the folio lock is acquired first, followed by starting a transaction
>>> to create block mappings. In data=ordered mode, writeback triggered
>>> by the journal commit process may attempt to acquire a folio lock
>>> that is already held by iomap. Meanwhile, iomap, under that same
>>> folio lock, may start a new transaction and wait for the currently
>>> committing transaction to finish, resulting in a deadlock.
>>
>> Right, makes sense.
>>>
>>> 2) Partial folio submission not supported
>>> When block size is smaller than folio size, a folio may contain both
>>> mapped and unmapped blocks. In data=ordered mode, if the journal
>>> waits for such a folio to be written back while the regular writeback
>>> process has already started committing it (with the writeback flag
>>> set), mapping the remaining unmapped blocks can deadlock. This is
>>> because the writeback flag is cleared only after the entire folio is
>>> processed and committed.
>>
>> Okay so IIUC, if we do end up using iomap with ordered data, there are 2
>> codepaths with issues here:
>>
>> txn_commit
>> ordered data writeback (say it goes via iomap)
>> folio_lock
>> iomap_writeback_folio
>> folio_start_writeback
>> iomap_writeback_range
>> ext4_map_block
>> txn_start
>> wait for tnx commit - DEADLOCK
>>
>> Currently we avoid this by having ext4_normal_submit_inode_buffers()
>> pass can_map = 0 so journal flush makese sure not to start any txn.
>>
>> Then we have
>>
>> txn_commit background writeback (via iomap)
>>
>> folio_lock()
>> ordered data writeback
>> folio_lock
>>
>> iomap_writeback_folio
>> folio_start_writeback
>> iomap_writeback_range
>> ext4_map_block
>> txn_start
>> wait for txn commit - DEADLOCK
>
> Sorry I forget to remove tabs
>
> this is what I meant:
>
> txn_commit
> ordered data writeback (say it goes via iomap)
> folio_lock
> iomap_writeback_folio
> folio_start_writeback
> iomap_writeback_range
> ext4_map_block
> txn_start
> wait for tnx commit - DEADLOCK
>
> Currently we avoid this by having ext4_normal_submit_inode_buffers()
> pass can_map = 0 so journal flush makese sure not to start any txn.
Yeah, but we can also solve this problem by adding similar tags. This
is not the most difficult part.
>
> Then we have
>
> txn_commit background writeback (via iomap)
>
> folio_lock()
> ordered data writeback
> folio_lock
>
> iomap_writeback_folio
> folio_start_writeback
> iomap_writeback_range
> ext4_map_block
> txn_start
> wait for txn commit - DEADLOCK
>
>
>>
>> Currently, this is taken care because we try to start the txn before
>> taking any folio locks/starting writeback, and hence we cannot deadlock.
Yeah. You are right! Actually, this deadlock scenario should essentially
belong to the first category: "Lock ordering conflict". This is not the
scenario I want to describe here. The problematic scenario is as
follows:
T0: Assume we have a folio contains four blocks, from front to back,
they are A, B, C, D. The last block D is written in delalloc mode
(the block is not allocated yet).
T1: The writeback process starts to write back data, set writeback flag
on the folio, allocates block D, and adds it to transaction N's
order list of jbd2 in JI_WAIT_DATA mode.
T2: This folio completes the writeback and clears the writeback flag.
T3: Before transaction N commit, we punch block B and C, and overwrite
A-C,
T4: Transaction N commit and folio writeback are running concurrently.
Transaction N commit folio writeback(iomap)
iomap_writeback_folio()
folio_start_writeback() -- set writeback
jbd2_journal_finish_inode_data_buffers()
__filemap_fdatawait_range()
-- wait writeback flag to clear
iomap_writeback_range()
ext4_map_block() -- map block B and C
start handle
wait for transaction N commit
- DEADLOCK
IOMAP does not support submitting partial folios during writeback.
Therefore, the writeback flag is cleared only after the entire folio
has been submitted. As a result, the commit of transaction N would never
wait for this flag to be cleared if we need to map some blocks in this
folio.
Currently, this is handled by ext4_bio_write_folio(), which supports
writing back partial folios. The writeback flag is only set after the
block has been mapped and before the bio is actually issued. There are
no other limitations that would block this flag from being cleared
after the I/O is completed.
>>
>> If the above description makes sense, do you think it'd be good to add
>> them to the commit message. The reason is that although these paths seem
>> obvious when we look at them a lot, it took me a good bit of time to
>> understand what deadlocks you are talking about here :p
>>
>> Having the code traces like above makes it very clear.
Indeed, these problematic cases are complicated and subtle. I also spent
some time recalling this scene. I can add these code traces in my next
iteration.
Thanks,
Yi.
>>>
>>> To support data=ordered mode, the iomap core would need two invasive
>>> changes:
>>> - Acquire the transaction handle before locking any folio for
>>> writeback.
>>> - Support partial folio submission.
>>>
>>> Both changes are complicated and risk performance regressions.
>>> Therefore, we must avoid using data=ordered mode when converting to the
>>> iomap path.
>>>
>>> Currently, data=ordered mode is used in three scenarios:
>>> - Append write
>>> - Post-EOF partial block truncate-up followed by append write
>>> - Online defragmentation
>>>
>>> We can address the first two without data=ordered mode:
>>> - For append write: always allocate unwritten blocks (i.e. always
>>> enable dioread_nolock), preserving the behavior of current
>>> extent-type inodes.
>>> - For post-EOF truncate-up + append write: postpone updating i_disksize
>>> until after the zeroed partial block has been written back.
>>
>> I'm still going through how we are addressing no data=ordered so will
>> get back on this in some time.
>>
>> Thanks,
>> Ojaswin
>>
>>>
>>> Online defragmentation does not yet support iomap; this can be resolved
>>> separately in the future.
>>>
>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>> ---
>>> fs/ext4/ext4_jbd2.h | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
>>> index 63d17c5201b5..26999f173870 100644
>>> --- a/fs/ext4/ext4_jbd2.h
>>> +++ b/fs/ext4/ext4_jbd2.h
>>> @@ -383,7 +383,12 @@ static inline int ext4_should_journal_data(struct inode *inode)
>>>
>>> static inline int ext4_should_order_data(struct inode *inode)
>>> {
>>> - return ext4_inode_journal_mode(inode) & EXT4_INODE_ORDERED_DATA_MODE;
>>> + /*
>>> + * inodes using the iomap buffered I/O path do not use the
>>> + * data=ordered mode.
>>> + */
>>> + return !ext4_inode_buffered_iomap(inode) &&
>>> + (ext4_inode_journal_mode(inode) & EXT4_INODE_ORDERED_DATA_MODE);
>>> }
>>>
>>> static inline int ext4_should_writeback_data(struct inode *inode)
>>> --
>>> 2.52.0
>>>
^ permalink raw reply
* Re: [PATCH] ext4: Fix ERR_PTR(0) in ext4_mkdir()
From: Jan Kara @ 2026-05-20 8:42 UTC (permalink / raw)
To: Hongling Zeng
Cc: tytso, adilger.kernel, libaokun, jack, ojaswin, ritesh.list,
yi.zhang, neil, brauner, jlayton, linux-ext4, linux-kernel,
zhongling0719
In-Reply-To: <20260520074634.53656-1-zenghongling@kylinos.cn>
On Wed 20-05-26 15:46:34, Hongling Zeng wrote:
> When mkdir succeeds, ext4_mkdir() returns ERR_PTR(0) which is incorrect.
> It should return NULL instead for success and ERR_PTR() only with
> negative error codes for failure.
>
> Fixes: 88d5baf69082 ("Change inode_operations.mkdir to return struct dentry *")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
You're right this is a bit sloppy programming although there's no actual
functional difference at this point. So feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/namei.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 4a47fbd8dd30..8cadaeb15b2b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3054,7 +3054,7 @@ static struct dentry *ext4_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> out_retry:
> if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries))
> goto retry;
> - return ERR_PTR(err);
> + return err ? ERR_PTR(err) : NULL;
> }
>
> /*
> --
> 2.25.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] ext4: Fix ERR_PTR(0) in ext4_mkdir()
From: Zhang Yi @ 2026-05-20 9:19 UTC (permalink / raw)
To: Hongling Zeng, tytso, adilger.kernel, libaokun, jack, ojaswin,
ritesh.list, neil, brauner, jlayton
Cc: linux-ext4, linux-kernel, zhongling0719
In-Reply-To: <20260520074634.53656-1-zenghongling@kylinos.cn>
On 5/20/2026 3:46 PM, Hongling Zeng wrote:
> When mkdir succeeds, ext4_mkdir() returns ERR_PTR(0) which is incorrect.
> It should return NULL instead for success and ERR_PTR() only with
> negative error codes for failure.
This point is indeed very easy to overlook. However, why not modify
other file systems as well? Commit 88d5baf69082 made changes not only
to ext4.
Thanks,
Yi.
>
> Fixes: 88d5baf69082 ("Change inode_operations.mkdir to return struct dentry *")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> fs/ext4/namei.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 4a47fbd8dd30..8cadaeb15b2b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3054,7 +3054,7 @@ static struct dentry *ext4_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> out_retry:
> if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries))
> goto retry;
> - return ERR_PTR(err);
> + return err ? ERR_PTR(err) : NULL;
> }
>
> /*
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox