| Message ID | 20230412124126.2286716-3-libaokun1@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: fix WARNING in ext4_da_update_reserve_space | expand |
On Wed 12-04-23 20:41:20, Baokun Li wrote: > A helper function is added to help determine if the current extent can > be dropped, although only ext4_es_is_delayed() extents cannot be dropped > currently. > > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Baokun Li <libaokun1@huawei.com> Looks good. Just some small suggestions below... > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c > index 7bc221038c6c..f9dab2510bdc 100644 > --- a/fs/ext4/extents_status.c > +++ b/fs/ext4/extents_status.c > @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode) > spin_unlock(&sbi->s_es_lock); > } > > +/* > + * Returns 1 indicates that we cannot fail to allocate memory for this > + * extent_status and cannot reclaim, clear, or free the extent until > + * its status changes. > + */ > +static inline int ext4_es_must_keep(struct extent_status *es) Maybe we can return bool? Also I'd rephrase the comment as: /* * Returns true if we cannot fail to allocate memory for this extent_status * entry and cannot reclaim it until its status changes. */ > +{ > + /* filemap, bigalloc, and seek_data/hole need to use it. */ > + if (ext4_es_is_delayed(es)) > + return 1; > + > + return 0; > +} > + > static struct extent_status * > ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len, > ext4_fsblk_t pblk) > @@ -460,10 +474,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len, > es->es_len = len; > es->es_pblk = pblk; > > - /* > - * We don't count delayed extent because we never try to reclaim them > - */ > - if (!ext4_es_is_delayed(es)) { > + /* We never try to reclaim a must kept extent, so we don't count it. */ > + if (!ext4_es_must_keep(es)) { > if (!EXT4_I(inode)->i_es_shk_nr++) > ext4_es_list_add(inode); > percpu_counter_inc(&EXT4_SB(inode->i_sb)-> > @@ -481,8 +493,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es) > EXT4_I(inode)->i_es_all_nr--; > percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt); > > - /* Decrease the shrink counter when this es is not delayed */ > - if (!ext4_es_is_delayed(es)) { > + /* Decrease the shrink counter when this es is not a must be kept */ Let's rephrase the comment as: /* Decrease the shrink counter when we can reclaim the extent */ > + if (!ext4_es_must_keep(es)) { > BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0); > if (!--EXT4_I(inode)->i_es_shk_nr) > ext4_es_list_del(inode); > @@ -853,7 +865,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, > if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb), > 128, EXT4_I(inode))) > goto retry; > - if (err == -ENOMEM && !ext4_es_is_delayed(&newes)) > + if (err == -ENOMEM && !ext4_es_must_keep(&newes)) > err = 0; > > if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) && > @@ -1706,11 +1718,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end, > > (*nr_to_scan)--; > node = rb_next(&es->rb_node); > - /* > - * We can't reclaim delayed extent from status tree because > - * fiemap, bigallic, and seek_data/hole need to use it. > - */ > - if (ext4_es_is_delayed(es)) > + /* We can't reclaim a must be kept extent from status tree. */ I guess we can just drop this comment. The function name explains enough... > + if (ext4_es_must_keep(es)) > goto next; > if (ext4_es_is_referenced(es)) { > ext4_es_clear_referenced(es); Honza
On 2023/4/13 2:53, Jan Kara wrote: > On Wed 12-04-23 20:41:20, Baokun Li wrote: >> A helper function is added to help determine if the current extent can >> be dropped, although only ext4_es_is_delayed() extents cannot be dropped >> currently. >> >> Suggested-by: Jan Kara <jack@suse.cz> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> > Looks good. Just some small suggestions below... Thank you very much for your review! > >> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c >> index 7bc221038c6c..f9dab2510bdc 100644 >> --- a/fs/ext4/extents_status.c >> +++ b/fs/ext4/extents_status.c >> @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode) >> spin_unlock(&sbi->s_es_lock); >> } >> >> +/* >> + * Returns 1 indicates that we cannot fail to allocate memory for this >> + * extent_status and cannot reclaim, clear, or free the extent until >> + * its status changes. >> + */ >> +static inline int ext4_es_must_keep(struct extent_status *es) > Maybe we can return bool? Also I'd rephrase the comment as: Totally agree! I tried to move it to fs/ext4/extents_status.h before, so I changed the function type to int, but later I realized that it was not necessary to move it to the header file, but the function type was not changed back. > > /* > * Returns true if we cannot fail to allocate memory for this extent_status > * entry and cannot reclaim it until its status changes. > */ OK, looks good, thanks! >> +{ >> + /* filemap, bigalloc, and seek_data/hole need to use it. */ >> + if (ext4_es_is_delayed(es)) >> + return 1; >> + >> + return 0; >> +} >> + >> static struct extent_status * >> ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len, >> ext4_fsblk_t pblk) >> @@ -460,10 +474,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len, >> es->es_len = len; >> es->es_pblk = pblk; >> >> - /* >> - * We don't count delayed extent because we never try to reclaim them >> - */ >> - if (!ext4_es_is_delayed(es)) { >> + /* We never try to reclaim a must kept extent, so we don't count it. */ >> + if (!ext4_es_must_keep(es)) { >> if (!EXT4_I(inode)->i_es_shk_nr++) >> ext4_es_list_add(inode); >> percpu_counter_inc(&EXT4_SB(inode->i_sb)-> >> @@ -481,8 +493,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es) >> EXT4_I(inode)->i_es_all_nr--; >> percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt); >> >> - /* Decrease the shrink counter when this es is not delayed */ >> - if (!ext4_es_is_delayed(es)) { >> + /* Decrease the shrink counter when this es is not a must be kept */ > Let's rephrase the comment as: > /* Decrease the shrink counter when we can reclaim the extent */ Okay, this is very nice! > >> + if (!ext4_es_must_keep(es)) { >> BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0); >> if (!--EXT4_I(inode)->i_es_shk_nr) >> ext4_es_list_del(inode); >> @@ -853,7 +865,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, >> if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb), >> 128, EXT4_I(inode))) >> goto retry; >> - if (err == -ENOMEM && !ext4_es_is_delayed(&newes)) >> + if (err == -ENOMEM && !ext4_es_must_keep(&newes)) >> err = 0; >> >> if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) && >> @@ -1706,11 +1718,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end, >> >> (*nr_to_scan)--; >> node = rb_next(&es->rb_node); >> - /* >> - * We can't reclaim delayed extent from status tree because >> - * fiemap, bigallic, and seek_data/hole need to use it. >> - */ >> - if (ext4_es_is_delayed(es)) >> + /* We can't reclaim a must be kept extent from status tree. */ > I guess we can just drop this comment. The function name explains enough... Totally agree! > >> + if (ext4_es_must_keep(es)) >> goto next; >> if (ext4_es_is_referenced(es)) { >> ext4_es_clear_referenced(es); > Honza Can you please help review the remaining patches for any problems? If you have any suggestions, I'll fix them together and post another version of v4. Thanks again for your review!
On Thu 13-04-23 10:00:56, Baokun Li wrote: > Can you please help review the remaining patches for any problems? > If you have any suggestions, I'll fix them together and post another version > of v4. Yes, I've got a bit stuck thinking about the GFP_NOFAIL problem and how to best solve it. It will require some changes but most of the patchset can stay I believe. Honza
On 2023/4/13 18:34, Jan Kara wrote: > On Thu 13-04-23 10:00:56, Baokun Li wrote: >> Can you please help review the remaining patches for any problems? >> If you have any suggestions, I'll fix them together and post another version >> of v4. > Yes, I've got a bit stuck thinking about the GFP_NOFAIL problem and how to > best solve it. It will require some changes but most of the patchset can > stay I believe. > > Honza OK! Thank you so much! That's a tricky one indeed.
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c index 7bc221038c6c..f9dab2510bdc 100644 --- a/fs/ext4/extents_status.c +++ b/fs/ext4/extents_status.c @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode) spin_unlock(&sbi->s_es_lock); } +/* + * Returns 1 indicates that we cannot fail to allocate memory for this + * extent_status and cannot reclaim, clear, or free the extent until + * its status changes. + */ +static inline int ext4_es_must_keep(struct extent_status *es) +{ + /* filemap, bigalloc, and seek_data/hole need to use it. */ + if (ext4_es_is_delayed(es)) + return 1; + + return 0; +} + static struct extent_status * ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len, ext4_fsblk_t pblk) @@ -460,10 +474,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len, es->es_len = len; es->es_pblk = pblk; - /* - * We don't count delayed extent because we never try to reclaim them - */ - if (!ext4_es_is_delayed(es)) { + /* We never try to reclaim a must kept extent, so we don't count it. */ + if (!ext4_es_must_keep(es)) { if (!EXT4_I(inode)->i_es_shk_nr++) ext4_es_list_add(inode); percpu_counter_inc(&EXT4_SB(inode->i_sb)-> @@ -481,8 +493,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es) EXT4_I(inode)->i_es_all_nr--; percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt); - /* Decrease the shrink counter when this es is not delayed */ - if (!ext4_es_is_delayed(es)) { + /* Decrease the shrink counter when this es is not a must be kept */ + if (!ext4_es_must_keep(es)) { BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0); if (!--EXT4_I(inode)->i_es_shk_nr) ext4_es_list_del(inode); @@ -853,7 +865,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb), 128, EXT4_I(inode))) goto retry; - if (err == -ENOMEM && !ext4_es_is_delayed(&newes)) + if (err == -ENOMEM && !ext4_es_must_keep(&newes)) err = 0; if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) && @@ -1706,11 +1718,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end, (*nr_to_scan)--; node = rb_next(&es->rb_node); - /* - * We can't reclaim delayed extent from status tree because - * fiemap, bigallic, and seek_data/hole need to use it. - */ - if (ext4_es_is_delayed(es)) + /* We can't reclaim a must be kept extent from status tree. */ + if (ext4_es_must_keep(es)) goto next; if (ext4_es_is_referenced(es)) { ext4_es_clear_referenced(es); @@ -1774,7 +1783,7 @@ void ext4_clear_inode_es(struct inode *inode) while (node) { es = rb_entry(node, struct extent_status, rb_node); node = rb_next(node); - if (!ext4_es_is_delayed(es)) { + if (!ext4_es_must_keep(es)) { rb_erase(&es->rb_node, &tree->root); ext4_es_free_extent(inode, es); }
A helper function is added to help determine if the current extent can be dropped, although only ext4_es_is_delayed() extents cannot be dropped currently. Suggested-by: Jan Kara <jack@suse.cz> Signed-off-by: Baokun Li <libaokun1@huawei.com> --- V2->V3: Rename ext4_es_alloc_should_nofail to ext4_es_must_keep. fs/ext4/extents_status.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)