| Message ID | 20180220023038.19883-6-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | ext4: fix up shutdown handling | expand |
On Mon 19-02-18 21:30:36, Theodore Ts'o wrote: > Previously the jbd2 layer assumed that a file system check would be > required after a journal abort. In the case of the deliberate file > system shutdown, this should not be necessary. Allow the jbd2 layer > to distinguish between these two cases by using the ESHUTDOWN errno. > > Also add proper locking to __journal_abort_soft(). > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: stable@vger.kernel.org > --- Maybe a cleaner api would be to have a separate JBD2 function like jbd2_journal_abort_clean() that would just abort the journal without setting an error to it. > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 3fbf48ec2188..efa0c72a0b9f 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -2105,12 +2108,22 @@ void __jbd2_journal_abort_hard(journal_t *journal) > * but don't do any other IO. */ > static void __journal_abort_soft (journal_t *journal, int errno) > { > - if (journal->j_flags & JBD2_ABORT) > - return; > + int old_errno; > > - if (!journal->j_errno) > + write_lock(&journal->j_state_lock); > + old_errno = journal->j_errno; > + if (!journal->j_errno || errno == -ESHUTDOWN) > journal->j_errno = errno; > > + if (journal->j_flags & JBD2_ABORT) { > + write_unlock(&journal->j_state_lock); > + if (!old_errno && old_errno != -ESHUTDOWN && > + errno == -ESHUTDOWN) > + jbd2_journal_update_sb_errno(journal); > + return; > + } > + write_unlock(&journal->j_state_lock); > + Is it really correct that once the filesystem gets shutdown you clear the previous error from the journal? Because if we hit some real fs corruption, the journal gets aborted, and then someone calls ext4_shutdown(), we'd clear that error which looks like a bug to me because that shutdown hardly fixes the fs corruption... Honza
On Tue, Mar 06, 2018 at 06:10:41PM +0100, Jan Kara wrote: > Is it really correct that once the filesystem gets shutdown you clear the > previous error from the journal? Because if we hit some real fs corruption, > the journal gets aborted, and then someone calls ext4_shutdown(), we'd > clear that error which looks like a bug to me because that shutdown hardly > fixes the fs corruption... That's not what the code does. If journal->j_errno is set, then we won't clear it, for precisely what concern you've articulated. - Ted
On Thu 22-03-18 11:26:45, Theodore Y. Ts'o wrote: > On Tue, Mar 06, 2018 at 06:10:41PM +0100, Jan Kara wrote: > > Is it really correct that once the filesystem gets shutdown you clear the > > previous error from the journal? Because if we hit some real fs corruption, > > the journal gets aborted, and then someone calls ext4_shutdown(), we'd > > clear that error which looks like a bug to me because that shutdown hardly > > fixes the fs corruption... > > That's not what the code does. If journal->j_errno is set, then we > won't clear it, for precisely what concern you've articulated. Sorry for not following up on this earlier but now I've returned to this and I still cannot wrap my head around checks in __journal_abort_soft(). There's: if (!journal->j_errno || errno == -ESHUTDOWN) journal->j_errno = errno; Due to this ESHUTDOWN will override anything in journal->j_errno. Why is that? And then: if (journal->j_flags & JBD2_ABORT) { write_unlock(&journal->j_state_lock); if (!old_errno && old_errno != -ESHUTDOWN && errno == -ESHUTDOWN) jbd2_journal_update_sb_errno(journal); And here can the test ever be true? Probably only if we hard-aborted the journal. The test !old_errno && old_errno != -ESHUTDOWN definitely looks weird and is equivalent to old_errno == 0. Furthermore the errno == -ESHUTDOWN part looks strange as well as in that case we'd only clear the sb->s_errno field... So what was really the intention here? Honza
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 16d3d1325f5b..9ac33a7cbd32 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -493,13 +493,13 @@ static int ext4_shutdown(struct super_block *sb, unsigned long arg) set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags); if (sbi->s_journal && !is_journal_aborted(sbi->s_journal)) { (void) ext4_force_commit(sb); - jbd2_journal_abort(sbi->s_journal, 0); + jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN); } break; case EXT4_GOING_FLAGS_NOLOGFLUSH: set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags); if (sbi->s_journal && !is_journal_aborted(sbi->s_journal)) - jbd2_journal_abort(sbi->s_journal, 0); + jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN); break; default: return -EINVAL; diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 3fbf48ec2188..efa0c72a0b9f 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1483,12 +1483,15 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op) void jbd2_journal_update_sb_errno(journal_t *journal) { journal_superblock_t *sb = journal->j_superblock; + int errcode; read_lock(&journal->j_state_lock); - jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", - journal->j_errno); - sb->s_errno = cpu_to_be32(journal->j_errno); + errcode = journal->j_errno; read_unlock(&journal->j_state_lock); + if (errcode == -ESHUTDOWN) + errcode = 0; + jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode); + sb->s_errno = cpu_to_be32(errcode); jbd2_write_superblock(journal, REQ_SYNC | REQ_FUA); } @@ -2105,12 +2108,22 @@ void __jbd2_journal_abort_hard(journal_t *journal) * but don't do any other IO. */ static void __journal_abort_soft (journal_t *journal, int errno) { - if (journal->j_flags & JBD2_ABORT) - return; + int old_errno; - if (!journal->j_errno) + write_lock(&journal->j_state_lock); + old_errno = journal->j_errno; + if (!journal->j_errno || errno == -ESHUTDOWN) journal->j_errno = errno; + if (journal->j_flags & JBD2_ABORT) { + write_unlock(&journal->j_state_lock); + if (!old_errno && old_errno != -ESHUTDOWN && + errno == -ESHUTDOWN) + jbd2_journal_update_sb_errno(journal); + return; + } + write_unlock(&journal->j_state_lock); + __jbd2_journal_abort_hard(journal); if (errno) {
Previously the jbd2 layer assumed that a file system check would be required after a journal abort. In the case of the deliberate file system shutdown, this should not be necessary. Allow the jbd2 layer to distinguish between these two cases by using the ESHUTDOWN errno. Also add proper locking to __journal_abort_soft(). Signed-off-by: Theodore Ts'o <tytso@mit.edu> Cc: stable@vger.kernel.org --- fs/ext4/ioctl.c | 4 ++-- fs/jbd2/journal.c | 25 +++++++++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-)