* [PATCH v2] f2fs: remove sleep from atomic lock
@ 2018-09-09 4:05 Zhikang Zhang
2018-09-09 6:20 ` Chao Yu
0 siblings, 1 reply; 3+ messages in thread
From: Zhikang Zhang @ 2018-09-09 4:05 UTC (permalink / raw)
To: jaegeuk, yuchao0; +Cc: miaoxie, linux-f2fs-devel
In the call trace below, we might sleep in function dput().
So in order to avoid sleeping in atomic lock, we remove f2fs_mark_inode_dirty_sync
in __try_update_largest_extent && __drop_largest_extent.
BUG: sleeping function called from invalid context at fs/dcache.c:796
Call trace:
dump_backtrace+0x0/0x3f4
show_stack+0x24/0x30
dump_stack+0xe0/0x138
___might_sleep+0x2a8/0x2c8
__might_sleep+0x78/0x10c
dput+0x7c/0x750
block_dump___mark_inode_dirty+0x120/0x17c
__mark_inode_dirty+0x344/0x11f0
f2fs_mark_inode_dirty_sync+0x40/0x50
__insert_extent_tree+0x2e0/0x2f4
f2fs_update_extent_tree_range+0xcf4/0xde8
f2fs_update_extent_cache+0x114/0x12c
f2fs_update_data_blkaddr+0x40/0x50
write_data_page+0x150/0x314
do_write_data_page+0x648/0x2318
__write_data_page+0xdb4/0x1640
f2fs_write_cache_pages+0x768/0xafc
__f2fs_write_data_pages+0x590/0x1218
f2fs_write_data_pages+0x64/0x74
do_writepages+0x74/0xe4
__writeback_single_inode+0xdc/0x15f0
writeback_sb_inodes+0x574/0xc98
__writeback_inodes_wb+0x190/0x204
wb_writeback+0x730/0xf14
wb_check_old_data_flush+0x1bc/0x1c8
wb_workfn+0x554/0xf74
process_one_work+0x440/0x118c
worker_thread+0xac/0x974
kthread+0x1a0/0x1c8
ret_from_fork+0x10/0x1c
---
fs/f2fs/extent_cache.c | 31 ++++++++++++++++---------------
fs/f2fs/f2fs.h | 8 +++-----
2 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 231b77e..8cc5cba 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -313,10 +313,8 @@ static void __drop_largest_extent(struct inode *inode,
{
struct extent_info *largest = &F2FS_I(inode)->extent_tree->largest;
- if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs) {
+ if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs)
largest->len = 0;
- f2fs_mark_inode_dirty_sync(inode, true);
- }
}
/* return true, if inode page is changed */
@@ -416,12 +414,11 @@ static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
return ret;
}
-static struct extent_node *__try_merge_extent_node(struct inode *inode,
+static struct extent_node *__try_merge_extent_node(struct f2fs_sb_info *sbi,
struct extent_tree *et, struct extent_info *ei,
struct extent_node *prev_ex,
struct extent_node *next_ex)
{
- struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct extent_node *en = NULL;
if (prev_ex && __is_back_mergeable(ei, &prev_ex->ei)) {
@@ -443,7 +440,7 @@ static struct extent_node *__try_merge_extent_node(struct inode *inode,
if (!en)
return NULL;
- __try_update_largest_extent(inode, et, en);
+ __try_update_largest_extent(et, en);
spin_lock(&sbi->extent_lock);
if (!list_empty(&en->list)) {
@@ -454,12 +451,11 @@ static struct extent_node *__try_merge_extent_node(struct inode *inode,
return en;
}
-static struct extent_node *__insert_extent_tree(struct inode *inode,
+static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
struct extent_tree *et, struct extent_info *ei,
struct rb_node **insert_p,
struct rb_node *insert_parent)
{
- struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct rb_node **p;
struct rb_node *parent = NULL;
struct extent_node *en = NULL;
@@ -476,7 +472,7 @@ static struct extent_node *__insert_extent_tree(struct inode *inode,
if (!en)
return NULL;
- __try_update_largest_extent(inode, et, en);
+ __try_update_largest_extent(et, en);
/* update in global extent list */
spin_lock(&sbi->extent_lock);
@@ -497,6 +493,7 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
struct rb_node **insert_p = NULL, *insert_parent = NULL;
unsigned int end = fofs + len;
unsigned int pos = (unsigned int)fofs;
+ unsigned int old_largest = et->largest.len;
if (!et)
return;
@@ -550,7 +547,7 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
set_extent_info(&ei, end,
end - dei.fofs + dei.blk,
org_end - end);
- en1 = __insert_extent_tree(inode, et, &ei,
+ en1 = __insert_extent_tree(sbi, et, &ei,
NULL, NULL);
next_en = en1;
} else {
@@ -570,7 +567,7 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
}
if (parts)
- __try_update_largest_extent(inode, et, en);
+ __try_update_largest_extent(et, en);
else
__release_extent_node(sbi, et, en);
@@ -590,15 +587,15 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
if (blkaddr) {
set_extent_info(&ei, fofs, blkaddr, len);
- if (!__try_merge_extent_node(inode, et, &ei, prev_en, next_en))
- __insert_extent_tree(inode, et, &ei,
+ if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
+ __insert_extent_tree(sbi, et, &ei,
insert_p, insert_parent);
/* give up extent_cache, if split and small updates happen */
if (dei.len >= 1 &&
prev.len < F2FS_MIN_EXTENT_LEN &&
et->largest.len < F2FS_MIN_EXTENT_LEN) {
- __drop_largest_extent(inode, 0, UINT_MAX);
+ et->largest.len = 0;
set_inode_flag(inode, FI_NO_EXTENT);
}
}
@@ -607,6 +604,9 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
__free_extent_tree(sbi, et);
write_unlock(&et->lock);
+
+ if (old_largest != et->largest.len)
+ f2fs_mark_inode_dirty_sync(inode, true);
}
unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
@@ -713,8 +713,9 @@ void f2fs_drop_extent_tree(struct inode *inode)
write_lock(&et->lock);
__free_extent_tree(sbi, et);
- __drop_largest_extent(inode, 0, UINT_MAX);
+ et->largest.len = 0;
write_unlock(&et->lock);
+ f2fs_mark_inode_dirty_sync(inode, true);
}
void f2fs_destroy_extent_tree(struct inode *inode)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index abf9256..eef3016 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -754,13 +754,11 @@ static inline bool __is_front_mergeable(struct extent_info *cur,
}
extern void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync);
-static inline void __try_update_largest_extent(struct inode *inode,
- struct extent_tree *et, struct extent_node *en)
+static inline void __try_update_largest_extent(struct extent_tree *et,
+ struct extent_node *en)
{
- if (en->ei.len > et->largest.len) {
+ if (en->ei.len > et->largest.len)
et->largest = en->ei;
- f2fs_mark_inode_dirty_sync(inode, true);
- }
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] f2fs: remove sleep from atomic lock
2018-09-09 4:05 [PATCH v2] f2fs: remove sleep from atomic lock Zhikang Zhang
@ 2018-09-09 6:20 ` Chao Yu
2018-09-10 1:34 ` 答复: " zhangzhikang
0 siblings, 1 reply; 3+ messages in thread
From: Chao Yu @ 2018-09-09 6:20 UTC (permalink / raw)
To: Zhikang Zhang, jaegeuk, yuchao0; +Cc: miaoxie, linux-f2fs-devel
Hi Zhikang,
It's completely trivial, but how about changing title as below, :)
f2fs: avoid sleeping under spin_lock
On 2018/9/9 12:05, Zhikang Zhang wrote:
> In the call trace below, we might sleep in function dput().
>
> So in order to avoid sleeping in atomic lock, we remove f2fs_mark_inode_dirty_sync
> in __try_update_largest_extent && __drop_largest_extent.
>
> BUG: sleeping function called from invalid context at fs/dcache.c:796
> Call trace:
> dump_backtrace+0x0/0x3f4
> show_stack+0x24/0x30
> dump_stack+0xe0/0x138
> ___might_sleep+0x2a8/0x2c8
> __might_sleep+0x78/0x10c
> dput+0x7c/0x750
> block_dump___mark_inode_dirty+0x120/0x17c
> __mark_inode_dirty+0x344/0x11f0
> f2fs_mark_inode_dirty_sync+0x40/0x50
> __insert_extent_tree+0x2e0/0x2f4
> f2fs_update_extent_tree_range+0xcf4/0xde8
> f2fs_update_extent_cache+0x114/0x12c
> f2fs_update_data_blkaddr+0x40/0x50
> write_data_page+0x150/0x314
> do_write_data_page+0x648/0x2318
> __write_data_page+0xdb4/0x1640
> f2fs_write_cache_pages+0x768/0xafc
> __f2fs_write_data_pages+0x590/0x1218
> f2fs_write_data_pages+0x64/0x74
> do_writepages+0x74/0xe4
> __writeback_single_inode+0xdc/0x15f0
> writeback_sb_inodes+0x574/0xc98
> __writeback_inodes_wb+0x190/0x204
> wb_writeback+0x730/0xf14
> wb_check_old_data_flush+0x1bc/0x1c8
> wb_workfn+0x554/0xf74
> process_one_work+0x440/0x118c
> worker_thread+0xac/0x974
> kthread+0x1a0/0x1c8
> ret_from_fork+0x10/0x1c
> ---
> fs/f2fs/extent_cache.c | 31 ++++++++++++++++---------------
> fs/f2fs/f2fs.h | 8 +++-----
> 2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 231b77e..8cc5cba 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -313,10 +313,8 @@ static void __drop_largest_extent(struct inode *inode,
> {
> struct extent_info *largest = &F2FS_I(inode)->extent_tree->largest;
>
> - if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs) {
> + if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs)
> largest->len = 0;
et->largest_updated = true;
> - f2fs_mark_inode_dirty_sync(inode, true);
> - }
> }
>
> /* return true, if inode page is changed */
> @@ -416,12 +414,11 @@ static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
> return ret;
> }
>
> -static struct extent_node *__try_merge_extent_node(struct inode *inode,
> +static struct extent_node *__try_merge_extent_node(struct f2fs_sb_info *sbi,
> struct extent_tree *et, struct extent_info *ei,
> struct extent_node *prev_ex,
> struct extent_node *next_ex)
> {
> - struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> struct extent_node *en = NULL;
>
> if (prev_ex && __is_back_mergeable(ei, &prev_ex->ei)) {
> @@ -443,7 +440,7 @@ static struct extent_node *__try_merge_extent_node(struct inode *inode,
> if (!en)
> return NULL;
>
> - __try_update_largest_extent(inode, et, en);
> + __try_update_largest_extent(et, en);
>
> spin_lock(&sbi->extent_lock);
> if (!list_empty(&en->list)) {
> @@ -454,12 +451,11 @@ static struct extent_node *__try_merge_extent_node(struct inode *inode,
> return en;
> }
>
> -static struct extent_node *__insert_extent_tree(struct inode *inode,
> +static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
> struct extent_tree *et, struct extent_info *ei,
> struct rb_node **insert_p,
> struct rb_node *insert_parent)
> {
> - struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> struct rb_node **p;
> struct rb_node *parent = NULL;
> struct extent_node *en = NULL;
> @@ -476,7 +472,7 @@ static struct extent_node *__insert_extent_tree(struct inode *inode,
> if (!en)
> return NULL;
>
> - __try_update_largest_extent(inode, et, en);
> + __try_update_largest_extent(et, en);
>
> /* update in global extent list */
> spin_lock(&sbi->extent_lock);
> @@ -497,6 +493,7 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
> struct rb_node **insert_p = NULL, *insert_parent = NULL;
> unsigned int end = fofs + len;
> unsigned int pos = (unsigned int)fofs;
> + unsigned int old_largest = et->largest.len;
>
> if (!et)
> return;
> @@ -550,7 +547,7 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
> set_extent_info(&ei, end,
> end - dei.fofs + dei.blk,
> org_end - end);
> - en1 = __insert_extent_tree(inode, et, &ei,
> + en1 = __insert_extent_tree(sbi, et, &ei,
> NULL, NULL);
> next_en = en1;
> } else {
> @@ -570,7 +567,7 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
> }
>
> if (parts)
> - __try_update_largest_extent(inode, et, en);
> + __try_update_largest_extent(et, en);
> else
> __release_extent_node(sbi, et, en);
>
> @@ -590,15 +587,15 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
> if (blkaddr) {
>
> set_extent_info(&ei, fofs, blkaddr, len);
> - if (!__try_merge_extent_node(inode, et, &ei, prev_en, next_en))
> - __insert_extent_tree(inode, et, &ei,
> + if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
> + __insert_extent_tree(sbi, et, &ei,
> insert_p, insert_parent);
>
> /* give up extent_cache, if split and small updates happen */
> if (dei.len >= 1 &&
> prev.len < F2FS_MIN_EXTENT_LEN &&
> et->largest.len < F2FS_MIN_EXTENT_LEN) {
> - __drop_largest_extent(inode, 0, UINT_MAX);
> + et->largest.len = 0;
et->largest_updated = true;
> set_inode_flag(inode, FI_NO_EXTENT);
> }
> }
> @@ -607,6 +604,9 @@ static void f2fs_update_extent_tree_range(struct inode *inode,
> __free_extent_tree(sbi, et);
>
> write_unlock(&et->lock);
There is no lock protection, so in race case, if there are other updaters
increase largest.len, and In case of old_largest == et->largest.len, we will
miss dirtying inode. So I think length of largest extent node based comparison
is not precise.
What about adding one more variable to indicate that largest extent was changed.
struct extent_tree {
....
bool largest_updated; /* largest extent updated */
};
f2fs_update_extent_tree_range()
{
write_lock(&et->lock);
....
if (et->largest_updated) {
et->largest_updated = false;
updated = true;
}
write_unlock(&et->lock);
if (updated)
f2fs_mark_inode_dirty_sync(inode, true);
}
> +
> + if (old_largest != et->largest.len)
> + f2fs_mark_inode_dirty_sync(inode, true);
> }
>
> unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int nr_shrink)
> @@ -713,8 +713,9 @@ void f2fs_drop_extent_tree(struct inode *inode)
>
> write_lock(&et->lock);
> __free_extent_tree(sbi, et);
> - __drop_largest_extent(inode, 0, UINT_MAX);
> + et->largest.len = 0;
if (et->largest.len) {
et->largest.len = 0;
udpated = true;
}
> write_unlock(&et->lock);
if (updated)
f2fs_mark_inode_dirty_sync(inode, true);
> + f2fs_mark_inode_dirty_sync(inode, true);
> }
>
> void f2fs_destroy_extent_tree(struct inode *inode)
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index abf9256..eef3016 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -754,13 +754,11 @@ static inline bool __is_front_mergeable(struct extent_info *cur,
> }
>
> extern void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync);
> -static inline void __try_update_largest_extent(struct inode *inode,
> - struct extent_tree *et, struct extent_node *en)
> +static inline void __try_update_largest_extent(struct extent_tree *et,
> + struct extent_node *en)
> {
> - if (en->ei.len > et->largest.len) {
> + if (en->ei.len > et->largest.len)
> et->largest = en->ei;
et->largest_updated = true;
Thanks,
> - f2fs_mark_inode_dirty_sync(inode, true);
> - }
> }
>
> /*
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* 答复: [PATCH v2] f2fs: remove sleep from atomic lock
2018-09-09 6:20 ` Chao Yu
@ 2018-09-10 1:34 ` zhangzhikang
0 siblings, 0 replies; 3+ messages in thread
From: zhangzhikang @ 2018-09-10 1:34 UTC (permalink / raw)
To: Chao Yu, jaegeuk, Yuchao (T); +Cc: miaoxie (A), linux-f2fs-devel
Hi Chao,
Thanks for your suggestion, I will send a new patch for this.
Regards,
Zhikang
On 2018/9/9 14:20, Chao Yu wrote:
> Hi Zhikang,
>
> It's completely trivial, but how about changing title as below, :)
>
> f2fs: avoid sleeping under spin_lock
>
> On 2018/9/9 12:05, Zhikang Zhang wrote:
> > In the call trace below, we might sleep in function dput().
> >
> > So in order to avoid sleeping in atomic lock, we remove
> > f2fs_mark_inode_dirty_sync in __try_update_largest_extent &&
> __drop_largest_extent.
> >
> > BUG: sleeping function called from invalid context at fs/dcache.c:796
> > Call trace:
> > dump_backtrace+0x0/0x3f4
> > show_stack+0x24/0x30
> > dump_stack+0xe0/0x138
> > ___might_sleep+0x2a8/0x2c8
> > __might_sleep+0x78/0x10c
> > dput+0x7c/0x750
> > block_dump___mark_inode_dirty+0x120/0x17c
> > __mark_inode_dirty+0x344/0x11f0
> > f2fs_mark_inode_dirty_sync+0x40/0x50
> > __insert_extent_tree+0x2e0/0x2f4
> > f2fs_update_extent_tree_range+0xcf4/0xde8
> > f2fs_update_extent_cache+0x114/0x12c
> > f2fs_update_data_blkaddr+0x40/0x50
> > write_data_page+0x150/0x314
> > do_write_data_page+0x648/0x2318
> > __write_data_page+0xdb4/0x1640
> > f2fs_write_cache_pages+0x768/0xafc
> > __f2fs_write_data_pages+0x590/0x1218
> > f2fs_write_data_pages+0x64/0x74
> > do_writepages+0x74/0xe4
> > __writeback_single_inode+0xdc/0x15f0
> > writeback_sb_inodes+0x574/0xc98
> > __writeback_inodes_wb+0x190/0x204
> > wb_writeback+0x730/0xf14
> > wb_check_old_data_flush+0x1bc/0x1c8
> > wb_workfn+0x554/0xf74
> > process_one_work+0x440/0x118c
> > worker_thread+0xac/0x974
> > kthread+0x1a0/0x1c8
> > ret_from_fork+0x10/0x1c
> > ---
> > fs/f2fs/extent_cache.c | 31 ++++++++++++++++---------------
> > fs/f2fs/f2fs.h | 8 +++-----
> > 2 files changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c index
> > 231b77e..8cc5cba 100644
> > --- a/fs/f2fs/extent_cache.c
> > +++ b/fs/f2fs/extent_cache.c
> > @@ -313,10 +313,8 @@ static void __drop_largest_extent(struct inode
> > *inode, {
> > struct extent_info *largest = &F2FS_I(inode)->extent_tree->largest;
> >
> > - if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs) {
> > + if (fofs < largest->fofs + largest->len && fofs + len >
> > +largest->fofs)
> > largest->len = 0;
>
> et->largest_updated = true;
>
> > - f2fs_mark_inode_dirty_sync(inode, true);
> > - }
> > }
> >
> > /* return true, if inode page is changed */ @@ -416,12 +414,11 @@
> > static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
> > return ret;
> > }
> >
> > -static struct extent_node *__try_merge_extent_node(struct inode
> > *inode,
> > +static struct extent_node *__try_merge_extent_node(struct
> > +f2fs_sb_info *sbi,
> > struct extent_tree *et, struct extent_info *ei,
> > struct extent_node *prev_ex,
> > struct extent_node *next_ex)
> > {
> > - struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > struct extent_node *en = NULL;
> >
> > if (prev_ex && __is_back_mergeable(ei, &prev_ex->ei)) { @@ -
> 443,7
> > +440,7 @@ static struct extent_node *__try_merge_extent_node(struct
> inode *inode,
> > if (!en)
> > return NULL;
> >
> > - __try_update_largest_extent(inode, et, en);
> > + __try_update_largest_extent(et, en);
> >
> > spin_lock(&sbi->extent_lock);
> > if (!list_empty(&en->list)) {
> > @@ -454,12 +451,11 @@ static struct extent_node
> *__try_merge_extent_node(struct inode *inode,
> > return en;
> > }
> >
> > -static struct extent_node *__insert_extent_tree(struct inode *inode,
> > +static struct extent_node *__insert_extent_tree(struct f2fs_sb_info
> > +*sbi,
> > struct extent_tree *et, struct extent_info *ei,
> > struct rb_node **insert_p,
> > struct rb_node *insert_parent)
> > {
> > - struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > struct rb_node **p;
> > struct rb_node *parent = NULL;
> > struct extent_node *en = NULL;
> > @@ -476,7 +472,7 @@ static struct extent_node
> *__insert_extent_tree(struct inode *inode,
> > if (!en)
> > return NULL;
> >
> > - __try_update_largest_extent(inode, et, en);
> > + __try_update_largest_extent(et, en);
> >
> > /* update in global extent list */
> > spin_lock(&sbi->extent_lock);
> > @@ -497,6 +493,7 @@ static void f2fs_update_extent_tree_range(struct
> inode *inode,
> > struct rb_node **insert_p = NULL, *insert_parent = NULL;
> > unsigned int end = fofs + len;
> > unsigned int pos = (unsigned int)fofs;
> > + unsigned int old_largest = et->largest.len;
> >
> > if (!et)
> > return;
> > @@ -550,7 +547,7 @@ static void f2fs_update_extent_tree_range(struct
> inode *inode,
> > set_extent_info(&ei, end,
> > end - dei.fofs + dei.blk,
> > org_end - end);
> > - en1 = __insert_extent_tree(inode, et, &ei,
> > + en1 = __insert_extent_tree(sbi, et, &ei,
> > NULL, NULL);
> > next_en = en1;
> > } else {
> > @@ -570,7 +567,7 @@ static void f2fs_update_extent_tree_range(struct
> inode *inode,
> > }
> >
> > if (parts)
> > - __try_update_largest_extent(inode, et, en);
> > + __try_update_largest_extent(et, en);
> > else
> > __release_extent_node(sbi, et, en);
> >
> > @@ -590,15 +587,15 @@ static void
> f2fs_update_extent_tree_range(struct inode *inode,
> > if (blkaddr) {
> >
> > set_extent_info(&ei, fofs, blkaddr, len);
> > - if (!__try_merge_extent_node(inode, et, &ei, prev_en,
> next_en))
> > - __insert_extent_tree(inode, et, &ei,
> > + if (!__try_merge_extent_node(sbi, et, &ei, prev_en,
> next_en))
> > + __insert_extent_tree(sbi, et, &ei,
> > insert_p, insert_parent);
> >
> > /* give up extent_cache, if split and small updates happen */
> > if (dei.len >= 1 &&
> > prev.len < F2FS_MIN_EXTENT_LEN &&
> > et->largest.len < F2FS_MIN_EXTENT_LEN) {
> > - __drop_largest_extent(inode, 0, UINT_MAX);
> > + et->largest.len = 0;
>
> et->largest_updated = true;
>
> > set_inode_flag(inode, FI_NO_EXTENT);
> > }
> > }
> > @@ -607,6 +604,9 @@ static void f2fs_update_extent_tree_range(struct
> inode *inode,
> > __free_extent_tree(sbi, et);
> >
> > write_unlock(&et->lock);
>
> There is no lock protection, so in race case, if there are other updaters
> increase largest.len, and In case of old_largest == et->largest.len, we will
> miss dirtying inode. So I think length of largest extent node based
> comparison is not precise.
>
> What about adding one more variable to indicate that largest extent was
> changed.
>
> struct extent_tree {
> ....
> bool largest_updated; /* largest extent updated */
> };
>
>
> f2fs_update_extent_tree_range()
> {
> write_lock(&et->lock);
>
> ....
>
> if (et->largest_updated) {
> et->largest_updated = false;
> updated = true;
> }
> write_unlock(&et->lock);
>
> if (updated)
> f2fs_mark_inode_dirty_sync(inode, true); }
> > +
> > + if (old_largest != et->largest.len)
> > + f2fs_mark_inode_dirty_sync(inode, true);
> > }
> >
> > unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int
> > nr_shrink) @@ -713,8 +713,9 @@ void f2fs_drop_extent_tree(struct inode
> > *inode)
> >
> > write_lock(&et->lock);
> > __free_extent_tree(sbi, et);
> > - __drop_largest_extent(inode, 0, UINT_MAX);
> > + et->largest.len = 0;
>
> if (et->largest.len) {
> et->largest.len = 0;
> udpated = true;
> }
>
> > write_unlock(&et->lock);
>
> if (updated)
> f2fs_mark_inode_dirty_sync(inode, true);
>
> > + f2fs_mark_inode_dirty_sync(inode, true);
> > }
> >
> > void f2fs_destroy_extent_tree(struct inode *inode) diff --git
> > a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index abf9256..eef3016 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -754,13 +754,11 @@ static inline bool __is_front_mergeable(struct
> > extent_info *cur, }
> >
> > extern void f2fs_mark_inode_dirty_sync(struct inode *inode, bool
> > sync); -static inline void __try_update_largest_extent(struct inode *inode,
> > - struct extent_tree *et, struct extent_node *en)
> > +static inline void __try_update_largest_extent(struct extent_tree *et,
> > + struct extent_node *en)
> > {
> > - if (en->ei.len > et->largest.len) {
> > + if (en->ei.len > et->largest.len)
> > et->largest = en->ei;
>
> et->largest_updated = true;
>
> Thanks,
>
> > - f2fs_mark_inode_dirty_sync(inode, true);
> > - }
> > }
> >
> > /*
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-10 1:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-09 4:05 [PATCH v2] f2fs: remove sleep from atomic lock Zhikang Zhang
2018-09-09 6:20 ` Chao Yu
2018-09-10 1:34 ` 答复: " zhangzhikang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.