Re: [PATCH 1/6] jbd2: Issue cache flush after checkpointing even with internal journal

[Ted Ts'o <tytso@mit.edu>]

Hi Jan,

Am I missing something?  In the original code, we figure out the block
# of the tail of the journal while holding the j_state_lock for
writing, and we hold the lock until journal->j_tail is updated.

In your proposed replacement code, you call
jbd2_journal_get_log_tail() to determine the block #, but you aren't
holding any locks.  jbd2_journal_get_log_tail() grabs a read lock to
figure out the block number, but then drops the lock before it
returns.  So then journal->j_tail gets updated by
jbd2_journal_update_tail() --- using the block # determined by
jbd2_journal_get_log_tail(), but we've released the lock, so can we
guarantee the block number is still accurate?

In particular, since jbd2_cleanup_journal_tail() is now not holding
any locks, what if it is racing against itself?  I can't quite see
race that would lead to something horrible happening, but my spidey
sense is tingling....

Also:

> +/*
> + * Update information in journal about log tail. The function returns 1 if
> + * tail was updated, 0 otherwise. If 1 is returned, caller *must* write
> + * journal superblock before next transaction commit is started.
> + */

If jbd2_update_log_tail() returns 1, how is this enforced?  The caller
can issue a journal superblocok update, sure, but there's no locking
to prevent some other process from immediately starting a new
transaction?

Again, am I missing something?

Regards,

							- Ted