Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201031200518.4178786-1-harshadshirwadkar@gmail.com>
 <20201031200518.4178786-8-harshadshirwadkar@gmail.com> <20201103165237.GK3440@quack2.suse.cz>
In-Reply-To: <20201103165237.GK3440@quack2.suse.cz>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Thu, 5 Nov 2020 19:07:43 -0800
Message-ID: <CAD+ocbz3y0L0N=hfFoJhub1bL=GvYU7d2rtyeVaPMYChC3nbPA@mail.gmail.com>
Subject: Re: [PATCH 07/10] ext4: misc fast commit fixes
To:     Jan Kara <jack@suse.cz>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        "Theodore Y. Ts'o" <tytso@mit.edu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 3, 2020 at 8:52 AM Jan Kara <jack@suse.cz> 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 <jack@suse.cz>
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
>
> 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 <jack@suse.com>
> SUSE Labs, CR