2023-07-18 08:54:16

by Chao Liu

[permalink] [raw]
Subject: [PATCH] f2fs: introduce two helper functions for the largest cached extent

From: Chao Liu <[email protected]>

This patch is a cleanup:
1. Merge __drop_largest_extent() since it has only one caller.
2. Introduce __notify_largest_extent_updated() and
__drop_largest_extent() to help manage largest and largest_update
in extent_tree.

Signed-off-by: Chao Liu <[email protected]>
---
fs/f2fs/extent_cache.c | 60 +++++++++++++++++++-----------------------
fs/f2fs/f2fs.h | 4 +--
2 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 0e2d49140c07f..45dfddd8c3ad0 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);
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 */
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;
- }
-}

void f2fs_init_read_extent_tree(struct inode *inode, struct page *ipage)
{
@@ -601,6 +599,15 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
return en;
}

+static void __notify_largest_extent_updated(struct extent_tree *et,
+ struct inode *inode)
+{
+ if (et->type == EX_READ && et->largest_updated) {
+ f2fs_mark_inode_dirty_sync(inode, true);
+ et->largest_updated = false;
+ }
+}
+
static void __update_extent_tree_range(struct inode *inode,
struct extent_info *tei, enum extent_type type)
{
@@ -612,7 +619,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 +642,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);
+ /* 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 +738,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);
set_inode_flag(inode, FI_NO_EXTENT);
}
}
@@ -742,10 +746,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;
- }
goto out_read_extent_cache;
update_age_extent_cache:
if (!tei->last_blocks)
@@ -758,9 +758,7 @@ static void __update_extent_tree_range(struct inode *inode,
insert_p, insert_parent, leftmost);
out_read_extent_cache:
write_unlock(&et->lock);
-
- if (updated)
- f2fs_mark_inode_dirty_sync(inode, true);
+ __notify_largest_extent_updated(et, inode);
}

#ifdef CONFIG_F2FS_FS_COMPRESSION
@@ -1092,7 +1090,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 +1098,11 @@ 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);
+ __notify_largest_extent_updated(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 */
--
2.25.1



2023-07-22 14:06:46

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH] f2fs: introduce two helper functions for the largest cached extent

On 2023/7/18 16:33, Chao Liu wrote:
> From: Chao Liu <[email protected]>
>
> This patch is a cleanup:
> 1. Merge __drop_largest_extent() since it has only one caller.
> 2. Introduce __notify_largest_extent_updated() and
> __drop_largest_extent() to help manage largest and largest_update
> in extent_tree.
>
> Signed-off-by: Chao Liu <[email protected]>
> ---
> fs/f2fs/extent_cache.c | 60 +++++++++++++++++++-----------------------
> fs/f2fs/f2fs.h | 4 +--
> 2 files changed, 29 insertions(+), 35 deletions(-)
>
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 0e2d49140c07f..45dfddd8c3ad0 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);
> 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 */
> 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;
> - }
> -}
>
> void f2fs_init_read_extent_tree(struct inode *inode, struct page *ipage)
> {
> @@ -601,6 +599,15 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
> return en;
> }
>
> +static void __notify_largest_extent_updated(struct extent_tree *et,
> + struct inode *inode)
> +{
> + if (et->type == EX_READ && et->largest_updated) {
> + f2fs_mark_inode_dirty_sync(inode, true);
> + et->largest_updated = false;
> + }
> +}
> +
> static void __update_extent_tree_range(struct inode *inode,
> struct extent_info *tei, enum extent_type type)
> {
> @@ -612,7 +619,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 +642,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);
> + /* 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 +738,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);
> set_inode_flag(inode, FI_NO_EXTENT);
> }
> }
> @@ -742,10 +746,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;
> - }
> goto out_read_extent_cache;
> update_age_extent_cache:
> if (!tei->last_blocks)
> @@ -758,9 +758,7 @@ static void __update_extent_tree_range(struct inode *inode,
> insert_p, insert_parent, leftmost);
> out_read_extent_cache:
> write_unlock(&et->lock);
> -
> - if (updated)
> - f2fs_mark_inode_dirty_sync(inode, true);
> + __notify_largest_extent_updated(et, inode);

et->largest_updated should be updated w/ &et->lock, this is why we checks
local variable @updated here to decide whether setting inode dirty or not.

Thanks,

> }
>
> #ifdef CONFIG_F2FS_FS_COMPRESSION
> @@ -1092,7 +1090,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 +1098,11 @@ 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);
> + __notify_largest_extent_updated(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 */