Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 26 Jul 2023 14:16:05 +0800
From:   Chao Liu <chaoliu719@gmail.com>
To:     Chao Yu <chao@kernel.org>
Cc:     Jaegeuk Kim <jaegeuk@kernel.org>,
        linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, Chao Liu <liuchao@coolpad.com>
Subject: Re: [PATCH v2] f2fs: introduce two helper functions for the largest
 cached extent
Message-ID: <ZMC6JfDFOo3WrRsC@liuchao-VM>
References: <20230725013607.4134123-1-chaoliu719@gmail.com>
 <16625fbb-3dc0-34d5-ee75-fe010aa0d9ec@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=gbk
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <16625fbb-3dc0-34d5-ee75-fe010aa0d9ec@kernel.org>
Precedence: bulk

On 7?? 26 09:24, Chao Yu wrote:
> On 2023/7/25 9:36, Chao Liu wrote:
> > From: Chao Liu <liuchao@coolpad.com>
> >
> > This patch is a cleanup:
> > 1. Merge __drop_largest_extent() since it has only one caller.
> > 2. Introduce __unlock_tree_with_checking_largest() and
> >     __drop_largest_extent() to help manage largest and largest_update
> >     in extent_tree.
> >
> > Signed-off-by: Chao Liu <liuchao@coolpad.com>
> > ---
> > v2: Make sure et->largest_updated gets updated within &et->lock.
> >      Thanks to Chao Yu for pointing out.
> > ---
> >   fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++----------------------
> >   fs/f2fs/f2fs.h         |  4 +--
> >   2 files changed, 33 insertions(+), 37 deletions(-)
> >
> > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> > index 0e2d49140c07f..cfc69621a8a26 100644
> > --- a/fs/f2fs/extent_cache.c
> > +++ b/fs/f2fs/extent_cache.c
> > @@ -19,6 +19,12 @@
> >   #include "node.h"
> >   #include <trace/events/f2fs.h>
> > +static void __drop_largest_extent(struct extent_tree *et)
> > +{
> > +	et->largest.len = 0;
> > +	et->largest_updated = true;
> > +}
> > +
> >   bool sanity_check_extent_cache(struct inode *inode)
> >   {
> >   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > @@ -35,8 +41,7 @@ bool sanity_check_extent_cache(struct inode *inode)
> >   	/* Let's drop, if checkpoint got corrupted. */
> >   	if (is_set_ckpt_flags(sbi, CP_ERROR_FLAG)) {
> > -		ei->len = 0;
> > -		et->largest_updated = true;
> > +		__drop_largest_extent(et);
>
> __drop_largest_extent_force(et);
>
> >   		return true;
> >   	}
> > @@ -310,6 +315,8 @@ static void __detach_extent_node(struct f2fs_sb_info *sbi,
> >   	if (et->cached_en == en)
> >   		et->cached_en = NULL;
> > +
> > +	/* keep the largest as we can still use it */
>
> The comments doesn't match below code?
>

Sorry for not explaining this earlier.

It's just a hint and has nothing to do with the code below. It
simply explains that we don't need to disable the largest here, which
makes the whole code logic of the largest more clear. :)

If it's not fitting, please let me know, and I'll drop them.

> >   	kmem_cache_free(extent_node_slab, en);
> >   }
> > @@ -385,15 +392,6 @@ static unsigned int __free_extent_tree(struct f2fs_sb_info *sbi,
> >   	return count - atomic_read(&et->node_cnt);
> >   }
> > -static void __drop_largest_extent(struct extent_tree *et,
> > -					pgoff_t fofs, unsigned int len)
> > -{
> > -	if (fofs < et->largest.fofs + et->largest.len &&
> > -			fofs + len > et->largest.fofs) {
> > -		et->largest.len = 0;
> > -		et->largest_updated = true;
> > -	}
> > -}
>
> What about:
>
> static void __drop_largest_extent_cond(struct extent_tree *et,
> 					pgoff_t fofs, unsigned int len,
> 					bool force)
> {
> 	if (force || (fofs < et->largest.fofs + et->largest.len &&
> 			fofs + len > et->largest.fofs)) {
> 		et->largest.len = 0;
> 		et->largest_updated = true;
> 	}
> }
>
> static void __drop_largest_extent_force(struct extent_tree *et)
> {
> 	__drop_largest_extent_cond(et, 0, 0, true);
> }
>

Thank you. I feel it matches better with the existing code
organization style. Let me apply it in v3.

> >   void f2fs_init_read_extent_tree(struct inode *inode, struct page *ipage)
> >   {
> > @@ -601,6 +599,19 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
> >   	return en;
> >   }
> > +static void __unlock_tree_with_checking_largest(struct extent_tree *et,
> > +						struct inode *inode)
> > +{
> > +	if (et->type == EX_READ && et->largest_updated) {
> > +		et->largest_updated = false;
> > +		write_unlock(&et->lock);
> > +		f2fs_mark_inode_dirty_sync(inode, true);
> > +		return;
> > +	}
> > +
> > +	write_unlock(&et->lock);
> > +}
> > +
> >   static void __update_extent_tree_range(struct inode *inode,
> >   			struct extent_info *tei, enum extent_type type)
> >   {
> > @@ -612,7 +623,6 @@ static void __update_extent_tree_range(struct inode *inode,
> >   	struct rb_node **insert_p = NULL, *insert_parent = NULL;
> >   	unsigned int fofs = tei->fofs, len = tei->len;
> >   	unsigned int end = fofs + len;
> > -	bool updated = false;
> >   	bool leftmost = false;
> >   	if (!et)
> > @@ -636,11 +646,10 @@ static void __update_extent_tree_range(struct inode *inode,
> >   		prev = et->largest;
> >   		dei.len = 0;
> > -		/*
> > -		 * drop largest extent before lookup, in case it's already
> > -		 * been shrunk from extent tree
> > -		 */
> > -		__drop_largest_extent(et, fofs, len);
>
> __drop_largest_extent_cond(et, fofs, len, false);
>
> > +		/* updates may cause largest extent cache to become invalid */
> > +		if (fofs < et->largest.fofs + et->largest.len &&
> > +		    fofs + len > et->largest.fofs)
> > +			__drop_largest_extent(et);
> >   	}
> >   	/* 1. lookup first extent node in range [fofs, fofs + len - 1] */
> > @@ -733,8 +742,7 @@ static void __update_extent_tree_range(struct inode *inode,
> >   		if (dei.len >= 1 &&
> >   				prev.len < F2FS_MIN_EXTENT_LEN &&
> >   				et->largest.len < F2FS_MIN_EXTENT_LEN) {
> > -			et->largest.len = 0;
> > -			et->largest_updated = true;
> > +			__drop_largest_extent(et);
>
> __drop_largest_extent_force(et);
>
> >   			set_inode_flag(inode, FI_NO_EXTENT);
> >   		}
> >   	}
> > @@ -742,10 +750,6 @@ static void __update_extent_tree_range(struct inode *inode,
> >   	if (is_inode_flag_set(inode, FI_NO_EXTENT))
> >   		__free_extent_tree(sbi, et);
> > -	if (et->largest_updated) {
> > -		et->largest_updated = false;
> > -		updated = true;
> > -	}
>
> I guess we'd better keep previous logic.

Ok, I will drop these changes in v3. Ditto for __drop_extent_tree().

>
> >   	goto out_read_extent_cache;
> >   update_age_extent_cache:
> >   	if (!tei->last_blocks)
> > @@ -757,10 +761,7 @@ static void __update_extent_tree_range(struct inode *inode,
> >   		__insert_extent_tree(sbi, et, &ei,
> >   					insert_p, insert_parent, leftmost);
> >   out_read_extent_cache:
> > -	write_unlock(&et->lock);
> > -
> > -	if (updated)
> > -		f2fs_mark_inode_dirty_sync(inode, true);
>
> Ditto,
>
> > +	__unlock_tree_with_checking_largest(et, inode);
> >   }
> >   #ifdef CONFIG_F2FS_FS_COMPRESSION
> > @@ -1092,7 +1093,6 @@ static void __drop_extent_tree(struct inode *inode, enum extent_type type)
> >   {
> >   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >   	struct extent_tree *et = F2FS_I(inode)->extent_tree[type];
> > -	bool updated = false;
> >   	if (!__may_extent_tree(inode, type))
> >   		return;
> > @@ -1101,14 +1101,10 @@ static void __drop_extent_tree(struct inode *inode, enum extent_type type)
> >   	__free_extent_tree(sbi, et);
> >   	if (type == EX_READ) {
> >   		set_inode_flag(inode, FI_NO_EXTENT);
> > -		if (et->largest.len) {
> > -			et->largest.len = 0;
> > -			updated = true;
> > -		}
> > +		if (et->largest.len)
> > +			__drop_largest_extent(et);
> >   	}
> > -	write_unlock(&et->lock);
> > -	if (updated)
> > -		f2fs_mark_inode_dirty_sync(inode, true);
>
> Ditto,
>
> Thanks,
>
> > +	__unlock_tree_with_checking_largest(et, inode);
> >   }
> >   void f2fs_drop_extent_tree(struct inode *inode)
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index d372bedb0fe4e..da02e120e5ea6 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -665,7 +665,7 @@ struct extent_tree {
> >   struct extent_tree_info {
> >   	struct radix_tree_root extent_tree_root;/* cache extent cache entries */
> > -	struct mutex extent_tree_lock;	/* locking extent radix tree */
> > +	struct mutex extent_tree_lock;		/* locking extent radix tree */
> >   	struct list_head extent_list;		/* lru list for shrinker */
> >   	spinlock_t extent_lock;			/* locking extent lru list */
> >   	atomic_t total_ext_tree;		/* extent tree count */
> > @@ -766,7 +766,7 @@ enum {
> >   	FI_ACL_MODE,		/* indicate acl mode */
> >   	FI_NO_ALLOC,		/* should not allocate any blocks */
> >   	FI_FREE_NID,		/* free allocated nide */
> > -	FI_NO_EXTENT,		/* not to use the extent cache */
> > +	FI_NO_EXTENT,		/* not to use the read extent cache */
> >   	FI_INLINE_XATTR,	/* used for inline xattr */
> >   	FI_INLINE_DATA,		/* used for inline data*/
> >   	FI_INLINE_DENTRY,	/* used for inline dentry */