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
[off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

+               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