Re: [PATCH 4/4] ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()

[Jan Kara]

On Wed 24-07-24 15:02:49, Luis Henriques wrote:
> On Wed, Jul 24 2024, Jan Kara wrote:
> 
> > On Tue 23-07-24 16:44:02, Luis Henriques (SUSE) wrote:
> >> Function jbd2_journal_shrink_checkpoint_list() assumes that '0' is not a
> >> valid value for transaction IDs, which is incorrect.
> >> 
> >> Furthermore, the sbi->s_fc_ineligible_tid handling also makes the same
> >> assumption by being initialised to '0'.  Fortunately, the sb flag
> >> EXT4_MF_FC_INELIGIBLE can be used to check whether sbi->s_fc_ineligible_tid
> >> has been previously set instead of comparing it with '0'.
> >> 
> >> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@xxxxxxxxx>
> >
> > Just one style nit below, otherwise looks good. Feel free to add:
> >
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
> >
> > BTW, the ineligibility handling looks flaky to me, in particular the cases
> > where we call ext4_fc_mark_ineligible() with NULL handle seem racy to me as
> > fastcommit can happen *before* we mark the filesystem as ineligible.  But
> > that's not really related to your changes, they just made me look at that
> > code in detail and I couldn't resist complaining :)
> 
> Heh, fair enough.  Regarding this race, I may try to look into it but I'll
> need to dig a bit more.  And yeah it's probably better to separate that
> from this patch.

I suspect all the places that mark the fs as ineligible with NULL handle
need to actually mark corresponding transactions as ineligible using handle
instead. This is going to require a bit of churn e.g. for stuff like
resize or __track_dentry_update() but shouldn't be hard to do.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR