From: Zhang Yi <yizhang089@gmail.com>
To: changfengnan <changfengnan@bytedance.com>,
Zhang Yi <yi.zhang@huaweicloud.com>
Cc: linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
tytso@mit.edu, adilger.kernel@dilger.ca,
libaokun@linux.alibaba.com, jack@suse.cz, ojaswin@linux.ibm.com,
ritesh.list@gmail.com
Subject: Re: [PATCH] ext4: cache full extents during mapping lookup
Date: Wed, 22 Jul 2026 20:56:36 +0800 [thread overview]
Message-ID: <86f0fec6-c84c-4f86-b1f6-1ab7fda20658@gmail.com> (raw)
In-Reply-To: <d9210bcdf73fbe1ac8b6ec132865609a3ed68688.61b2aa58.70d3.491a.bf4d.2ab068c26e69@bytedance.com>
On 7/22/2026 11:00 AM, changfengnan wrote:
>
>> From: "Zhang Yi"<yi.zhang@huaweicloud.com>
>> Date: Tue, Jul 21, 2026, 15:43
>> Subject: Re: [PATCH] ext4: cache full extents during mapping lookup
>> To: "Fengnan Chang"<changfengnan@bytedance.com>
>> Cc: <linux-ext4@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <tytso@mit.edu>, <adilger.kernel@dilger.ca>, <libaokun@linux.alibaba.com>, <jack@suse.cz>, <ojaswin@linux.ibm.com>, <ritesh.list@gmail.com>
>> On 7/20/2026 3:51 PM, Fengnan Chang wrote:
>>> The extent status (ES) tree and extent tree leaf buffers are reclaimed
>>> independently. An ES entry can be reclaimed while the leaf buffer remains
>>> cached with BH_Verified set.
>>>
>>> When this happens, ext4_es_lookup_extent() misses. Since the leaf buffer
>>> is already verified, __read_extent_tree_block() returns it without calling
>>> ext4_cache_extents(). ext4_ext_map_blocks() returns the part of the extent
>>> covered by the request, and ext4_map_query_blocks() caches only that range.
>>> Reads to other blocks in the same extent then keep missing the ES tree and
>>> walking the extent tree.
>>>
>>> Cache the full extent in ext4_ext_map_blocks() before the returned mapping
>>> is limited to the requested range. Check the ES tree under the read lock
>>> first. If the full extent is not there, ext4_es_cache_extent() checks
>>> again under the write lock before inserting it.
>>>
>>> Do not do this when ext4_ext_map_blocks() is called with
>>> EXT4_GET_BLOCKS_CREATE or EXT4_EX_NOCACHE. NOCACHE paths can change extent
>>> mappings, so caching ranges outside the request can leave stale ES entries.
>>> Cache both written and unwritten extents, as ext4_cache_extents() does.
>>>
>>> Tested with fio 4K random direct reads using libaio at iodepth 128 on a
>>> 128 GiB file with 208 on-disk extents. The results are averages of three
>>> 10-second runs:
>>>
>>> before reclaim after reclaim
>>> unpatched 419.6k IOPS 274.6k IOPS
>>> patched 426.1k IOPS 423.7k IOPS
>>>
>>> Average completion latency went from 303.02 to 462.78 us without the patch,
>>> and from 298.36 to 300.07 us with the patch.
>>>
>>> No latency regression was seen in A/B test.
>>>
>>> Suggested-by: Jan Kara <jack@suse.cz>
>>> Link: https://lore.kernel.org/r/20260714114358.93335-1-changfengnan@bytedance.com
>>> Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
>>
>> Thank you for the patch! This overall looks good to me, just some minor
>> suggestions below.
>>
>>> ---
>>> fs/ext4/extents.c | 9 +++++++++
>>> fs/ext4/extents_status.c | 33 +++++++++++++++++++++++++++++++++
>>> fs/ext4/extents_status.h | 3 +++
>>> 3 files changed, 45 insertions(+)
>>>
>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>> index 91c97af64b317..f43dd9c3bc44e 100644
>>> --- a/fs/ext4/extents.c
>>> +++ b/fs/ext4/extents.c
>>> @@ -4310,6 +4310,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>>> ext4_lblk_t ee_block = le32_to_cpu(ex->ee_block);
>>> ext4_fsblk_t ee_start = ext4_ext_pblock(ex);
>>> unsigned short ee_len;
>>> + unsigned int status;
>>>
>>>
>>> /*
>>> @@ -4317,6 +4318,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>>> * we split out initialized portions during a write.
>>> */
>>> ee_len = ext4_ext_get_actual_len(ex);
>>> + status = ext4_ext_is_unwritten(ex) ?
>>> + EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>>>
>>> trace_ext4_ext_show_extent(inode, ee_block, ee_start, ee_len);
>>>
>>> @@ -4328,6 +4331,12 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>>> ext_debug(inode, "%u fit into %u:%d -> %llu\n",
>>> map->m_lblk, ee_block, ee_len, newblock);
>>>
>>> + if (!(flags & (EXT4_GET_BLOCKS_CREATE |
>>> + EXT4_EX_NOCACHE)))
>>> + ext4_es_cache_extent_if_missing(inode, ee_block,
>>> + ee_len, ee_start,
>>> + status);
>>
>> After this patch, I think the calls to ext4_es_cache_extent() in
>> ext4_map_query_blocks_next_in_leaf() and ext4_map_query_blocks() can
>> also be replaced with this helper.
>>
>>> +
>>> /*
>>> * If the extent is initialized check whether the
>>> * caller wants to convert it to unwritten.
>>> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
>>> index 6e4a191e82191..83fe2068ab656 100644
>>> --- a/fs/ext4/extents_status.c
>>> +++ b/fs/ext4/extents_status.c
>>> @@ -1082,6 +1082,39 @@ void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
>>> ext4_es_pblock(&chkes), ext4_es_status(&chkes));
>>> }
>>>
>>> +/*
>>> + * Avoid taking i_es_lock for writing if the entire extent is already cached.
>>> + * ext4_es_cache_extent() rechecks under the write lock if the cache changes
>>> + * after this read-side check.
>>> + */
>>> +void ext4_es_cache_extent_if_missing(struct inode *inode, ext4_lblk_t lblk,
>>
>> I'd prefer to move this logic into ext4_es_cache_extent() and add a
>> parameter(e.g., read_search) to control whether to do pre-search unbder
>> the read lock. If the extent is likely to already exist on the status
>> tree, we can pass 1. Sounds good?
>
> ext4_es_cache_extent add a parameter to control whether to do pre-search
> sounds good.
> I have some questions about when to perform a pre-search.
> Are you meaning that a pre-search should also be performed when
> ext4_map_query_blocks_next_in_leaf and ext4_map_query_blocks call
> ext4_es_cache_extent ?
> ext4_map_query_blocks_next_in_leaf is ok , but I think it only makes sense
> to perform a pre-search for ext4_es_cache_extent in ext4_map_query_blocks
> when in extent mode, because in extent mode, the entire leaf is cached, so
> it’s likely that the extent already exists—but this isn’t the case in indirect mode.
> Right ?
Yes.
>
> I’d like to make the following change:
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 91c97af64b317..2ffb5027aa1ee 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -537,12 +537,12 @@ static void ext4_cache_extents(struct inode *inode,
>
> if (prev && (prev != lblk))
> ext4_es_cache_extent(inode, prev, lblk - prev, ~0,
> - EXTENT_STATUS_HOLE);
> + EXTENT_STATUS_HOLE, false);
>
> if (ext4_ext_is_unwritten(ex))
> status = EXTENT_STATUS_UNWRITTEN;
> ext4_es_cache_extent(inode, lblk, len,
> - ext4_ext_pblock(ex), status);
> + ext4_ext_pblock(ex), status, false);
> prev = lblk + len;
> }
> }
> @@ -4239,7 +4239,8 @@ static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
> insert_hole:
> /* Put just found gap into cache to speed up subsequent requests */
> ext_debug(inode, " -> %u:%u\n", hole_start, len);
> - ext4_es_cache_extent(inode, hole_start, len, ~0, EXTENT_STATUS_HOLE);
> + ext4_es_cache_extent(inode, hole_start, len, ~0, EXTENT_STATUS_HOLE,
> + false);
>
> /* Update hole_len to reflect hole size after lblk */
> if (hole_start != lblk)
> @@ -4310,6 +4311,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> ext4_lblk_t ee_block = le32_to_cpu(ex->ee_block);
> ext4_fsblk_t ee_start = ext4_ext_pblock(ex);
> unsigned short ee_len;
> + unsigned int status;
>
>
> /*
> @@ -4317,6 +4319,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> * we split out initialized portions during a write.
> */
> ee_len = ext4_ext_get_actual_len(ex);
> + status = ext4_ext_is_unwritten(ex) ?
> + EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>
> trace_ext4_ext_show_extent(inode, ee_block, ee_start, ee_len);
>
> @@ -4328,6 +4332,12 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> ext_debug(inode, "%u fit into %u:%d -> %llu\n",
> map->m_lblk, ee_block, ee_len, newblock);
>
> + if (!(flags & (EXT4_GET_BLOCKS_CREATE |
> + EXT4_EX_NOCACHE)))
> + ext4_es_cache_extent(inode, ee_block, ee_len,
> + ee_start, status,
> + true);
> +
> /*
> * If the extent is initialized check whether the
> * caller wants to convert it to unwritten.
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 6e4a191e82191..5a03878b06160 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1023,7 +1023,7 @@ void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> */
> void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
> ext4_lblk_t len, ext4_fsblk_t pblk,
> - unsigned int status)
> + unsigned int status, bool pre_search)
> {
> struct extent_status *es;
> struct extent_status chkes, newes;
> @@ -1043,6 +1043,21 @@ void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
>
> BUG_ON(end < lblk);
>
> + /*
> + * Avoid taking i_es_lock for writing if the entire extent is already
> + * cached. The write-locked search below rechecks after a miss.
> + */
> + if (pre_search) {
> + read_lock(&EXT4_I(inode)->i_es_lock);
> + es = __es_tree_search(&EXT4_I(inode)->i_es_tree.root, lblk);
> + if (es && es->es_lblk <= lblk && ext4_es_end(es) >= end &&
> + !__es_check_extent_status(es, status, NULL)) {
> + read_unlock(&EXT4_I(inode)->i_es_lock);
> + return;
> + }
> + read_unlock(&EXT4_I(inode)->i_es_lock);
> + }
> +
> write_lock(&EXT4_I(inode)->i_es_lock);
> es = __es_tree_search(&EXT4_I(inode)->i_es_tree.root, lblk);
> if (es && es->es_lblk <= end) {
> diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
> index f3396cf32b446..c2da72e3c82ba 100644
> --- a/fs/ext4/extents_status.h
> +++ b/fs/ext4/extents_status.h
> @@ -139,7 +139,7 @@ extern void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> bool delalloc_reserve_used);
> extern void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
> ext4_lblk_t len, ext4_fsblk_t pblk,
> - unsigned int status);
> + unsigned int status, bool pre_search);
> extern void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> ext4_lblk_t len);
> extern void ext4_es_find_extent_range(struct inode *inode,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ce99807c5f5b2..341b7c649e133 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -523,7 +523,7 @@ static int ext4_map_query_blocks_next_in_leaf(handle_t *handle,
>
> if (retval <= 0) {
> ext4_es_cache_extent(inode, map->m_lblk, map->m_len,
> - map->m_pblk, status);
> + map->m_pblk, status, true);
> return map->m_len;
> }
>
> @@ -546,11 +546,11 @@ static int ext4_map_query_blocks_next_in_leaf(handle_t *handle,
> status == status2) {
> ext4_es_cache_extent(inode, map->m_lblk,
> map->m_len + map2.m_len, map->m_pblk,
> - status);
> + status, true);
> map->m_len += map2.m_len;
> } else {
> ext4_es_cache_extent(inode, map->m_lblk, map->m_len,
> - map->m_pblk, status);
> + map->m_pblk, status, true);
> }
>
> return map->m_len;
> @@ -562,6 +562,8 @@ int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
> unsigned int status;
> int retval;
> unsigned int orig_mlen = map->m_len;
> + bool pre_search = ext4_test_inode_flag(inode,
> + EXT4_INODE_EXTENTS);
>
> flags &= EXT4_EX_QUERY_FILTER;
> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> @@ -593,7 +595,7 @@ int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
> status = map->m_flags & EXT4_MAP_UNWRITTEN ?
> EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
> ext4_es_cache_extent(inode, map->m_lblk, map->m_len,
> - map->m_pblk, status);
> + map->m_pblk, status, pre_search);
It would be helpful to add a comment here. Otherwise, this looks good to
me.
Thanks,
Yi.
> } else {
> retval = ext4_map_query_blocks_next_in_leaf(handle, inode, map,
> orig_mlen);
> --
>
>>
>> Thanks,
>> Yi.
>>
>>> + ext4_lblk_t len, ext4_fsblk_t pblk,
>>> + unsigned int status)
>>> +{
>>> + struct extent_status *es;
>>> + ext4_lblk_t end;
>>> + bool cached;
>>> +
>>> + if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
>>> + return;
>>> +
>>> + if (!len)
>>> + return;
>>> +
>>> + end = lblk + len - 1;
>>> + if (WARN_ON_ONCE(end < lblk))
>>> + return;
>>> +
>>> + read_lock(&EXT4_I(inode)->i_es_lock);
>>> + es = __es_tree_search(&EXT4_I(inode)->i_es_tree.root, lblk);
>>> + cached = es && es->es_lblk <= lblk && ext4_es_end(es) >= end &&
>>> + !__es_check_extent_status(es, status, NULL);
>>> + read_unlock(&EXT4_I(inode)->i_es_lock);
>>> +
>>> + if (!cached)
>>> + ext4_es_cache_extent(inode, lblk, len, pblk, status);
>>> +}
>>> +
>>> /*
>>> * ext4_es_lookup_extent() looks up an extent in extent status tree.
>>> *
>>> diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
>>> index f3396cf32b446..d8fab84911352 100644
>>> --- a/fs/ext4/extents_status.h
>>> +++ b/fs/ext4/extents_status.h
>>> @@ -140,6 +140,9 @@ extern void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>>> extern void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
>>> ext4_lblk_t len, ext4_fsblk_t pblk,
>>> unsigned int status);
>>> +void ext4_es_cache_extent_if_missing(struct inode *inode, ext4_lblk_t lblk,
>>> + ext4_lblk_t len, ext4_fsblk_t pblk,
>>> + unsigned int status);
>>> extern void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>>> ext4_lblk_t len);
>>> extern void ext4_es_find_extent_range(struct inode *inode,
>>
>
prev parent reply other threads:[~2026-07-22 12:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-20 7:51 [PATCH] ext4: cache full extents during mapping lookup Fengnan Chang
2026-07-21 7:42 ` Zhang Yi
2026-07-22 3:00 ` changfengnan
2026-07-22 12:56 ` Zhang Yi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86f0fec6-c84c-4f86-b1f6-1ab7fda20658@gmail.com \
--to=yizhang089@gmail.com \
--cc=adilger.kernel@dilger.ca \
--cc=changfengnan@bytedance.com \
--cc=jack@suse.cz \
--cc=libaokun@linux.alibaba.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ojaswin@linux.ibm.com \
--cc=ritesh.list@gmail.com \
--cc=tytso@mit.edu \
--cc=yi.zhang@huaweicloud.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox