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: <20210504163550.1486337-1-leah.rumancik@gmail.com>
 <20210504163550.1486337-2-leah.rumancik@gmail.com> <20210505212711.GA8532@magnolia>
 <20210505220844.GD8532@magnolia> <20210506155853.GF8532@magnolia>
 <YJQy/DfuCFyZdwe7@google.com> <CAD+ocbwK9v=ky+bBtMuVMP+zDNYnTSO2DjPZ4sE1AnYw8iEmew@mail.gmail.com>
In-Reply-To: <CAD+ocbwK9v=ky+bBtMuVMP+zDNYnTSO2DjPZ4sE1AnYw8iEmew@mail.gmail.com>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Tue, 11 May 2021 10:11:00 -0700
Message-ID: <CAD+ocbyn_3+ToBFiujGNvphddzRR7civ1UPtRrEpnhQKv27opg@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] ext4: add ioctl EXT4_IOC_CHECKPOINT
To:     Leah Rumancik <leah.rumancik@gmail.com>
Cc:     "Darrick J. Wong" <djwong@kernel.org>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        "Theodore Y. Ts'o" <tytso@mit.edu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Leah pointed out that jbd2_journal_flush() function commits the
currently running transaction. EXT4_IOC_CHECKPOINT calls
jbd2_journal_flush() so unless I'm missing something, we're actually
doing all the work that syncfs() would do from EXT4_IOC_CHECKPOINT
ioctl before sending discards. Also, the journal commit that happens
as a part of jbd2_journal_flush() is always a full commit. So, it
sounds like we don't really need to do anything more here. Thanks Leah
for pointing it out!

On Fri, May 7, 2021 at 9:22 AM harshad shirwadkar
<harshadshirwadkar@gmail.com> wrote:
>
> > > > > > +               err = jbd2_journal_flush(EXT4_SB(sb)->s_journal,
> > > >
> > > > Huh.  So we don't flush the filesystem at all, just the journal?  I
> > > > don't see anything in the documentation saying that syncfs() is a
> > > > prerequisite.
> >
> > This is just for the journal, good point, I'll update the documentation.
> It just occurred to me this morning that we need to ensure that a
> *full* commit happens before IOC_CHECKPOINT and not a *fast* commit.
> Fast commits cannot be checkpointed, they rely on full commits for
> checkpoint operation. So, if a syncfs call results in a fast commit,
> the following sequence of events will happen:
>
> * Ext4 writes fast commit information in fast commit area
>
> * When user calls EXT4_IOC_CHECKPOINT, the checkpoint operation would
> result in checkpointing everything in the main journal, except things
> written in fast commit area
>
> * During the discard phase of EXT4_IOC_CHECKPOINT, fast commit area
> will be discarded and thus we'll lose the log updates present in the
> fast commit area
>
> However, this isn't a problem today. Syncfs doesn't result in a fast
> commit but results in a full commit. But, that can change at some
> point in the future. So, unless we can either come up with syncfs()
> variant that can induce a full commit (which would be a little ugly -
> I don't think the user needs to know what kind of journal commit file
> system is doing) or add checkpointing support in fast commits, we
> should just do a full commit from the IOCTL code.
>
> - Harshad