Re: [PATCH v3 12/13] docs: Add fast commit documentation

From: harshad shirwadkar <harshadshirwadkar@gmail.com>
To: "Theodore Y. Ts'o" <tytso@mit.edu>
Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH v3 12/13] docs: Add fast commit documentation
Date: Wed, 30 Oct 2019 23:41:13 -0700	[thread overview]
Message-ID: <CAD+ocbz55DYWEJDVNcV_tU1BpN_-Vd04YiwgtCaMDMGqkVdL1g@mail.gmail.com> (raw)
In-Reply-To: <CAD+ocbwV+f_sp9-oJyaX=9xvj_DXgLzcXu3CohVEaLDuOSx0hA@mail.gmail.com>

Also, at high level I realized that in order to allow fast commits
being invoked from kjournald thread, the whole patch set has become
more complicated that it needs to be. In other words, if we only
support "asynchronous fast commits" in this patch set and worry about
integrating it with journald thread later, we can simplify this series
a whole lot and yet retain mostly all the functionality. Besides that
adding support of fast commits in kjournald thread would just be an in
memory change. So, just to summarize on this, 1) we will have fsync()
result in only the inode in question being fast committed in async
fashion. 2) ext4_nfs_commit_metadata() would result in all the changed
inodes result in fast commit in async fashion as well. 3) We could
very well use fast commits for normal jbd2 periodic commits as well.
But it's not clear if that will add any value, so we'll leave it out
from this patch series. Do you agree with this?

On Wed, Oct 30, 2019 at 10:34 PM harshad shirwadkar
<harshadshirwadkar@gmail.com> wrote:
>
> Thanks good point. I was trying to imitate how a jbd2 commit I guess.
> There's no reason really to do this in atomic way. I'll fix this in
> next version.
>
> On Thu, Oct 17, 2019 at 6:56 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> >
> > On Tue, Oct 01, 2019 at 12:41:01AM -0700, Harshad Shirwadkar wrote:
> > > +
> > > +Multiple fast commit blocks are a part of one sub-transaction. To
> > > +indicate the last block in a fast commit transaction, fc_flags field
> > > +in the last block in every subtransaction is marked with "LAST" (0x1)
> > > +flag. A subtransaction is valid only if all the following conditions
> > > +are met:
> > > +
> > > +1) SUBTID of all blocks is either equal to or greater than SUBTID of
> > > +   the previous fast commit block.
> > > +2) For every sub-transaction, last block is marked with LAST flag.
> > > +3) There are no invalid blocks in between.
> >
> > I'm wondering why we need to support multiple inodes being modified in
> > a single transaction.  As we currently have defined what can be done,
> > all updates to an inode should be free standing and not dependent on a
> > change to another inode, right?  And today, one block only modifies
> > one inode.
> >
> > The only reason why we might want to define a sub-transaction as being
> > composed of multiple inodes, which must all be updated in an
> > all-or-nothing fashion, is the swap boot inode ioctl, and if that's
> > the only one, I wonder if it's worth the extra complexity.
> >
> > Am I missing anything?
> >
> >                                         - Ted