On Fri 28-04-17 08:03:24, Christoph Hellwig wrote:
> On Fri, Apr 28, 2017 at 11:59:34AM +0200, Jan Kara wrote:
> > Fix the problem by making sure journal superblock writes are always
> > treated as synchronous since they generally block progress of the
> > journalling machinery and thus the whole filesystem.
>
> The callchains leading down to jbd2_write_superblock looks a little
> suspicious to me. It seems like jbd2_journal_commit_transaction
> will actually call without FUA in the JBD2_FLUSHED case. Is that
> really intentional, and if yes should it be documented?
I guess you mean this:
/*
* We hold j_checkpoint_mutex so tail cannot change under us.
* We don't need any special data guarantees for writing sb
* since journal is empty and it is ok for write to be
* flushed only with transaction commit.
*/
jbd2_journal_update_sb_log_tail(journal,
journal->j_tail_sequence,
journal->j_tail,
REQ_SYNC);
And yes, omitting REQ_FUA is intentional and the comment mentions it as "We
don't need any special data guarantees...". Maybe I could add there an
explicit mentioning of REQ_FUA and REQ_PREFLUSH so that it is clearer what
we are talking about.
> Except for that it would seem more useful to move to a "bool preflush"
> argument passed down.
Well, we can call jbd2_write_superblock() with REQ_FUA, REQ_PREFLUSH |
REQ_FUA, REQ_SYNC. So one bool argument won't be enough. However I do agree
that it would be cleaner to pass REQ_SYNC directly from all the places
which set some flags which are eventually passed down to
jbd2_write_superblock(). I'll create a cleanup patch for that.
> But I guess we'll need a quick fix first, for that:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Thanks!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR