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

[Jan Kara <jack@suse.cz>]

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