by harshad shirwadkar

[permalink] [raw]

Subject: Re: [PATCH 04/10] ext4: clean up the JBD2 API that initializes fast commits

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 <[email protected]> wrote:
>
> On Thu, Nov 5, 2020, 4:45 AM Jan Kara <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> SUSE Labs, CR
>
>
> Best regards,
> Amy Parker
> (they/them)

2020-11-06 03:08:42

by harshad shirwadkar

[permalink] [raw]

Subject: Re: [PATCH 07/10] ext4: misc fast commit fixes

On Tue, Nov 3, 2020 at 8:52 AM Jan Kara <[email protected]> wrote:
>
> On Sat 31-10-20 13:05:15, Harshad Shirwadkar wrote:
> > This patch adds a small number of misc fast commit fixes. Along with
> > functional fixes such as setting the right buffer flags, there also
> > typo fixes and comment additions.
> >
> > Suggested-by: Jan Kara <[email protected]>
> > Signed-off-by: Harshad Shirwadkar <[email protected]>
>
> Again, please don't merge logically separate fixes into one commit.
Sure, I'll break this up.
>
> > diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> > index 00dc668e052b..10855cd230c7 100644
> > --- a/fs/ext4/ext4_jbd2.h
> > +++ b/fs/ext4/ext4_jbd2.h
> > @@ -422,9 +422,13 @@ static inline int ext4_journal_force_commit(journal_t *journal)
> > static inline int ext4_jbd2_inode_add_write(handle_t *handle,
> > struct inode *inode, loff_t start_byte, loff_t length)
> > {
> > - if (ext4_handle_valid(handle))
> > + if (ext4_handle_valid(handle)) {
> > + ext4_fc_track_range(handle, inode,
> > + start_byte >> inode->i_sb->s_blocksize_bits,
> > + (start_byte + length) >> inode->i_sb->s_blocksize_bits);
> > return jbd2_journal_inode_ranged_write(handle,
> > EXT4_I(inode)->jinode, start_byte, length);
> > + }
>
> Why this change? A good changelog would tell me... I'm suspicious here
> because ext4_jbd2_inode_add_write() gets called only in data=ordered mode
> but fastcommit can run also in data=writeback mode...
>
> I suppose this is for the mmap coverage we were speaking about. Now that
> I'm speaking about it again maybe the ext4_fc_track_range() call in
> ext4_map_blocks() is actually enough? I mean once we allocate blocks for a
> range (either from page fault, write, or writeback of delalloc), they will
> become properly tracked in ext4_map_blocks() and that's all we need? But
> then I'm missing why we have so many ext4_fc_track_range() calls around the
> code... Can you please explain?
You are right. I was trying to handle the mmap case as we discussed on
the previous patch set. But as I mentioned in one of the other patches
in this series, I scanned all the callers of ext4_fc_track_range() and
realized that there were a few redundant callers of this function.
Ultimately, this function needs to get called only when blocks are
added or removed from an inode. So the places where this call remains
are - fallocate ops, ext4_map_blocks, truncate.

Thanks,
Harshad
>
> Honza
>
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR