| Message ID | 20180220023038.19883-4-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | ext4: fix up shutdown handling | expand |
On Mon 19-02-18 21:30:34, Theodore Ts'o wrote: > The ext4 forced shutdown flag needs to prevent new handles from being > started, but it needs to allow existing handles to complete. So the > forced shutdown flag should not force ext4_journal_get_write_access to > fail. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: stable@vger.kernel.org OK, if you want the semantics of ext4 shutdown to be that running handles should be allowed to complete, I see where you are going with this patch. However there are more problems with this semantics than just __ext4_journal_get_write_access(). Just for example ext4_reserve_inode_write() will bail in case the fs got shutdown and thus inode changes won't be properly added to the running handle. Also places that rely on nested transactions being possible will not work because ext4_journal_start_sb() will refuse to get refcount of a running handle (ext4_journal_check_start() fails) in case fs got shutdown. And I may have missed other cases. The above problems are a reason why I though the semantics of ext4 shutdown was terminate the fs *now* - effectively a software equivalent of power off. That is much easier to implement since we just have to make sure no running handle makes it to the journal... Since I've said Google is using ext4 shutdown - is there any reason why you need the "running handles are allowed to finish" semantics? After all it seems it's just a race whether some handle makes it before the cut off or not... Honza > --- > fs/ext4/ext4_jbd2.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c > index 2d593201cf7a..7c70b08d104c 100644 > --- a/fs/ext4/ext4_jbd2.c > +++ b/fs/ext4/ext4_jbd2.c > @@ -166,13 +166,6 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line, > might_sleep(); > > if (ext4_handle_valid(handle)) { > - struct super_block *sb; > - > - sb = handle->h_transaction->t_journal->j_private; > - if (unlikely(ext4_forced_shutdown(EXT4_SB(sb)))) { > - jbd2_journal_abort_handle(handle); > - return -EIO; > - } > err = jbd2_journal_get_write_access(handle, bh); > if (err) > ext4_journal_abort_handle(where, line, __func__, bh, > -- > 2.16.1.72.g5be1f00a9a >
On Wed, Mar 07, 2018 at 10:42:01AM +0100, Jan Kara wrote: > On Mon 19-02-18 21:30:34, Theodore Ts'o wrote: > > The ext4 forced shutdown flag needs to prevent new handles from being > > started, but it needs to allow existing handles to complete. So the > > forced shutdown flag should not force ext4_journal_get_write_access to > > fail. > > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > > Cc: stable@vger.kernel.org > > OK, if you want the semantics of ext4 shutdown to be that running > handles should be allowed to complete, I see where you are going with this > patch. However there are more problems with this semantics than just > __ext4_journal_get_write_access(). Just for example > ext4_reserve_inode_write() will bail in case the fs got shutdown and thus > inode changes won't be properly added to the running handle. Also places > that rely on nested transactions being possible will not work because > ext4_journal_start_sb() will refuse to get refcount of a running handle > (ext4_journal_check_start() fails) in case fs got shutdown. And I may have > missed other cases. We have three cases in ext4_shutdown: EXT4_GOING_FLAGS_DEFUALT: Freezes the block device, sets the EXT4_FLAGS_SHUTDOWN flag, and then unfreezes the block device: EXT4_GOING_FLAGS_LOGFLUSH: Sets the EXT4_FLAGS_SHUTDOWN flag, forces a commit, and then aborts the journal. EXT4_GOING_FLAGS_NOLOGFLUSH Sets the EXT4_FLAGS_SHUTDOWN flag, aborts the journal. > The above problems are a reason why I though the semantics of ext4 shutdown > was terminate the fs *now* - effectively a software equivalent of power > off. That is much easier to implement since we just have to make sure no > running handle makes it to the journal... Since I've said Google is using > ext4 shutdown - is there any reason why you need the "running handles are > allowed to finish" semantics? After all it seems it's just a race whether > some handle makes it before the cut off or not... Good points. I'll have to look at what happens if we just drop the EXT4_FLAGS_SHUTDOWN flag altogether. - Ted
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c index 2d593201cf7a..7c70b08d104c 100644 --- a/fs/ext4/ext4_jbd2.c +++ b/fs/ext4/ext4_jbd2.c @@ -166,13 +166,6 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line, might_sleep(); if (ext4_handle_valid(handle)) { - struct super_block *sb; - - sb = handle->h_transaction->t_journal->j_private; - if (unlikely(ext4_forced_shutdown(EXT4_SB(sb)))) { - jbd2_journal_abort_handle(handle); - return -EIO; - } err = jbd2_journal_get_write_access(handle, bh); if (err) ext4_journal_abort_handle(where, line, __func__, bh,
The ext4 forced shutdown flag needs to prevent new handles from being started, but it needs to allow existing handles to complete. So the forced shutdown flag should not force ext4_journal_get_write_access to fail. Signed-off-by: Theodore Ts'o <tytso@mit.edu> Cc: stable@vger.kernel.org --- fs/ext4/ext4_jbd2.c | 7 ------- 1 file changed, 7 deletions(-)