Re: [f2fs-dev] [PATCH v2] f2fs: call __add_ino_entry out of the eviction path

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 08/06, Chao Yu via Linux-f2fs-devel wrote:
> On 8/5/26 09:28, Jaegeuk Kim via Linux-f2fs-devel wrote:
> > The f2fs_evict_inode() can be called during the direct reclaim path, but
> > __add_ino_entry requires allocating some memory. Since we don't need to
> > do that in that context, let's migrate it in other workqueue context.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  Change log from v1:
> >   - fix a bug on the wait logic
> > 
> >  fs/f2fs/checkpoint.c | 11 ++++++
> >  fs/f2fs/data.c       | 13 ++++++-
> >  fs/f2fs/f2fs.h       |  4 ++
> >  fs/f2fs/inode.c      | 91 +++++++++++++++++++++++++++++++++++++++++---
> >  fs/f2fs/super.c      |  9 ++++-
> >  5 files changed, 121 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 064f5b537423..4413eccb5ecb 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -766,6 +766,15 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> >  	spin_unlock(&im->ino_lock);
> >  }
> >  
> > +static void f2fs_wait_for_inode_record(struct f2fs_sb_info *sbi, int mode)
> > +{
> > +	if (mode != APPEND_INO && mode != UPDATE_INO)
> > +		return;
> > +
> > +	/* Let's wait for some pending updates for APPEND_INO and UPDATE_INO. */
> > +	flush_workqueue(sbi->evict_wq);
> > +}
> > +
> >  void f2fs_add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> >  {
> >  	/* add new dirty ino entry into list */
> > @@ -798,6 +807,8 @@ void f2fs_release_ino_entry(struct f2fs_sb_info *sbi, bool all)
> >  	for (i = all ? ORPHAN_INO : APPEND_INO; i < MAX_INO_ENTRY; i++) {
> >  		struct inode_management *im = &sbi->im[i];
> >  
> > +		f2fs_wait_for_inode_record(sbi, i);
> > +
> >  		spin_lock(&im->ino_lock);
> >  		list_for_each_entry_safe(e, tmp, &im->ino_list, list) {
> >  			list_del(&e->list);
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index c219ea76a3a7..6ae0eb37d20f 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -4558,13 +4558,24 @@ int f2fs_init_wq(struct f2fs_sb_info *sbi)
> >  {
> >  	sbi->wq = alloc_workqueue("f2fs_wq", WQ_UNBOUND | WQ_HIGHPRI,
> >  				  num_online_cpus());
> > -	return sbi->wq ? 0 : -ENOMEM;
> > +	if (!sbi->wq)
> > +		return -ENOMEM;
> > +
> > +	sbi->evict_wq = alloc_workqueue("f2fs_evict_wq",
> > +			WQ_UNBOUND | WQ_HIGHPRI, num_online_cpus());
> > +	if (!sbi->evict_wq) {
> > +		destroy_workqueue(sbi->wq);
> > +		return -ENOMEM;
> > +	}
> > +	return 0;
> >  }
> >  
> >  void f2fs_destroy_wq(struct f2fs_sb_info *sbi)
> >  {
> >  	if (sbi->wq)
> >  		destroy_workqueue(sbi->wq);
> > +	if (sbi->evict_wq)
> > +		destroy_workqueue(sbi->evict_wq);
> >  }
> >  
> >  int __init f2fs_init_bio_entry_cache(void)
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index c44908258dc1..54f9d3856b5c 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2016,6 +2016,8 @@ struct f2fs_sb_info {
> >  
> >  	struct workqueue_struct *wq;		/* bio completion workqueue */
> >  
> > +	struct workqueue_struct *evict_wq;	/* inode eviction workqueue */
> > +
> >  	/*
> >  	 * If we are in irq context, let's update error information into
> >  	 * on-disk superblock in the work.
> > @@ -3875,6 +3877,8 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
> >  void f2fs_remove_donate_inode(struct inode *inode);
> >  void f2fs_evict_inode(struct inode *inode);
> >  void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc);
> > +int f2fs_init_evict_inode_work(void);
> > +void f2fs_destroy_evict_inode_work(void);
> >  
> >  /*
> >   * namei.c
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index c95e0b126da4..0d30489cf083 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -24,6 +24,18 @@
> >  extern const struct address_space_operations f2fs_compress_aops;
> >  #endif
> >  
> > +#define NUM_PREALLOC_EVICT_INODE_WORK 8
> > +
> > +static struct kmem_cache *evict_inode_work_cache;
> > +static mempool_t *evict_inode_work_pool;
> > +
> > +struct evict_inode_work {
> > +	struct work_struct work;
> > +	struct f2fs_sb_info *sbi;
> > +	nid_t ino;
> > +	unsigned int add_ino_entry_bits;
> > +};
> > +
> >  void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> >  {
> >  	if (is_inode_flag_set(inode, FI_NEW_INODE))
> > @@ -637,6 +649,9 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
> >  		inode->i_fop = &f2fs_dir_operations;
> >  		inode->i_mapping->a_ops = &f2fs_dblock_aops;
> >  		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
> > +
> > +		/* Let's prepare APPEND/UPDATE_INO before future access. */
> > +		flush_workqueue(sbi->evict_wq);
> >  	} else if (S_ISLNK(inode->i_mode)) {
> >  		if (file_is_encrypt(inode))
> >  			inode->i_op = &f2fs_encrypted_symlink_inode_operations;
> > @@ -854,6 +869,25 @@ void f2fs_remove_donate_inode(struct inode *inode)
> >  	spin_unlock(&sbi->inode_lock[DONATE_INODE]);
> >  }
> >  
> > +static void f2fs_record_inode_state(struct f2fs_sb_info *sbi, nid_t ino,
> > +				    unsigned int bits)
> > +{
> > +	if (bits & BIT(APPEND_INO))
> > +		f2fs_add_ino_entry(sbi, ino, APPEND_INO);
> > +	if (bits & BIT(UPDATE_INO))
> > +		f2fs_add_ino_entry(sbi, ino, UPDATE_INO);
> > +}
> > +
> > +static void f2fs_evict_inode_work(struct work_struct *work)
> > +{
> > +	struct evict_inode_work *ew =
> > +		container_of(work, struct evict_inode_work, work);
> > +
> > +	f2fs_record_inode_state(ew->sbi, ew->ino, ew->add_ino_entry_bits);
> > +
> > +	mempool_free(ew, evict_inode_work_pool);
> > +}
> > +
> >  /*
> >   * Called at the last iput() if i_nlink is zero
> >   */
> > @@ -864,6 +898,7 @@ void f2fs_evict_inode(struct inode *inode)
> >  	nid_t xnid = fi->i_xattr_nid;
> >  	int err = 0;
> >  	bool freeze_protected = false;
> > +	unsigned int record_bits = 0;
> >  
> >  	f2fs_abort_atomic_write(inode, true);
> >  
> > @@ -1003,12 +1038,32 @@ void f2fs_evict_inode(struct inode *inode)
> >  							inode->i_ino);
> >  	if (xnid)
> >  		invalidate_mapping_pages(NODE_MAPPING(sbi), xnid, xnid);
> > -	if (inode->i_nlink) {
> > -		if (is_inode_flag_set(inode, FI_APPEND_WRITE))
> > -			f2fs_add_ino_entry(sbi, inode->i_ino, APPEND_INO);
> > -		if (is_inode_flag_set(inode, FI_UPDATE_WRITE))
> > -			f2fs_add_ino_entry(sbi, inode->i_ino, UPDATE_INO);
> > +
> > +	if (!inode->i_nlink)
> > +		goto skip_record;
> > +
> > +	if (is_inode_flag_set(inode, FI_APPEND_WRITE))
> > +		record_bits = BIT(APPEND_INO);
> > +	if (is_inode_flag_set(inode, FI_UPDATE_WRITE))
> > +		record_bits = BIT(UPDATE_INO);
> > +
> > +	if (!record_bits)
> > +		goto skip_record;
> > +
> > +	/* Let's do this in workqueue out of the direct reclaim path. */
> > +	if (current_is_kswapd()) {
> > +		f2fs_record_inode_state(sbi, inode->i_ino, record_bits);
> > +	} else {
> > +		struct evict_inode_work *ew =
> > +			mempool_alloc(evict_inode_work_pool, GFP_NOFS);
> > +
> > +		ew->sbi = sbi;
> > +		ew->ino = inode->i_ino;
> > +		ew->add_ino_entry_bits = record_bits;
> > +		INIT_WORK(&ew->work, f2fs_evict_inode_work);
> > +		queue_work(sbi->evict_wq, &ew->work);
> >  	}
> > +skip_record:
> 
> What do you think of wrapping above codes into a static function for cleanup?

Sumbitted another patch series having the refactoring patch. Thanks,

> 
> Thanks,
> 
> >  	if (is_inode_flag_set(inode, FI_FREE_NID)) {
> >  		f2fs_alloc_nid_failed(sbi, inode->i_ino);
> >  		clear_inode_flag(inode, FI_FREE_NID);
> > @@ -1079,3 +1134,29 @@ void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc)
> >  	/* iput will drop the inode object */
> >  	iput(inode);
> >  }
> > +
> > +int __init f2fs_init_evict_inode_work(void)
> > +{
> > +	evict_inode_work_cache =
> > +		kmem_cache_create("f2fs_evict_inode_work",
> > +				  sizeof(struct evict_inode_work), 0, 0, NULL);
> > +	if (!evict_inode_work_cache)
> > +		goto fail;
> > +	evict_inode_work_pool =
> > +		mempool_create_slab_pool(NUM_PREALLOC_EVICT_INODE_WORK,
> > +					 evict_inode_work_cache);
> > +	if (!evict_inode_work_pool)
> > +		goto fail_free_cache;
> > +	return 0;
> > +
> > +fail_free_cache:
> > +	kmem_cache_destroy(evict_inode_work_cache);
> > +fail:
> > +	return -ENOMEM;
> > +}
> > +
> > +void f2fs_destroy_evict_inode_work(void)
> > +{
> > +	mempool_destroy(evict_inode_work_pool);
> > +	kmem_cache_destroy(evict_inode_work_cache);
> > +}
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 67abbf7ab477..6b55c30f0daa 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -5763,10 +5763,16 @@ static int __init init_f2fs_fs(void)
> >  	err = f2fs_init_xattr_cache();
> >  	if (err)
> >  		goto free_casefold_cache;
> > -	err = register_filesystem(&f2fs_fs_type);
> > +	err = f2fs_init_evict_inode_work();
> >  	if (err)
> >  		goto free_xattr_cache;
> > +	err = register_filesystem(&f2fs_fs_type);
> > +	if (err)
> > +		goto free_evict_inode_cache;
> >  	return 0;
> > +
> > +free_evict_inode_cache:
> > +	f2fs_destroy_evict_inode_work();
> >  free_xattr_cache:
> >  	f2fs_destroy_xattr_cache();
> >  free_casefold_cache:
> > @@ -5809,6 +5815,7 @@ static int __init init_f2fs_fs(void)
> >  static void __exit exit_f2fs_fs(void)
> >  {
> >  	unregister_filesystem(&f2fs_fs_type);
> > +	f2fs_destroy_evict_inode_work();
> >  	f2fs_destroy_xattr_cache();
> >  	f2fs_destroy_casefold_cache();
> >  	f2fs_destroy_compress_cache();
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel