Re: [PATCH 3/3] ext4: move dcache manipulation out of __ext4_link()

[Jan Kara <jack@suse.cz>]

On Wed 18-03-26 09:39:51, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> __ext4_link() has two callers.
> 
> - ext4_link() calls it during normal handling of the link() system
>   call or similar
> - ext4_fc_replay_link_internal() calls it when replaying the journal
>   at mount time.
> 
> The former needs changes to dcache - instaniating the dentry to the
					^^ instantiating

> inode on success.  The latter doesn't need or want any dcache
> manipluation.
  ^^ manipulation

> So move the manipulation out of __ext4_link() and do it in ext4_link()
> only.
> 
> This requires:
>  - passing the qname from the dentry explicitly to __ext4_link.
>    The parent dir is already passed.  The dentry is still passed
>    in the ext4_link() case purely for use by ext4_fc_track_link().
>  - passing the inode separately to ext4_fc_track_link() as the
>    dentry will not be instantiated yet.
>  - using __ext4_add_entry() in ext4_link, which doesn't need a dentry.
>  - moving ext4_inc_count(), ihold)(, d_instantiate(), drop_nlink() and
>    iput() calls out of __ext4_link() into ext4_link().
> 
> This substantially simplifies ext4_fc_replay_link_internal(), and
> removes a use of d_alloc() which, it is planned, will be removed.
> 
> Signed-off-by: NeilBrown <neil@brown.name>

Looks good to me! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ext4.h        |  5 +++--
>  fs/ext4/fast_commit.c | 32 ++++----------------------------
>  fs/ext4/namei.c       | 25 ++++++++++++++-----------
>  3 files changed, 21 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 293f698b7042..e757e9cf9728 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2974,7 +2974,8 @@ void __ext4_fc_track_unlink(handle_t *handle, struct inode *inode,
>  void __ext4_fc_track_link(handle_t *handle, struct inode *inode,
>  	struct dentry *dentry);
>  void ext4_fc_track_unlink(handle_t *handle, struct dentry *dentry);
> -void ext4_fc_track_link(handle_t *handle, struct dentry *dentry);
> +void ext4_fc_track_link(handle_t *handle, struct inode *inode,
> +			struct dentry *dentry);
>  void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
>  			    struct dentry *dentry);
>  void ext4_fc_track_create(handle_t *handle, struct dentry *dentry);
> @@ -3719,7 +3720,7 @@ extern int ext4_handle_dirty_dirblock(handle_t *handle, struct inode *inode,
>  extern int __ext4_unlink(struct inode *dir, const struct qstr *d_name,
>  			 struct inode *inode, struct dentry *dentry);
>  extern int __ext4_link(struct inode *dir, struct inode *inode,
> -		       struct dentry *dentry);
> +		       const struct qstr *d_name, struct dentry *dentry);
>  
>  #define S_SHIFT 12
>  static const unsigned char ext4_type_by_mode[(S_IFMT >> S_SHIFT) + 1] = {
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 3ee302b73f45..175dda11f377 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -497,10 +497,9 @@ void __ext4_fc_track_link(handle_t *handle,
>  	trace_ext4_fc_track_link(handle, inode, dentry, ret);
>  }
>  
> -void ext4_fc_track_link(handle_t *handle, struct dentry *dentry)
> +void ext4_fc_track_link(handle_t *handle, struct inode *inode,
> +			struct dentry *dentry)
>  {
> -	struct inode *inode = d_inode(dentry);
> -
>  	if (ext4_fc_eligible(inode->i_sb))
>  		__ext4_fc_track_link(handle, inode, dentry);
>  }
> @@ -1431,7 +1430,6 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
>  				struct inode *inode)
>  {
>  	struct inode *dir = NULL;
> -	struct dentry *dentry_dir = NULL, *dentry_inode = NULL;
>  	struct qstr qstr_dname = QSTR_INIT(darg->dname, darg->dname_len);
>  	int ret = 0;
>  
> @@ -1442,21 +1440,7 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
>  		goto out;
>  	}
>  
> -	dentry_dir = d_obtain_alias(dir);
> -	if (IS_ERR(dentry_dir)) {
> -		ext4_debug("Failed to obtain dentry");
> -		dentry_dir = NULL;
> -		goto out;
> -	}
> -
> -	dentry_inode = d_alloc(dentry_dir, &qstr_dname);
> -	if (!dentry_inode) {
> -		ext4_debug("Inode dentry not created.");
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
> -	ret = __ext4_link(dir, inode, dentry_inode);
> +	ret = __ext4_link(dir, inode, &qstr_dname, NULL);
>  	/*
>  	 * It's possible that link already existed since data blocks
>  	 * for the dir in question got persisted before we crashed OR
> @@ -1470,16 +1454,8 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
>  
>  	ret = 0;
>  out:
> -	if (dentry_dir) {
> -		d_drop(dentry_dir);
> -		dput(dentry_dir);
> -	} else if (dir) {
> +	if (dir)
>  		iput(dir);
> -	}
> -	if (dentry_inode) {
> -		d_drop(dentry_inode);
> -		dput(dentry_inode);
> -	}
>  
>  	return ret;
>  }
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 768036a109d7..23942320c515 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3452,7 +3452,8 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	return err;
>  }
>  
> -int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> +int __ext4_link(struct inode *dir, struct inode *inode,
> +		const struct qstr *d_name, struct dentry *dentry)
>  {
>  	handle_t *handle;
>  	int err, retries = 0;
> @@ -3467,10 +3468,8 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
>  		ext4_handle_sync(handle);
>  
>  	inode_set_ctime_current(inode);
> -	ext4_inc_count(inode);
> -	ihold(inode);
>  
> -	err = ext4_add_entry(handle, dentry, inode);
> +	err = __ext4_add_entry(handle, dir, d_name, inode);
>  	if (!err) {
>  		err = ext4_mark_inode_dirty(handle, inode);
>  		/* this can happen only for tmpfile being
> @@ -3478,11 +3477,8 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
>  		 */
>  		if (inode->i_nlink == 1)
>  			ext4_orphan_del(handle, inode);
> -		d_instantiate(dentry, inode);
> -		ext4_fc_track_link(handle, dentry);
> -	} else {
> -		drop_nlink(inode);
> -		iput(inode);
> +		if (dentry)
> +			ext4_fc_track_link(handle, inode, dentry);
>  	}
>  	ext4_journal_stop(handle);
>  	if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries))
> @@ -3511,9 +3507,16 @@ static int ext4_link(struct dentry *old_dentry,
>  	err = dquot_initialize(dir);
>  	if (err)
>  		return err;
> -	return __ext4_link(dir, inode, dentry);
> +	ext4_inc_count(inode);
> +	err = __ext4_link(dir, inode, &dentry->d_name, dentry);
> +	if (!err) {
> +		ihold(inode);
> +		d_instantiate(dentry, inode);
> +	} else {
> +		drop_nlink(inode);
> +	}
> +	return err;
>  }
> -
>  /*
>   * Try to find buffer head where contains the parent block.
>   * It should be the inode block if it is inlined or the 1st block
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR