Re: [PATCH v3] ext4: cache full extents during mapping lookup

Hi Ted:

It looks like this patch is good to go, can we merge it?


> From: "Jan Kara"<[email protected]>
> Date:  Wed, Jul 29, 2026, 04:26
> Subject:  Re: [PATCH v3] ext4: cache full extents during mapping lookup
> To: "Fengnan Chang"<[email protected]>
> Cc: <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>
> On Thu 23-07-26 20:43:26, 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. Add an optional read-side lookup to
> > ext4_es_cache_extent().
> > 
> > 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 <[email protected]>
> > Suggested-by: Zhang Yi <[email protected]>
> > Reviewed-by: Zhang Yi <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Fengnan Chang <[email protected]>
> 
> Hum, you have taken an easy way out :) I was hoping to get rid of
> ext4_es_cache_extent() calls in ext4_map_query_blocks_next_in_leaf(),
> ext4_map_query_blocks(), and perhaps more places by a strategically placed
> calls in ext4_ext_map_blocks() and ext4_ind_map_blocks(). But this patch
> fixes your problem without adding too much technical debt so I'm fine with
> that. We can cleanup the ext4_es_cache_extent() calls later. The patch
> looks good to me so feel free to add:
> 
> Reviewed-by: Jan Kara <[email protected]>
> 
>                                                                 Honza
> 
> > ---
> >  fs/ext4/extents.c        | 17 +++++++++++++----
> >  fs/ext4/extents_status.c | 17 ++++++++++++++++-
> >  fs/ext4/extents_status.h |  2 +-
> >  fs/ext4/inode.c          | 15 +++++++++++----
> >  4 files changed, 41 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 91c97af64b317..85e213761a18d 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,13 +4311,15 @@ 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;
> >  
> >                  /*
> >                   * unwritten extents are treated as holes, except that
> >                   * 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(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..d43266c489785 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,13 @@ int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
> >          unsigned int status;
> >          int retval;
> >          unsigned int orig_mlen = map->m_len;
> > +        /*
> > +         * ext4_ext_map_blocks() may have already cached the full extent, so
> > +         * search first to avoid taking i_es_lock for writing. The indirect
> > +         * path does not prepopulate the ES tree after the initial lookup miss.
> > +         */
> > +        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 +600,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);
> >          } else {
> >                  retval = ext4_map_query_blocks_next_in_leaf(handle, inode, map,
> >                                                              orig_mlen);
> > -- 
> > 2.39.5 (Apple Git-154)
> -- 
> Jan Kara <[email protected]>
> SUSE Labs, CR
>