Thanks Jan and Amy for the feedback. In V2, I'll drop "no_fc" mount
option which was very confusing. Also, we have a mount option called
"fc_debug_force" that forcefully turns fast commits on even if it was
not enabled by mke2fs. It's handy for running performance tests
without relying on e2fsprogs. But I understand that this option could
also be confusing. There's "debug" in its name and I will also move it
inside #ifdef CONFIG_EXT4_DEBUG so that for production, this gets
compiled out.
On Thu, Nov 5, 2020 at 5:30 AM Amy Parker <enbyamy@xxxxxxxxx> wrote:
>
> On Thu, Nov 5, 2020, 4:45 AM Jan Kara <jack@xxxxxxx> wrote:
>>
>> On Thu 05-11-20 11:30:24, Jan Kara wrote:
>> > On Wed 04-11-20 11:52:24, harshad shirwadkar wrote:
>> > > On Tue, Nov 3, 2020 at 8:29 AM Jan Kara <jack@xxxxxxx> wrote:
>> > > > > -int jbd2_fc_init(journal_t *journal, int num_fc_blks)
>> > > > > +int jbd2_fc_init(journal_t *journal)
>> > > > > {
>> > > > > - journal->j_fc_wbufsize = num_fc_blks;
>> > > > > - journal->j_fc_wbuf = kmalloc_array(journal->j_fc_wbufsize,
>> > > > > - sizeof(struct buffer_head *), GFP_KERNEL);
>> > > > > - if (!journal->j_fc_wbuf)
>> > > > > - return -ENOMEM;
>> > > > > + /*
>> > > > > + * Only set j_fc_wbufsize here to indicate that the client file
>> > > > > + * system is interested in using fast commits. The actual number of
>> > > > > + * fast commit blocks is found inside jbd2_superblock and is only
>> > > > > + * valid if j_fc_wbufsize is non-zero. The real value of j_fc_wbufsize
>> > > > > + * gets set in journal_reset().
>> > > > > + */
>> > > > > + journal->j_fc_wbufsize = JBD2_MIN_FC_BLOCKS;
>> > > > > return 0;
>> > > > > }
>> > > >
>> > > > When looking at this, is there a reason why jbd2_fc_init() still exists? I
>> > > > mean why not just make the rule that the journal has FC block number set
>> > > > iff FC gets enabled? Anything else seems a bit confusing to me and also
>> > > > dangerous - imagine we have fs with FC running, we write some FCs and then
>> > > > crash. Then on system recovery we mount with no_fc mount option. We have
>> > > > just lost data on the filesystem AFAIU... So I'd just remove all the mount
>> > > > options related to fastcommits and leave everything to the journal setup
>> > > > (which can be modified with e2fsprogs if needed) to keep things simple.
>> > > The problem is whether or not to use fast commits is the file system's
>> > > call. The JBD2 feature flag will be cleared on a clean unmount and if
>> > > we rely solely on the JBD2 feature flag, fast commit will be turned
>> > > off after a clean unmount. Whereas the FS compat flag is the source of
>> > > truth about whether fast commit needs to be used or not. That's why we
>> > > need an API for the file system to tell JBD2 to still do fast commits.
>> >
>> > Yes, I meant the API could be just that the filesystem either calls
>> > jbd2_journal_set_features() with FASTCOMMIT feature or it won't. Similarly
>> > to how e.g. JBD2_FEATURE_INCOMPAT_64BIT is handled. No need for
>> > jbd2_fc_init() function AFAICT.
Sounds good, I'll drop it.
Thanks,
Harshad
>> >
>> > > Mount options that override the feature flag in Ext4 were mainly meant
>> > > for debugging purposes. So, perhaps there should be a clear warning
>> > > message in the kernel if any of these options are used? Even if we get
>> > > rid of the mount options, we still need the jbd2_fc_init() API for the
>> > > FS to tell JBD2 that it wants to use fast commit. Note that even if
>> > > jbd2_fc_init() is not called, JBD2 will still try to replay fast
>> > > commit blocks.
>>
>> I forgot to add here: I don't like "debug-only" mount options in production
>> kernels because users tend to try them out and:
>> a) occasionally get burnt by unexpected behavior
>
>
> Seen that happen enough times myself, there's a
> reason I always leave notes to users of systems I
> set up to never turn on debug-only mode in day to
>
>> b) the options become hard to get rid of because someone starts to depend
>> on them.
>
>
> This occurs not just with kernel things but with all
> software in general. Leaving options in long-term
> that then need to be removed gets problematic
> pretty quickly.
>
>>
>> So I'd prefer that the options are removed unless they are really essential
>> for debugging the feature
>
>
> Yeah - remove them if we can, if they aren't essential
> then no need to keep them.
>
>> and if they are essential, they should be clearly
>> marked as debug aid... (e.g. with debug in the name or so).
>
>
> At *least* that if not more.
>
>>
>> Honza
>>
>> --
>> Jan Kara <jack@xxxxxxxx>
>> SUSE Labs, CR
>
>
> Best regards,
> Amy Parker
> (they/them)
