[PATCH 0/3] ext4: remove use of d_alloc()

[PATCH 0/3] ext4: remove use of d_alloc()

[NeilBrown]

This is a revised version of the ext4 patches from my recent patchset
for revising directory locking.  My particular interest in changing ext4
is to remove the use of d_alloc().  I will want to deprecate d_alloc()
as it doesn't fit the new model well.

The use of d_alloc() in ext4 is incidental to the actual task at hand.
The code really wants to pass around a parent directory and a name, and
only uses the dentry because that seems convenient.  As these patches
show the code actually becomes simpler when we avoid the dentry.

The second patch here isn't needed.  I thought it would be, wrote it,
then found it didn't directly help.  However I think it is still a nice
simplification so I left it.  If you don't want it, please drop it.

Thanks to Jan Kara for his initial review which helpped me see some
problems with my initial attempt more clearly.

NeilBrown

Note: these patches are quite independant of the rest of that big
patchset, except for the late removal of d_alloc().  They can safely
land indepenantly if you would like to take them now in the ext4 tree.

 [PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()
 [PATCH 2/3] ext4: add ext4_fc_eligible()
 [PATCH 3/3] ext4: move dcache manipulation out of __ext4_link()

[PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()

[NeilBrown]

From: NeilBrown <neil@brown.name>

__ext4_add_entry() is not given a dentry - just inodes and name.
This will help the next patch which simplifies __ex4_link().

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/ext4/namei.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index c4b5e252af0e..768036a109d7 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2353,10 +2353,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
  * may not sleep between calling this and putting something into
  * the entry, as someone else might have used it while you slept.
  */
-static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
+static int __ext4_add_entry(handle_t *handle, struct inode *dir,
+			  const struct qstr *d_name,
 			  struct inode *inode)
 {
-	struct inode *dir = d_inode(dentry->d_parent);
 	struct buffer_head *bh = NULL;
 	struct ext4_dir_entry_2 *de;
 	struct super_block *sb;
@@ -2373,13 +2373,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 	sb = dir->i_sb;
 	blocksize = sb->s_blocksize;
 
-	if (fscrypt_is_nokey_name(dentry))
-		return -ENOKEY;
-
-	if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
+	if (!generic_ci_validate_strict_name(dir, d_name))
 		return -EINVAL;
 
-	retval = ext4_fname_setup_filename(dir, &dentry->d_name, 0, &fname);
+	retval = ext4_fname_setup_filename(dir, d_name, 0, &fname);
 	if (retval)
 		return retval;
 
@@ -2460,6 +2457,16 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 	return retval;
 }
 
+static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
+			  struct inode *inode)
+{
+	struct inode *dir = d_inode(dentry->d_parent);
+
+	if (fscrypt_is_nokey_name(dentry))
+		return -ENOKEY;
+	return __ext4_add_entry(handle, dir, &dentry->d_name, inode);
+}
+
 /*
  * Returns 0 for success, or a negative error value
  */
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()

[Andreas Dilger]

On Mar 17, 2026, at 16:39, NeilBrown <neilb@ownmail.net> wrote:
> 
> From: NeilBrown <neil@brown.name>
> 
> __ext4_add_entry() is not given a dentry - just inodes and name.
> This will help the next patch which simplifies __ex4_link().
> 
> Signed-off-by: NeilBrown <neil@brown.name>

Reviewed-by: Andreas Dilger <adilger@dilger.ca <mailto:adilger@dilger.ca>>

> ---
> fs/ext4/namei.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c4b5e252af0e..768036a109d7 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2353,10 +2353,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>  * may not sleep between calling this and putting something into
>  * the entry, as someone else might have used it while you slept.
>  */
> -static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> +static int __ext4_add_entry(handle_t *handle, struct inode *dir,
> +  const struct qstr *d_name,
>  struct inode *inode)
> {
> - struct inode *dir = d_inode(dentry->d_parent);
> struct buffer_head *bh = NULL;
> struct ext4_dir_entry_2 *de;
> struct super_block *sb;
> @@ -2373,13 +2373,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> sb = dir->i_sb;
> blocksize = sb->s_blocksize;
> 
> - if (fscrypt_is_nokey_name(dentry))
> - return -ENOKEY;
> -
> - if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
> + if (!generic_ci_validate_strict_name(dir, d_name))
> return -EINVAL;
> 
> - retval = ext4_fname_setup_filename(dir, &dentry->d_name, 0, &fname);
> + retval = ext4_fname_setup_filename(dir, d_name, 0, &fname);
> if (retval)
> return retval;
> 
> @@ -2460,6 +2457,16 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> return retval;
> }
> 
> +static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> +  struct inode *inode)
> +{
> + struct inode *dir = d_inode(dentry->d_parent);
> +
> + if (fscrypt_is_nokey_name(dentry))
> + return -ENOKEY;
> + return __ext4_add_entry(handle, dir, &dentry->d_name, inode);
> +}
> +
> /*
>  * Returns 0 for success, or a negative error value
>  */
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 

Re: [PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()

[Jan Kara]

On Wed 18-03-26 09:39:49, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> __ext4_add_entry() is not given a dentry - just inodes and name.
> This will help the next patch which simplifies __ex4_link().
> 
> Signed-off-by: NeilBrown <neil@brown.name>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/namei.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c4b5e252af0e..768036a109d7 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2353,10 +2353,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>   * may not sleep between calling this and putting something into
>   * the entry, as someone else might have used it while you slept.
>   */
> -static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> +static int __ext4_add_entry(handle_t *handle, struct inode *dir,
> +			  const struct qstr *d_name,
>  			  struct inode *inode)
>  {
> -	struct inode *dir = d_inode(dentry->d_parent);
>  	struct buffer_head *bh = NULL;
>  	struct ext4_dir_entry_2 *de;
>  	struct super_block *sb;
> @@ -2373,13 +2373,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
>  	sb = dir->i_sb;
>  	blocksize = sb->s_blocksize;
>  
> -	if (fscrypt_is_nokey_name(dentry))
> -		return -ENOKEY;
> -
> -	if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
> +	if (!generic_ci_validate_strict_name(dir, d_name))
>  		return -EINVAL;
>  
> -	retval = ext4_fname_setup_filename(dir, &dentry->d_name, 0, &fname);
> +	retval = ext4_fname_setup_filename(dir, d_name, 0, &fname);
>  	if (retval)
>  		return retval;
>  
> @@ -2460,6 +2457,16 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
>  	return retval;
>  }
>  
> +static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> +			  struct inode *inode)
> +{
> +	struct inode *dir = d_inode(dentry->d_parent);
> +
> +	if (fscrypt_is_nokey_name(dentry))
> +		return -ENOKEY;
> +	return __ext4_add_entry(handle, dir, &dentry->d_name, inode);
> +}
> +
>  /*
>   * Returns 0 for success, or a negative error value
>   */
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/3] ext4: add ext4_fc_eligible()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
ext4_fc_disabled().  The code can be simplified by combining these two
in a new ext4_fc_eligible().

In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
ext4_fc_mark_ineligible(), but as that is a non-op when
ext4_fc_disabled() is true, this is no no consequence.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/ext4/fast_commit.c | 43 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index f575751f1cae..3ee302b73f45 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -224,6 +224,12 @@ static bool ext4_fc_disabled(struct super_block *sb)
 		(EXT4_SB(sb)->s_mount_state & EXT4_FC_REPLAY));
 }
 
+static bool ext4_fc_eligible(struct super_block *sb)
+{
+	return !ext4_fc_disabled(sb) &&
+		!(ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE));
+}
+
 /*
  * Remove inode from fast commit list. If the inode is being committed
  * we wait until inode commit is done.
@@ -473,13 +479,8 @@ void ext4_fc_track_unlink(handle_t *handle, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 
-	if (ext4_fc_disabled(inode->i_sb))
-		return;
-
-	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
-		return;
-
-	__ext4_fc_track_unlink(handle, inode, dentry);
+	if (ext4_fc_eligible(inode->i_sb))
+		__ext4_fc_track_unlink(handle, inode, dentry);
 }
 
 void __ext4_fc_track_link(handle_t *handle,
@@ -500,13 +501,8 @@ void ext4_fc_track_link(handle_t *handle, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 
-	if (ext4_fc_disabled(inode->i_sb))
-		return;
-
-	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
-		return;
-
-	__ext4_fc_track_link(handle, inode, dentry);
+	if (ext4_fc_eligible(inode->i_sb))
+		__ext4_fc_track_link(handle, inode, dentry);
 }
 
 void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
@@ -527,13 +523,8 @@ void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 
-	if (ext4_fc_disabled(inode->i_sb))
-		return;
-
-	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
-		return;
-
-	__ext4_fc_track_create(handle, inode, dentry);
+	if (ext4_fc_eligible(inode->i_sb))
+		__ext4_fc_track_create(handle, inode, dentry);
 }
 
 /* __track_fn for inode tracking */
@@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 	if (S_ISDIR(inode->i_mode))
 		return;
 
-	if (ext4_fc_disabled(inode->i_sb))
-		return;
-
 	if (ext4_should_journal_data(inode)) {
 		ext4_fc_mark_ineligible(inode->i_sb,
 					EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
 		return;
 	}
 
-	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+	if (!ext4_fc_eligible(inode->i_sb))
 		return;
 
 	/*
@@ -644,10 +632,7 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star
 	if (S_ISDIR(inode->i_mode))
 		return;
 
-	if (ext4_fc_disabled(inode->i_sb))
-		return;
-
-	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+	if (!ext4_fc_eligible(inode->i_sb))
 		return;
 
 	if (ext4_has_inline_data(inode)) {
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 2/3] ext4: add ext4_fc_eligible()

[Andreas Dilger]

On Mar 17, 2026, at 16:39, NeilBrown <neilb@ownmail.net> wrote:
> 
> From: NeilBrown <neil@brown.name>

(add Harshad to CC list)

> Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
> ext4_fc_disabled().  The code can be simplified by combining these two
> in a new ext4_fc_eligible().
> 
> In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
> ext4_fc_mark_ineligible(), but as that is a non-op when
> ext4_fc_disabled() is true, this is no no consequence.
> 
> Signed-off-by: NeilBrown <neil@brown.name>

Nice cleanup.  One minor semantic change in ext4_fc_track_inode() that
deserves a bit of scrutiny is that ext4_fc_mark_ineligible() is now
always called before ext4_fc_disabled() is checked.  That looks OK,
since ext4_fc_mark_ineligible() checks ext4_fc_disabled() internally.

It looks slightly more efficient to check ext4_fc_eligible(inode->i_sb)
first?

Reviewed-by: Andreas Dilger <adilger@dilger.ca <mailto:adilger@dilger.ca>>

> @@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>  	if (S_ISDIR(inode->i_mode))
>  		return;
>  
> -	if (ext4_fc_disabled(inode->i_sb))
> -		return;
> -
>  	if (ext4_should_journal_data(inode)) {
>  		ext4_fc_mark_ineligible(inode->i_sb,
>  					EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
>  		return;
>  	}
> 
> -	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> +	if (!ext4_fc_eligible(inode->i_sb))
>  		return;
>  
>  /*

Re: [PATCH 2/3] ext4: add ext4_fc_eligible()

[Jan Kara]

On Wed 18-03-26 09:39:50, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
> ext4_fc_disabled().  The code can be simplified by combining these two
> in a new ext4_fc_eligible().
> 
> In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
> ext4_fc_mark_ineligible(), but as that is a non-op when
> ext4_fc_disabled() is true, this is no no consequence.
> 
> Signed-off-by: NeilBrown <neil@brown.name>

One nit below, otherwise feel free to add:

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

> @@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>  	if (S_ISDIR(inode->i_mode))
>  		return;
>  
> -	if (ext4_fc_disabled(inode->i_sb))
> -		return;
> -
>  	if (ext4_should_journal_data(inode)) {
>  		ext4_fc_mark_ineligible(inode->i_sb,
>  					EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
>  		return;
>  	}
>  
> -	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> +	if (!ext4_fc_eligible(inode->i_sb))
>  		return;

Here I think the !ext4_fc_eligible() check could be actually above the
ext4_should_journal_data() check - if the fs is not eligible for
fastcommit, there's no point in marking it ineligible again...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/3] ext4: add ext4_fc_eligible()

[NeilBrown]

On Thu, 19 Mar 2026, Jan Kara wrote:
> On Wed 18-03-26 09:39:50, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
> > ext4_fc_disabled().  The code can be simplified by combining these two
> > in a new ext4_fc_eligible().
> > 
> > In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
> > ext4_fc_mark_ineligible(), but as that is a non-op when
> > ext4_fc_disabled() is true, this is no no consequence.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> 
> One nit below, otherwise feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> > @@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
> >  	if (S_ISDIR(inode->i_mode))
> >  		return;
> >  
> > -	if (ext4_fc_disabled(inode->i_sb))
> > -		return;
> > -
> >  	if (ext4_should_journal_data(inode)) {
> >  		ext4_fc_mark_ineligible(inode->i_sb,
> >  					EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
> >  		return;
> >  	}
> >  
> > -	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> > +	if (!ext4_fc_eligible(inode->i_sb))
> >  		return;
> 
> Here I think the !ext4_fc_eligible() check could be actually above the
> ext4_should_journal_data() check - if the fs is not eligible for
> fastcommit, there's no point in marking it ineligible again...

Both you and Andreas have questioned that choice - so I should explain
my reasoning.

ext4_fc_mark_ineligible() is NOT a no-op when the sb is already marked
ineligible.  The code updates sb->s_fc_ineligible_tid to the largest tid
which was ineligible for fc.  Then it only clears the "ineligible" flag
after that highest numbered transaction has committed.  If we skip
ext4_fc_mark_ineligible() because EXT4_MF_FC_INELIGIBLE is already set,
then that flag could be cleared too early.

Thanks,
NeilBrown

Re: [PATCH 2/3] ext4: add ext4_fc_eligible()

[Andreas Dilger]

> On Mar 19, 2026, at 17:31, NeilBrown <neilb@ownmail.net> wrote:
> 
> On Thu, 19 Mar 2026, Jan Kara wrote:
>> On Wed 18-03-26 09:39:50, NeilBrown wrote:
>>> From: NeilBrown <neil@brown.name>
>>> 
>>> Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
>>> ext4_fc_disabled().  The code can be simplified by combining these two
>>> in a new ext4_fc_eligible().
>>> 
>>> In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
>>> ext4_fc_mark_ineligible(), but as that is a non-op when
>>> ext4_fc_disabled() is true, this is no no consequence.
>>> 
>>> Signed-off-by: NeilBrown <neil@brown.name>
>> 
>> One nit below, otherwise feel free to add:
>> 
>> Reviewed-by: Jan Kara <jack@suse.cz>
>> 
>>> @@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>>> if (S_ISDIR(inode->i_mode))
>>> return;
>>> 
>>> - if (ext4_fc_disabled(inode->i_sb))
>>> - return;
>>> -
>>> if (ext4_should_journal_data(inode)) {
>>> ext4_fc_mark_ineligible(inode->i_sb,
>>> EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
>>> return;
>>> }
>>> 
>>> - if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
>>> + if (!ext4_fc_eligible(inode->i_sb))
>>> return;
>> 
>> Here I think the !ext4_fc_eligible() check could be actually above the
>> ext4_should_journal_data() check - if the fs is not eligible for
>> fastcommit, there's no point in marking it ineligible again...
> 
> Both you and Andreas have questioned that choice - so I should explain
> my reasoning.
> 
> ext4_fc_mark_ineligible() is NOT a no-op when the sb is already marked
> ineligible.  The code updates sb->s_fc_ineligible_tid to the largest tid
> which was ineligible for fc.  Then it only clears the "ineligible" flag
> after that highest numbered transaction has committed.  If we skip
> ext4_fc_mark_ineligible() because EXT4_MF_FC_INELIGIBLE is already set,
> then that flag could be cleared too early.

You are right, I didn't understand the ongoing effect of FC_INELIGIBLE.
As you write, checking ext4_fc_eligible() afterward is equivalent, since
ext4_fc_mark_ineligible() is itself checking ext4_fc_disabled() internally,
so the added overhead is the ext4_should_journal_data() call, which looks
pretty complex but is just a bunch of bit field checks (though potentially
adds a lot of branches depending on the frequency of the branches).

Cheers, Andreas

Re: [PATCH 2/3] ext4: add ext4_fc_eligible()

[Jan Kara]

On Fri 20-03-26 10:31:01, NeilBrown wrote:
> On Thu, 19 Mar 2026, Jan Kara wrote:
> > On Wed 18-03-26 09:39:50, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > > 
> > > Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
> > > ext4_fc_disabled().  The code can be simplified by combining these two
> > > in a new ext4_fc_eligible().
> > > 
> > > In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
> > > ext4_fc_mark_ineligible(), but as that is a non-op when
> > > ext4_fc_disabled() is true, this is no no consequence.
> > > 
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > 
> > One nit below, otherwise feel free to add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > > @@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
> > >  	if (S_ISDIR(inode->i_mode))
> > >  		return;
> > >  
> > > -	if (ext4_fc_disabled(inode->i_sb))
> > > -		return;
> > > -
> > >  	if (ext4_should_journal_data(inode)) {
> > >  		ext4_fc_mark_ineligible(inode->i_sb,
> > >  					EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
> > >  		return;
> > >  	}
> > >  
> > > -	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> > > +	if (!ext4_fc_eligible(inode->i_sb))
> > >  		return;
> > 
> > Here I think the !ext4_fc_eligible() check could be actually above the
> > ext4_should_journal_data() check - if the fs is not eligible for
> > fastcommit, there's no point in marking it ineligible again...
> 
> Both you and Andreas have questioned that choice - so I should explain
> my reasoning.
> 
> ext4_fc_mark_ineligible() is NOT a no-op when the sb is already marked
> ineligible.  The code updates sb->s_fc_ineligible_tid to the largest tid
> which was ineligible for fc.  Then it only clears the "ineligible" flag
> after that highest numbered transaction has committed.  If we skip
> ext4_fc_mark_ineligible() because EXT4_MF_FC_INELIGIBLE is already set,
> then that flag could be cleared too early.

Oops, you're right. I forgot about this subtlety. Thanks for explanation!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[NeilBrown]

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
inode on success.  The latter doesn't need or want any dcache
manipluation.

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>
---
 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

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

[Jan Kara]

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