On Fri, Apr 10, 2020 at 5:12 AM Andreas Dilger <adilger@xxxxxxxxx> wrote:
>
> On Apr 8, 2020, at 3:55 PM, Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx> wrote:
> >
> > From: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
> >
> > Define feature flags for fast commits and add routines to allow ext4 to
> > initialize fast commits. Note that we allow 128 blocks to be used for
> > fast commits. As of now, that's the default constant value.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
>
> > +static inline int ext4_should_fast_commit(struct super_block *sb)
> > +{
> > + if (!ext4_has_feature_fast_commit(sb))
> > + return 0;
> > + if (!test_opt2(sb, JOURNAL_FAST_COMMIT))
> > + return 0;
> > + if (test_opt(sb, QUOTA))
> > + return 0;
> > + return 1;
> > +}
>
> This function seems more complex than it should be. In this patch the
> ext4_should_fast_commit() function is only called once during mount, but
> in later patches it looks like it is called many times per file/inode.
>
> Why not just check JOURNAL_FAST_COMMIT, and clear it at mount/remount
> time if the other conditions prevent fast commits being used at all?
> It seems that JOURNAL_FAST_COMMIT is only set if the FAST_COMMIT feature
> is already in the superblock, so always doing both checks seems redundant.
Sounds good, will fix that in the next version.
>
> Also, maybe I missed the discussion, but why does having quotas enabled
> on the filesystem disable fast commits entirely? I see in patch 11/20
> that EXT4_FC_REASON_QUOTA is a reason not to do fast commit on the quota
> inodes themselves, which seems like a reasonable limitation if needed,
> but the above check disables FC for any filesystem with quota, and I
> can't find anywhere that this line is later removed in this series.
Thanks Andreas for catching this. Actually, there is nothing stopping
us from enabling fast commits for quota. As it turns out, this is an
unintended carry over from an earlier version of the patchset. I'll
re-enable it in the next version.
Thanks,
Harshad
>
> Cheers, Andreas
>
>
>
>
>
