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
