jbd: possible filesystem corruption fixes (take 2)
cover jbd: possible filesystem corruption fixes (take 2)
Subject: [PATCH 0/5] jbd: possible filesystem corruption fixes (take 2)
This patch set is the take 2 of fixing error handling problem in ext3/JBD. The previous discussion can be found here:
http://lkml.org/lkml/2008/5/14/10
The same problem should also be in ext4/JBD, but I haven't prepared it yet.
Problem
=======
Currently some error checkings are missing, so the journal cannot abort correctly. This causes breakage of the ordered mode rule and filesystem corruption. Missing error checkings are:
(1) error check for dirty buffers flushed before the commit (addressed by PATCH 1/5 and 2/5)
(2) error check for the metadata writes to the journal before the commit (addressed by PATCH 3/5)
(3) error check for checkpointing and replay (addressed by PATCH 4/5
and 5/5)
Changes from take 1
===================
[PATCH 1/5]
o not changed
[PATCH 2/5]
o rewrite my coment in journal_dirty_data() comprehensibly
[PATCH 3/5]
o check for errors and abort the journal just before journal_write_commit_record() instead of after writing metadata buffers
[PATCH 4/5 and 5/5]
o separate the ext3 part from the jbd part in a patch
o use JFS_ABORT for checkpointing failures instead of introducing JFS_CP_ABORT flag
o don't update only the journal super block, but also j_tail and j_tail_sequence when the journal has aborted (at least we only have to avoid updating the super block, but keeping j_tail*'s values will be good thing because it may protect someone from adding bugs in the future)
o journal_destroy() returns -EIO when the journal has aborted so that ext3_put_super() can detect the abort
o journal_flush() uses j_checkpoint_mutex to avoid a race with __log_wait_for_space()
The last item targets a newly found problem. journal_flush() can be called while processing __log_wait_for_space(). In this case, cleanup_journal_tail() can be called between __journal_drop_transaction() and journal_abort(), then
the transaction with checkpointing failure is lost from the journal. Using j_checkpoint_mutex which is used by __log_wait_for_space(), we should avoid the race condition. But the test is not so sufficient because it is very difficult to produce this race. So I hope that this locking is reviewed carefully (including a possibility of deadlock.)
Regards,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
PATCH 1/5 jbd: strictly check for write errors on data buffers
Subject: [PATCH 1/5] jbd: strictly check for write errors on data buffers
In ordered mode, we should abort journaling when an I/O error has occurred on a file data buffer in the committing transaction. But there can be data buffers which are not checked for error:
(a) the buffer which has already been written out by pdflush
(b) the buffer which has been unlocked before scanned in the t_locked_list loop
This patch adds missing error checks and aborts journaling appropriately.
--- fs/jbd/commit.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
fs/jbd/commit.c +9 -5
@@ -172,7 +172,7 @@ static void journal_do_submit_data(struc/* * Submit all the data buffers to disk */ -static void journal_submit_data_buffers (journal_t *journal, +static int journal_submit_data_buffers (journal_t *journal,transaction_t *commit_transaction) {struct journal_head *jh;
@@ -180,6 +180,7 @@ static void journal_submit_data_buffers(int locked;int bufs =0 ;struct buffer_head **wbuf = journal->j_wbuf; +int err =0 ;/* * Whenever we unlock the journal and sleep, things can get added
@@ -253,6 +254,8 @@ write_out_data:put_bh (bh); }else {BUFFER_TRACE (bh,"writeout complete: unfile" ); +if (unlikely (!buffer_uptodate (bh))) + err = -EIO ;__journal_unfile_buffer (jh);jbd_unlock_bh_state (bh);if (locked)
@@ -271,6 +274,8 @@ write_out_data: }spin_unlock (&journal->j_list_lock);journal_do_submit_data (wbuf, bufs); + +return err; }/*
@@ -410,8 +415,7 @@ void journal_commit_transaction(journal_* Now start flushing things to disk, in the order they appear * on the transaction lists. Data blocks go first. */ - err =0 ; -journal_submit_data_buffers (journal, commit_transaction); + err =journal_submit_data_buffers (journal, commit_transaction);/* * Wait for all previously submitted IO to complete.
@@ -426,10 +430,10 @@ void journal_commit_transaction(journal_if (buffer_locked (bh)) {spin_unlock (&journal->j_list_lock);wait_on_buffer (bh); -if (unlikely (!buffer_uptodate (bh))) - err = -EIO ;spin_lock (&journal->j_list_lock); } +if (unlikely (!buffer_uptodate (bh))) + err = -EIO ;if (!inverted_lock (journal, bh)) {put_bh (bh);spin_lock (&journal->j_list_lock);
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 3 lines · On Mon, 02 Jun 2008 19:43:57 +0900 Hidehiro Kawai…
In ordered mode, we should abort journaling when an I/O error has occurred on a file data buffer in the committing transaction.
Why should we do that?
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 8 lines · Andrew Morton
Quoted 3 lines · On Mon, 02 Jun 2008 19:43:57 +0900 Hidehiro Kawai…
In ordered mode, we should abort journaling when an I/O error has occurred on a file data buffer in the committing transaction.
Why should we do that?
I see two reasons:
1) If fs below us is returning IO errors, we don't really know how severe it is so it's safest to stop accepting writes. Also user notices the problem early this way. I agree that with the growing size of disks and thus probability of seeing IO error, we should probably think of something cleverer than this but aborting seems better than just doing nothing.
2) If the IO error is just transient (i.e., link to NAS is disconnected for a while), we would silently break ordering mode guarantees (user could be able to see old / uninitialized data).
Honza
PS: Changed Andreas's address in the email to the new one...
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 16 lines · On Wed, 4 Jun 2008 12:19:25 +0200 Jan Kara <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
Quoted 3 lines · On Mon, 02 Jun 2008 19:43:57 +0900 Hidehiro Kawai…
In ordered mode, we should abort journaling when an I/O error has occurred on a file data buffer in the committing transaction.
Why should we do that?
I see two reasons:
1) If fs below us is returning IO errors, we don't really know how severe it is so it's safest to stop accepting writes. Also user notices the problem early this way. I agree that with the growing size of disks and thus probability of seeing IO error, we should probably think of something cleverer than this but aborting seems better than just doing nothing.
Quoted 4 lines
2) If the IO error is just transient (i.e., link to NAS is disconnected for a while), we would silently break ordering mode guarantees (user could be able to see old / uninitialized data).
Does any other filesystem driver turn the fs read-only on the first write-IO-error?
It seems like a big policy change to me. For a lot of applications it's effectively a complete outage and people might get a bit upset if this happens on the first blip from their NAS.
<waves vigorously at linux-ext4 people>
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 6 lines · Andrew Morton
Does any other filesystem driver turn the fs read-only on the first write-IO-error?
It seems like a big policy change to me. For a lot of applications it's effectively a complete outage and people might get a bit upset if this happens on the first blip from their NAS.
As I told Kawai-san when I met with him and his colleagues in Tokyo last week, it is the responsibility of the storage stack to retry errors as appropriate. From the filesystem perspective, a read or a write operation succeeds, or fails. A read or write operation could take a long time before returning, but the storage stack doesn't get to return a "fail, but try again at some point; maybe we'll succeed later, or if you try writing to a different block". The only sane thing for a filesystem to do is to treat any failure as a hard failure.
It is similarly insane to ask a filesystem to figure out that a newly plugged in USB stick is the same one that the user had accidentally unplugged 30 seconds ago. We don't want to put that kind of low-level knowlede about storage details in each different filesystem.
A much better place to put that kind of smarts is in a multipath module which sits in between the device and the filesystem. It can retry writes from a transient failure, if a path goes down or if a iSCSI device temporarily drops off the network. But if a filesystem gets a write failure, it has to assume that the write failure is permanent.
The question though is what should you do if you have a write failure in various different parts of the disk? If you have a write failure in a data block, you can return -EIO to the user. You could try reallocating to find another block, and try writing to that alternate location (although with modern filesystems that do block remapping, this is largely pointless, since an EIO failure on write probably means you've lost connectivity to the disk or the disk as run out of spare blocks). But for a failure to write to the a critical part of the filesystem, like the inode table, or failure to write to the journal, what the heck can you do? Remounting read-only is probably the best thing you can do.
In theory, if it is a failure to write to the journal, you could fall back to no-journaled operation, and if ext3 could support running w/o a journal, that is possibly an option --- but again, it's very likely that the disk is totally gone (i.e., the user pulled the USB stick without unmounting), or the disk is out of spare blocks in its bad block remapping pool, and the system is probably going to be in deep trouble --- and the next failure to write some data might be critical application data. You probably *are* better off failing the system hard, and letting the HA system swap in the hot spare backup, if this is some critical service.
That being said, ext3 can be tuned (and it is the default today, although I should probably change the default to be remount-ro), so that its behaviour on write errors is, "don't worry, be happy", and just leave the filesystem mounted read/write. That's actually quite dangerous for a critical production server, however.....
- Ted
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 3 lines · On Wed, 4 Jun 2008 17:22:02 -0400 Theodore Tso <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
Does any other filesystem driver turn the fs read-only on the first write-IO-error?
^^ this?
Quoted 19 lines
Quoted 3 lines
It seems like a big policy change to me. For a lot of applications it's effectively a complete outage and people might get a bit upset if this happens on the first blip from their NAS.
As I told Kawai-san when I met with him and his colleagues in Tokyo last week, it is the responsibility of the storage stack to retry errors as appropriate. From the filesystem perspective, a read or a write operation succeeds, or fails. A read or write operation could take a long time before returning, but the storage stack doesn't get to return a "fail, but try again at some point; maybe we'll succeed later, or if you try writing to a different block". The only sane thing for a filesystem to do is to treat any failure as a hard failure.
It is similarly insane to ask a filesystem to figure out that a newly plugged in USB stick is the same one that the user had accidentally unplugged 30 seconds ago. We don't want to put that kind of low-level knowlede about storage details in each different filesystem.
Quoted 4 lines
A much better place to put that kind of smarts is in a multipath module which sits in between the device and the filesystem. It can retry writes from a transient failure, if a path goes down or if a iSCSI device temporarily drops off the network.
That's fine in theory, but we don't do any if that right now, do we?
Quoted 3 lines
But if a filesystem
gets a write failure, it has to assume that the write failure is permanent.
To that sector, yes. But to the entire partition?
(Well, if the entire partition became unwriteable then we don't have a problem. It's "parts are writeable" or "it became writeable again later which is the problem).
Quoted 3 lines
The question though is what should you do if you have a write failure in various different parts of the disk? If you have a write failure in a data block, you can return
-EIOto the user.
Absolutely.
But afaict this patch changes things so that if we get a write failure in a data block we make the entire fs read-only. Which, as I said, is often "dead box".
This seems like a quite major policy change to me.
Quoted 9 lines
You could try
reallocating to find another block, and try writing to that alternate location (although with modern filesystems that do block remapping, this is largely pointless, since an EIO failure on write probably means you've lost connectivity to the disk or the disk as run out of spare blocks). But for a failure to write to the a critical part of the filesystem, like the inode table, or failure to write to the journal, what the heck can you do? Remounting read-only is probably the best thing you can do.
Ah, yes, I agree, a write error to the journal or to metadata is a quite different thing. An unwriteable journal block is surely terminal. And a write error during metadata checkpointing is pretty horrid ebcause we've (potentially) already told userspace that the write was successful.
But should we treat plain old data blocks in the same way?
Quoted 17 lines
In theory, if it is a failure to write to the journal, you could fall back to no-journaled operation, and if ext3 could support running w/o a journal, that is possibly an option --- but again, it's very likely that the disk is totally gone (i.e., the user pulled the USB stick without unmounting), or the disk is out of spare blocks in its bad block remapping pool, and the system is probably going to be in deep trouble --- and the next failure to write some data might be critical application data. You probably *are* better off failing the system hard, and letting the HA system swap in the hot spare backup, if this is some critical service.
That being said, ext3 can be tuned (and it is the default today, although I should probably change the default to be remount-ro), so that its behaviour on write errors is, "don't worry, be happy", and just leave the filesystem mounted read/write. That's actually quite dangerous for a critical production server, however.....
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Wed, 4 Jun 2008 17:22:02 -0400 Theodore Tso <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
Does any other filesystem driver turn the fs read-only on the first write-IO-error?
For failures to write to data blocks, I don't think any other filesystems turn the filesystem read-only. Not that many other filesystems do the remount on read-only thing in general; remounting read/only is something that might be unique to ext2/3/4.
Quoted 11 lines
Quoted 9 lines
It is similarly insane to ask a filesystem to figure out that a newly plugged in USB stick is the same one that the user had accidentally unplugged 30 seconds ago. We don't want to put that kind of low-level knowlede about storage details in each different filesystem.
A much better place to put that kind of smarts is in a multipath module which sits in between the device and the filesystem. It can retry writes from a transient failure, if a path goes down or if a iSCSI device temporarily drops off the network.
That's fine in theory, but we don't do any if that right now, do we?
No, but I think it's insane to put any of this into the filesystem. I don't want to put words in other people's mouths, but Mingming and I were chatting with Chris Mason the last two days at a BTRFS workshop (which is why we've been a bit slow on responding), and when discussed this thread informally, he agreed that it was really bad idea to try to put this kind of retry logic in an individual filesystem.
Quoted 5 lines
Quoted 3 lines
But if a filesystem
gets a write failure, it has to assume that the write failure is permanent.To that sector, yes. But to the entire partition?
I agree it's a bad idea. OTOH, we really need a good way of notifying a system daemon or administrator, and not just rely on the application to DTRT when it receives an -EIO. Probably what we should do is (1) return -EIO, (2) send a uevent that includes as much information as possible. At the minimum, the block that had the write (or read) error, and if available the file name involved, and application and/or pid involved. That way, the policy of what should happen in case of a data I/O error can be informed by what write just failed. It might be that if the failure is to some critical application data, the right thing to do is to kill of the application server and let the HA system bring up the hotbackup. Or if the failure is writing to a log file, some other recovery procedure is the right thing.
But forcing the entire filesystem read-only just because of a write failure to a data block is probably not the best way to go.
Quoted 5 lines
But afaict this patch changes things so that if we get a write failure in a data block we make the entire fs read-only. Which, as I said, is often "dead box".
This seems like a quite major policy change to me.
Agreed, and it's not appropriate. I could imagine that for some setups it is the right policy, but the kernel should not be setting policy like this. Maybe as a new tunable in the superblock, or maybe via a round-trip to userspace via a uevent, but certainly not as the new default behavior.
Apologies, I'm still catching up on patches sent in the past few days, so I haven't had a chance to do a detailed review on Kawai-san's patches yet. I do agree that if this patch is forcing the entire filesystem read-only on a write-failure to a data block, it's probably not appropriate.
- Ted
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 52 lines · Theodore Tso
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Wed, 4 Jun 2008 17:22:02 -0400 Theodore Tso <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
Does any other filesystem driver turn the fs read-only on the first write-IO-error?
For failures to write to data blocks, I don't think any other filesystems turn the filesystem read-only. Not that many other filesystems do the remount on read-only thing in general; remounting read/only is something that might be unique to ext2/3/4.
Quoted 11 lines
Quoted 9 lines
It is similarly insane to ask a filesystem to figure out that a newly plugged in USB stick is the same one that the user had accidentally unplugged 30 seconds ago. We don't want to put that kind of low-level knowlede about storage details in each different filesystem.
A much better place to put that kind of smarts is in a multipath module which sits in between the device and the filesystem. It can retry writes from a transient failure, if a path goes down or if a iSCSI device temporarily drops off the network.
That's fine in theory, but we don't do any if that right now, do we?
No, but I think it's insane to put any of this into the filesystem. I don't want to put words in other people's mouths, but Mingming and I were chatting with Chris Mason the last two days at a BTRFS workshop (which is why we've been a bit slow on responding), and when discussed this thread informally, he agreed that it was really bad idea to try to put this kind of retry logic in an individual filesystem.
Quoted 5 lines
Quoted 3 lines
But if a filesystem
gets a write failure, it has to assume that the write failure is permanent.To that sector, yes. But to the entire partition?
I agree it's a bad idea. OTOH, we really need a good way of notifying a system daemon or administrator, and not just rely on the application to DTRT when it receives an
-EIO. Probably what we should do is (1) return-EIO, (2) send a uevent that includes as much information as possible. At the minimum, the block that had the write (or read) error, and if available the file name involved, and application and/or pid involved. That way, the policy of what should happen in case of a data I/O error can be informed by what write just failed. It might be that if the failure is to some critical application data, the right thing to do is to kill of the application server and let the HA system bring up the hotbackup. Or if the failure is writing to a log file, some other recovery procedure is the right thing.
Quoted 2 lines
But forcing the entire filesystem read-only just because of a write failure to a data block is probably not the best way to go.
OK, you've convinced me that aborting journal because of EIO on data
block isn't the best thing to do. But it is a bit problematic to return EIO to userspace - we usually find this in commit code so we work on behalf of kjournald and user already thinks the write has succeeded (which is basically problem of any cached write). So the best thing we can do is set AS_EIO on the mapping and hope userspace will notice (we report such error when user does fsync/sync/sync_file_range).
Sending some event to userspace is certainly possible but can hardly be
more specific than "IO error at block %lu of file %s".
Quoted 11 lines
Quoted 5 lines
But afaict this patch changes things so that if we get a write failure in a data block we make the entire fs read-only. Which, as I said, is often "dead box".
This seems like a quite major policy change to me.
Agreed, and it's not appropriate. I could imagine that for some setups it is the right policy, but the kernel should not be setting policy like this. Maybe as a new tunable in the superblock, or maybe via a round-trip to userspace via a uevent, but certainly not as the new default behavior.
Yes, I believe a tunable in superblock controlling how do we behave on
EIO error in data block would be the best solution.
Quoted 5 lines
Apologies, I'm still catching up on patches sent in the past few days, so I haven't had a chance to do a detailed review on Kawai-san's patches yet. I do agree that if this patch is forcing the entire filesystem read-only on a write-failure to a data block, it's probably not appropriate.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 7 lines · Jan Kara
Quoted 5 lines · Theodore Tso
Quoted 3 lines · Andrew Morton
On Wed, 4 Jun 2008 17:22:02 -0400
Theodore Tso <[email protected]> wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line · Andrew Morton
But afaict this patch changes things so that if we get a write failure in a data block we make the entire fs read-only. Which, as I said, is often "dead box".
This seems like a quite major policy change to me.
My patch doesn't change the policy. JBD aborts the journal when it detects I/O error in file data since 2.6.11. Perhaps this patch:
http://marc.info/?l=linux-kernel&m=110483888632225
I just added missing error checkings.
Quoted 8 lines
Quoted 5 lines
Agreed, and it's not appropriate. I could imagine that for some setups it is the right policy, but the kernel should not be setting policy like this. Maybe as a new tunable in the superblock, or maybe via a round-trip to userspace via a uevent, but certainly not as the new default behavior.
Yes, I believe a tunable in superblock controlling how do we behave on
EIO error in data block would be the best solution.
I agree. I understood that there is a case where we don't want to make the fs read-only when writing file data failed. OTOH there are people who want to make the fs read-only to avoid the damage from expanding. Introducing the tunable would be better.
I'm going to send a patch to make this behavior tunable if some of you agree on this way.
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 6 lines · Hidehiro Kawai
My patch doesn't change the policy. JBD aborts the journal when it detects I/O error in file data since 2.6.11. Perhaps this patch:
http://marc.info/?l=linux-kernel&m=110483888632225
I just added missing error checkings.
Looking at the code paths touched by patch you referenced, you are correct. And Andrew even signed off on it. :-)
But if someone was only examining the patch, it wasn't obvious that the journal was getting aborted when the JBD layer was forcing buffers from t_sync_datalist to disk. So I suspect the change went in without proper consideration of the net effect. You just called it out explicitly in the subject line, which caused Andrew to ask some good questions; questions that weren't asked in 2005.
Quoted 6 lines
I agree. I understood that there is a case where we don't want to make the fs read-only when writing file data failed. OTOH there are people who want to make the fs read-only to avoid the damage from expanding. Introducing the tunable would be better.
I'm going to send a patch to make this behavior tunable if some of you agree on this way.
Note that doing this right may be tricky, since in the case where we aren't aborting the journal, we need to set the appropriate flags in the page cache so that when the user calls fsync() or close(), that they get the EIO error.
- Ted
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 11 lines · On Thu, 5 Jun 2008 10:29:48 -0400 Theodore Tso <[email protected]> wrote:
Quoted 6 lines · Hidehiro Kawai
My patch doesn't change the policy. JBD aborts the journal when it detects I/O error in file data since 2.6.11. Perhaps this patch:
http://marc.info/?l=linux-kernel&m=110483888632225
I just added missing error checkings.Looking at the code paths touched by patch you referenced, you are correct. And Andrew even signed off on it. :-)
Quoted 6 lines
But if someone was only examining the patch, it wasn't obvious that the journal was getting aborted when the JBD layer was forcing buffers from
t_sync_datalistto disk. So I suspect the change went in without proper consideration of the net effect. You just called it out explicitly in the subject line, which caused Andrew to ask some good questions; questions that weren't asked in 2005.
Sigh. An object lesson in the value of good changelogging :(
I guess we need to undo this. And yes, propagating errors into AS_EIO is the way. I guess that's safe without holding lock_page(), as long as the bh is pinned.
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 11 lines · Andrew Morton
Quoted 10 lines · On Thu, 5 Jun 2008 10:29:48 -0400 Theodore Tso <[email protected]> wrote:
Quoted 5 lines · Hidehiro Kawai
My patch doesn't change the policy. JBD aborts the journal when it detects I/O error in file data since 2.6.11. Perhaps this patch:
http://marc.info/?l=linux-kernel&m=110483888632225
I just added missing error checkings.Looking at the code paths touched by patch you referenced, you are correct. And Andrew even signed off on it. :-)
Quoted 8 lines
Quoted 6 lines
But if someone was only examining the patch, it wasn't obvious that the journal was getting aborted when the JBD layer was forcing buffers from
t_sync_datalistto disk. So I suspect the change went in without proper consideration of the net effect. You just called it out explicitly in the subject line, which caused Andrew to ask some good questions; questions that weren't asked in 2005.Sigh. An object lesson in the value of good changelogging :(
... and the value of "diff -p" so it is clear what function is being changed.
Quoted 3 lines
I guess we need to undo this. And yes, propagating errors into
AS_EIOis the way. I guess that's safe without holdinglock_page(), as long as the bh is pinned.
Something like the following instead if -EIO and journal abort:
if (!buffer_uptodate(bh)) {
set_bit(AS_EIO, &bh->b_page->mapping->flags);
SetPageError(bh->b_page);
}It seems end_buffer_async_write() does this already, but journal_do_submit_data() uses end_buffer_write_sync() and it does not do either of those operations.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 8 lines · Andreas Dilger
Quoted 3 lines · Andrew Morton
I guess we need to undo this. And yes, propagating errors into
AS_EIOis the way. I guess that's safe without holdinglock_page(), as long as the bh is pinned.Something like the following instead if
-EIOand journal abort:
Quoted 8 lines
if (!buffer_uptodate(bh)) { set_bit(AS_EIO, &bh->b_page->mapping->flags); SetPageError(bh->b_page); }It seems
end_buffer_async_write()does this already, butjournal_do_submit_data()usesend_buffer_write_sync()and it does not do either of those operations.
Thank you for your suggestion. I wrote an additional patch to do that below. Please apply it as the 6th patch of this patch series.
BTW, I'm developing a patch which makes "abort the journal if a file data buffer has an error" tunable. I'll send it in another thread because it's not a bug fix patch.
Regards,
Signature · 48 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center Subject: JBD: don't abort if flushing file data failed In ordered mode, it is not appropriate behavior to abort the journal when we failed to write file data. This patch calls printk() instead of aborting the journal. Additionally, set AS_EIO into the address_space object of the buffer which is written out by journal_do_submit_data() and failed so that fsync() can get -EIO. Signed-off-by: Hidehiro Kawai <[email protected]> --- fs/jbd/commit.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) Index: linux-2.6.26-rc4/fs/jbd/commit.c =================================================================== --- linux-2.6.26-rc4.orig/fs/jbd/commit.c +++ linux-2.6.26-rc4/fs/jbd/commit.c @@ -432,8 +432,11 @@ void journal_commit_transaction(journal_ wait_on_buffer(bh); spin_lock(&journal->j_list_lock); } - if (unlikely(!buffer_uptodate(bh))) + if (unlikely(!buffer_uptodate(bh))) { + set_bit(AS_EIO, &bh->b_page->mapping->flags); + SetPageError(bh->b_page); err = -EIO; + } if (!inverted_lock(journal, bh)) { put_bh(bh); spin_lock(&journal->j_list_lock); @@ -452,8 +455,11 @@ void journal_commit_transaction(journal_ } spin_unlock(&journal->j_list_lock); - if (err) - journal_abort(journal, err); + if (err) { + printk(KERN_WARNING + "JBD: Detected IO errors during flushing file data\n"); + err = 0; + } journal_write_revoke_records(journal, commit_transaction);
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Quoted 49 lines · Hidehiro Kawai
Quoted 8 lines · Andreas Dilger
Quoted 3 lines · Andrew Morton
I guess we need to undo this. And yes, propagating errors into
AS_EIOis the way. I guess that's safe without holdinglock_page(), as long as the bh is pinned.Something like the following instead if
-EIOand journal abort:Quoted 8 lines
if (!buffer_uptodate(bh)) { set_bit(AS_EIO, &bh->b_page->mapping->flags); SetPageError(bh->b_page); }It seems
end_buffer_async_write()does this already, butjournal_do_submit_data()usesend_buffer_write_sync()and it does not do either of those operations.Thank you for your suggestion. I wrote an additional patch to do that below. Please apply it as the 6th patch of this patch series.
BTW, I'm developing a patch which makes "abort the journal if a file data buffer has an error" tunable. I'll send it in another thread because it's not a bug fix patch.
Signature · 20 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center Subject: JBD: don't abort if flushing file data failed In ordered mode, it is not appropriate behavior to abort the journal when we failed to write file data. This patch calls printk() instead of aborting the journal. Additionally, set AS_EIO into the address_space object of the buffer which is written out by journal_do_submit_data() and failed so that fsync() can get -EIO. Signed-off-by: Hidehiro Kawai <[email protected]> --- fs/jbd/commit.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) Index: linux-2.6.26-rc4/fs/jbd/commit.c ===================================================================
fs/jbd/commit.c void journal_commit_transaction(journal_ · line 439
Quoted 9 lines
fs/jbd/commit.c +4
-1@@ -432,8 +432,11 @@ void journal_commit_transaction(journal_wait_on_buffer (bh);spin_lock (&journal->j_list_lock); } -if (unlikely (!buffer_uptodate (bh))) +if (unlikely (!buffer_uptodate (bh))) { +set_bit (AS_EIO , &bh->b_page->mapping->flags); +SetPageError (bh->b_page); err = -EIO ; + }
Actually, you should be more careful here because if the data buffer has
been truncated in the currently running transaction, it can happen that b_page->mapping is NULL. It is a question how to safely access page->mapping - probably you'll need page lock for that...
Quoted 6 lines
fs/jbd/commit.c +0
-0if (!inverted_lock (journal, bh)) {put_bh (bh);spin_lock (&journal->j_list_lock);@@ -452,8 +455,11 @@ void journal_commit_transaction(journal_ }spin_unlock (&journal->j_list_lock);
fs/jbd/commit.c void journal_commit_transaction(journal_ · line 464
Quoted 10 lines
fs/jbd/commit.c +5
-2-if (err) -journal_abort (journal, err); +if (err) { +printk (KERN_WARNING +"JBD: Detected IO errors during flushing file data\n" ); + err =0 ; + }journal_write_revoke_records (journal, commit_transaction);
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
Hi,
Quoted 13 lines · Jan Kara
Quoted 11 lines · Hidehiro Kawai
Index:
linux-2.6.26-rc4/fs/jbd/commit.c
===================================================================fs/jbd/commit.c +2
-1@@ -432,8 +432,11 @@ void journal_commit_transaction(journal_wait_on_buffer (bh);spin_lock (&journal->j_list_lock); } -if (unlikely (!buffer_uptodate (bh))) +if (unlikely (!buffer_uptodate (bh))) { +set_bit (AS_EIO , &bh->b_page->mapping->flags);
Quoted 8 lines
fs/jbd/commit.c void journal_commit_transaction(journal_ · line 439
Quoted 3 lines
fs/jbd/commit.c +2
-0+SetPageError (bh->b_page); err = -EIO ; + }Actually, you should be more careful here because if the data buffer has
been truncated in the currently running transaction, it can happen thatb_page->mappingis NULL. It is a question how to safely accesspage->mapping- probably you'll need page lock for that...
Thank you for pointing out this problem. I confirmed that b_page->mapping can be NULL. I'm making sure that the locking page approach works out well.
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
In reply to Theodore TsoQuoted 5 lines · Theodore Tso
That being said, ext3 can be tuned (and it is the default today, although I should probably change the default to be remount-ro), so that its behaviour on write errors is, "don't worry, be happy", and just leave the filesystem mounted read/write. That's actually quite dangerous for a critical production server, however.....
I'm not completely sure which "it" you mean in "it is the default today" (seems like you mean errors=continue) but isn't remount-ro already the ext3 default?
see commit: 1eca93f9cafdec4a332ace9b0fc0d3886d430c28
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
In reply to Andrew MortonQuoted 23 lines · Andrew Morton
Quoted 16 lines · On Wed, 4 Jun 2008 12:19:25 +0200 Jan Kara <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
Quoted 3 lines · On Mon, 02 Jun 2008 19:43:57 +0900 Hidehiro Kawai…
In ordered mode, we should abort journaling when an I/O error has occurred on a file data buffer in the committing transaction.
Why should we do that?
I see two reasons:
1) If fs below us is returning IO errors, we don't really know how severe it is so it's safest to stop accepting writes. Also user notices the problem early this way. I agree that with the growing size of disks and thus probability of seeing IO error, we should probably think of something cleverer than this but aborting seems better than just doing nothing.Quoted 4 lines
2) If the IO error is just transient (i.e., link to NAS is disconnected for a while), we would silently break ordering mode guarantees (user could be able to see old / uninitialized data).
Quoted 6 lines
Does any other filesystem driver turn the fs read-only on the first write-IO-error?
It seems like a big policy change to me. For a lot of applications it's effectively a complete outage and people might get a bit upset if this happens on the first blip from their NAS.
I agree with Andrew. The historical policy of ext2/3/4 is that write errors for FILE DATA propagate to the application via EIO, regardless of whether ordered mode is active or not. If filesystem METADATA is involved, yes this should cause an ext3_error() to be called and the policy for what to do next is controlled by the administrator.
If the journal is aborted for a file data write error, the node maybe panics (errors=panic) or is rebooted by the administrator, then e2fsck is run and no problem is found (which it will not because e2fsck does not check file data), then all that has been accomplished is to reboot the node.
Applications should check the error return codes from their writes, and call fsync on the file before closing it if they are really worried about whether the data made it to disk safely, otherwise even a read-only filesystem will not cause the application to notice anything.
Now, if we have a problem where the write error from the journal checkpoint is not being propagated back to the original buffer, that is a different issue. For ordered-mode data I don't _think_ that the data buffers are mirrored when a transaction is closed, so as long as the !buffer_uptodate() error is propagated back to the caller on fsync() that is enough.
We might also consider having a separate mechanism to handle write failures, but I don't think that that should be intermixed with the ext3 error handling for metadata errors.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers
In reply to Andrew MortonHi,
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Mon, 02 Jun 2008 19:43:57 +0900 Hidehiro Kawai…
In ordered mode, we should abort journaling when an I/O error has occurred on a file data buffer in the committing transaction.
Why should we do that?
Aborting the journal leads the filesystem to read-only state, and it can prevent inconsistent state in file data from diffusing furthermore. I think it makes sense for ordered mode users; they don't want to lose file updates as much as possible, but they don't want to do fsync.
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
PATCH 2/5 jbd: ordered data integrity fix
In reply to Hidehiro KawaiSubject: [PATCH 2/5] jbd: ordered data integrity fix
In ordered mode, if a buffer being dirtied exists in the committing transaction, we write the buffer to the disk, move it from the committing transaction to the running transaction, then dirty it. But we don't have to remove the buffer from the committing transaction when the buffer couldn't be written out, otherwise it would miss the error and the committing transaction would not abort.
This patch adds an error check before removing the buffer from the committing transaction.
--- fs/jbd/transaction.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
fs/jbd/transaction.c +13 -3
@@ -954,9 +954,10 @@ int journal_dirty_data(handle_t *handle,journal_t *journal = handle->h_transaction->t_journal;int need_brelse =0 ;struct journal_head *jh; +int ret =0 ;if (is_handle_aborted (handle)) -return 0 ; +return ret; jh =journal_add_journal_head (bh);JBUFFER_TRACE (jh,"entry" );
@@ -1067,7 +1068,16 @@ int journal_dirty_data(handle_t *handle,time if it is redirtied */ } -/* journal_clean_data_list() may have got there first */ +/* +* We cannot remove the buffer with io error from the +* committing transaction, because otherwise it would +* miss the error and the commit would not abort. +*/ +if (unlikely (!buffer_uptodate (bh))) { + ret = -EIO ; + goto no_journal; + } +if (jh->b_transaction !=NULL ) {JBUFFER_TRACE (jh,"unfile from commit" );__journal_temp_unlink_buffer (jh);
@@ -1108,7 +1118,7 @@ no_journal: }JBUFFER_TRACE (jh,"exit" );journal_put_journal_head (jh); -return 0 ; +return ret; }/**
Re: [PATCH 2/5] jbd: ordered data integrity fix
Quoted 9 lines · Hidehiro Kawai
Subject: [PATCH 2/5] jbd: ordered data integrity fix
In ordered mode, if a buffer being dirtied exists in the committing transaction, we write the buffer to the disk, move it from the committing transaction to the running transaction, then dirty it. But we don't have to remove the buffer from the committing transaction when the buffer couldn't be written out, otherwise it would miss the error and the committing transaction would not abort.
Quoted 4 lines
This patch adds an error check before removing the buffer from the committing transaction.
Signed-off-by Hidehiro Kawai <[email protected]>
You can add
Honza
Quoted 49 lines
--- fs/jbd/transaction.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)Index:
linux-2.6.26-rc4/fs/jbd/transaction.c
===================================================================fs/jbd/transaction.c +13
-3@@ -954,9 +954,10 @@ int journal_dirty_data(handle_t *handle,journal_t *journal = handle->h_transaction->t_journal;int need_brelse =0 ;struct journal_head *jh; +int ret =0 ;if (is_handle_aborted (handle)) -return 0 ; +return ret; jh =journal_add_journal_head (bh);JBUFFER_TRACE (jh,"entry" );@@ -1067,7 +1068,16 @@ int journal_dirty_data(handle_t *handle,time if it is redirtied */ } -/* journal_clean_data_list() may have got there first */ +/* +* We cannot remove the buffer with io error from the +* committing transaction, because otherwise it would +* miss the error and the commit would not abort. +*/ +if (unlikely (!buffer_uptodate (bh))) { + ret = -EIO ; + goto no_journal; + } +if (jh->b_transaction !=NULL ) {JBUFFER_TRACE (jh,"unfile from commit" );__journal_temp_unlink_buffer (jh);@@ -1108,7 +1118,7 @@ no_journal: }JBUFFER_TRACE (jh,"exit" );journal_put_journal_head (jh); -return 0 ; +return ret; }/**
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 2/5] jbd: ordered data integrity fix
In reply to Hidehiro KawaiQuoted 1 line · On Mon, 02 Jun 2008 19:45:04 +0900 Hidehiro Kawai…
In ordered mode, if a buffer being dirtied exists in the committing
I changed this text to read "if a file data buffer being dirtied". Please do be careful when describing all these buffers, else it gets more confusing that it already is.
Quoted 8 lines
transaction, we write the buffer to the disk, move it from the committing transaction to the running transaction, then dirty it. But we don't have to remove the buffer from the committing transaction when the buffer couldn't be written out, otherwise it would miss the error and the committing transaction would not abort.
This patch adds an error check before removing the buffer from the committing transaction.
Re: [PATCH 2/5] jbd: ordered data integrity fix
Hi,
Quoted 8 lines · Andrew Morton
Quoted 1 line · On Mon, 02 Jun 2008 19:45:04 +0900 Hidehiro Kawai…
In ordered mode, if a buffer being dirtied exists in the committing
I changed this text to read "if a file data buffer being dirtied". Please do be careful when describing all these buffers, else it gets more confusing that it already is.
Sorry for my misleading description. I'll be careful hereafter.
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
PATCH 3/5 jbd: abort when failed to log metadata buffers
In reply to Hidehiro KawaiSubject: [PATCH 3/5] jbd: abort when failed to log metadata buffers
If we failed to write metadata buffers to the journal space and succeeded to write the commit record, stale data can be written back to the filesystem as metadata in the recovery phase.
To avoid this, when we failed to write out metadata buffers, abort the journal before writing the commit record.
--- fs/jbd/commit.c | 3 +++ 1 file changed, 3 insertions(+)
fs/jbd/commit.c +3 -0
@@ -734,6 +734,9 @@ wait_for_iobuf:/* AKPM: bforget here */ } +if (err) +journal_abort (journal, err); +jbd_debug (3 ,"JBD: commit phase 6\n" );if (journal_write_commit_record (journal, commit_transaction))
Re: [PATCH 3/5] jbd: abort when failed to log metadata buffers
Quoted 10 lines · Hidehiro Kawai
Subject: [PATCH 3/5] jbd: abort when failed to log metadata buffers
If we failed to write metadata buffers to the journal space and succeeded to write the commit record, stale data can be written back to the filesystem as metadata in the recovery phase.
To avoid this, when we failed to write out metadata buffers, abort the journal before writing the commit record.
Signed-off-by Hidehiro Kawai <[email protected]>
Honza
Quoted 20 lines
--- fs/jbd/commit.c | 3 +++ 1 file changed, 3 insertions(+)Index:
linux-2.6.26-rc4/fs/jbd/commit.c
===================================================================fs/jbd/commit.c +3
-0@@ -734,6 +734,9 @@ wait_for_iobuf:/* AKPM: bforget here */ } +if (err) +journal_abort (journal, err); +jbd_debug (3 ,"JBD: commit phase 6\n" );if (journal_write_commit_record (journal, commit_transaction))
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 3/5] jbd: abort when failed to log metadata buffers
In reply to Hidehiro KawaiQuoted 20 lines · On Mon, 02 Jun 2008 19:46:02 +0900 Hidehiro Kawai…
Subject: [PATCH 3/5] jbd: abort when failed to log metadata buffers
If we failed to write metadata buffers to the journal space and succeeded to write the commit record, stale data can be written back to the filesystem as metadata in the recovery phase.
To avoid this, when we failed to write out metadata buffers, abort the journal before writing the commit record.
Signed-off-by Hidehiro Kawai <[email protected]>--- fs/jbd/commit.c | 3 +++ 1 file changed, 3 insertions(+)Index:
linux-2.6.26-rc4/fs/jbd/commit.c
===================================================================
fs/jbd/commit.c wait_for_iobuf: · line 742
Quoted 9 lines
fs/jbd/commit.c +3
-0@@ -734,6 +734,9 @@ wait_for_iobuf:/* AKPM: bforget here */ } +if (err) +journal_abort (journal, err); +jbd_debug (3 ,"JBD: commit phase 6\n" );if (journal_write_commit_record (journal, commit_transaction))
I assume this has all been tested?
How are you finding these problems and testing the fixes? Fault injection?
Does it make sense to proceed into phase 6 here, or should we bale out of commit at this point?
Re: [PATCH 3/5] jbd: abort when failed to log metadata buffers
Hi,
Quoted 30 lines · Andrew Morton
Quoted 27 lines · On Mon, 02 Jun 2008 19:46:02 +0900 Hidehiro Kawai…
Subject: [PATCH 3/5] jbd: abort when failed to log metadata buffers
If we failed to write metadata buffers to the journal space and succeeded to write the commit record, stale data can be written back to the filesystem as metadata in the recovery phase.
To avoid this, when we failed to write out metadata buffers, abort the journal before writing the commit record.
Signed-off-by Hidehiro Kawai <[email protected]>---
fs/jbd/commit.c| 3 +++1 file changed, 3 insertions(+)Index:
linux-2.6.26-rc4/fs/jbd/commit.c
===================================================================fs/jbd/commit.c +3
-0@@ -734,6 +734,9 @@ wait_for_iobuf:/* AKPM: bforget here */ } +if (err) +journal_abort (journal, err); +jbd_debug (3 ,"JBD: commit phase 6\n" );
Quoted 5 lines
fs/jbd/commit.c wait_for_iobuf: · line 742
Quoted 2 lines
fs/jbd/commit.c +0
-0if (journal_write_commit_record (journal, commit_transaction))I assume this has all been tested?
Yes, I tested all cases except for the following case (related to PATCH 4/5):
Quoted 13 lines
o
journal_flush()usesj_checkpoint_mutexto avoid a race with__log_wait_for_space()The last item targets a newly found problem.
journal_flush()can be called while processing__log_wait_for_space(). In this case,cleanup_journal_tail()can be called between__journal_drop_transaction()andjournal_abort(), then
the transaction with checkpointing failure is lost from the journal. Usingj_checkpoint_mutexwhich is used by__log_wait_for_space(), we should avoid the race condition. But the test is not so sufficient because it is very difficult to produce this race. So I hope that this locking is reviewed carefully (including a possibility of deadlock.)
I caused invocations of journal_flush() and __log_wait_for_space() and a write error simultaneously, but I haven't confirmed the race had occurred.
Quoted 2 lines
How are you finding these problems and testing the fixes? Fault injection?
I found these problems by reading souce codes, then tested them by the fault injection approach. To inject a fault, I used a SystemTap script at the bottom of this mail.
Quoted 2 lines
Does it make sense to proceed into phase 6 here, or should we bale out of commit at this point?
What I really want to do is that don't write the commit record when metadata buffers couldn't be written to the journal.
It should be no problem in the case of writing revoke records failure because the recovery process detects the invalid control block with a noncontiguous sequence number.
But it is nonsense to write the commit record even though we failed to write control blocks to the journal. So I think it makes sense to catch errors for all writes to the journal here and abort the journal to avoid writing the commit record.
* * * * * *
The following SystemTap script was used to inject a fault. Please don't use this script without changing. It is hard-coded for my environment.
global target_inode_block = 64
/*
* Inject a fault when a particular metadata buffer is journaled. */
%{
#include <linux/buffer_head.h>
#include <linux/jbd.h>
#include <scsi/scsi.h>
#include <scsi/scsi_cmnd.h>
enum fi_state_bits {
BH_Faulty = BH_Unshadow + 1,
};
%}
function fault_inject (scmd: long) %{
struct scsi_cmnd *cmd = (void *)((unsigned long)THIS->scmd);
cmd->cmnd[0] |= (7 << 5);
cmd->cmd_len = 255;%}
global do_fault_inject
global faulty_sector
probe module("jbd").function("journal_write_metadata_buffer") {
if ($jh_in->b_bh->b_blocknr == target_inode_block) {
do_fault_inject[tid()] = 1
}
}
probe module("jbd").function("journal_write_metadata_buffer").return {
do_fault_inject[tid()] = 0
}probe module("jbd").function("journal_file_buffer") {
if (do_fault_inject[tid()] && $jlist == 4 /* BJ_IO */) {
faulty_sector[$jh->b_bh->b_blocknr * 8 + 63] = 1
printf("mark faulty @ sector=%d\n",
$jh->b_bh->b_blocknr * 8 + 63)
}
}probe kernel.function("scsi_dispatch_cmd") {
host = $cmd->device->host->host_no
id = $cmd->device->id
lun = $cmd->device->lun
ch = $cmd->device->channel
sector = $cmd->request->bio->bi_sector
len = $cmd->transfersize / 512
if (id != 1) {
next
}
printf("%d:%d:%d:%d, #%d+%d\n", host, ch, id, lun, sector, len)
if ($cmd->request->cmd_flags & 1 == 1 && faulty_sector[sector]) {
delete faulty_sector[sector]
fault_inject($cmd)
printf("fault injected\n")
}
}Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
PATCH 4/5 jbd: fix error handling for checkpoint io
In reply to Hidehiro KawaiSubject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of log_do_checkpoint(). To achieve this, we need to do:
1. don't remove the failed buffer from the checkpoint list where in the case of __try_to_free_cp_buf() because it may be released or overwritten by a later transaction
2. log_do_checkpoint() is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't update j_tail and j_tail_sequence either
4. when checkpointing fails, notify this error to the ext3 layer so that ext3 don't clear the needs_recovery flag, otherwise the journaled contents are ignored and cleaned in the recovery phase
5. if the recovery fails, keep the needs_recovery flag
6. prevent cleanup_journal_tail() from being called between __journal_drop_transaction() and journal_abort() (a race issue between journal_flush() and __log_wait_for_space()
--- fs/jbd/checkpoint.c | 48 +++++++++++++++++++++++++++++++----------- fs/jbd/journal.c | 28 +++++++++++++++++++----- fs/jbd/recovery.c | 7 ++++-- include/linux/jbd.h | 2 - 4 files changed, 64 insertions(+), 21 deletions(-)
fs/jbd/checkpoint.c +36 -12
@@ -93,7 +93,8 @@ static int __try_to_free_cp_buf(struct jint ret =0 ;struct buffer_head *bh =jh2bh (jh); -if (jh->b_jlist == BJ_None && !buffer_locked (bh) && !buffer_dirty (bh)) { +if (jh->b_jlist == BJ_None && !buffer_locked (bh) && + !buffer_dirty (bh) &&buffer_uptodate (bh)) {JBUFFER_TRACE (jh,"remove from checkpoint list" ); ret =__journal_remove_checkpoint (jh) +1 ;jbd_unlock_bh_state (bh);
@@ -160,21 +161,25 @@ static void jbd_sync_bh(journal_t *journ* buffers. Note that we take the buffers in the opposite ordering * from the one in which they were submitted for IO. * +* Return 0 on success, and return <0 if some buffers have failed +* to be written out. +* * Called with j_list_lock held. */ -static void __wait_cp_io (journal_t *journal,transaction_t *transaction) +static int __wait_cp_io (journal_t *journal,transaction_t *transaction) {struct journal_head *jh;struct buffer_head *bh;tid_t this_tid;int released =0 ; +int ret =0 ; this_tid = transaction->t_tid; restart:/* Did somebody clean up the transaction in the meanwhile? */ if (journal->j_checkpoint_transactions != transaction || transaction->t_tid != this_tid) -return ; +return ret;while (!released && transaction->t_checkpoint_io_list) { jh = transaction->t_checkpoint_io_list; bh =jh2bh (jh);
@@ -194,6 +199,9 @@ restart:spin_lock (&journal->j_list_lock); goto restart; } +if (unlikely (!buffer_uptodate (bh))) + ret = -EIO ; +/* * Now in whatever state the buffer currently is, we know that * it has been written out and so we can drop it from the list
@@ -203,6 +211,8 @@ restart:journal_remove_journal_head (bh);__brelse (bh); } + +return ret; }#define NR_BATCH 64
@@ -226,7 +236,8 @@ __flush_batch(journal_t *journal, struct* Try to flush one buffer from the checkpoint list to disk. * * Return 1 if something happened which requires us to abort the current -* scan of the checkpoint list. +* scan of the checkpoint list. Return <0 if the buffer has failed to +* be written out. * * Called with j_list_lock held and drops it if 1 is returned * Called under jbd_lock_bh_state(jh2bh(jh)), and drops it
@@ -256,6 +267,9 @@ static int __process_buffer(journal_t *jlog_wait_commit (journal, tid); ret =1 ; }else if (!buffer_dirty (bh)) { + ret =1 ; +if (unlikely (!buffer_uptodate (bh))) + ret = -EIO ;J_ASSERT_JH (jh, !buffer_jbddirty (bh));BUFFER_TRACE (bh,"remove from checkpoint" );__journal_remove_checkpoint (jh);
@@ -263,7 +277,6 @@ static int __process_buffer(journal_t *jjbd_unlock_bh_state (bh);journal_remove_journal_head (bh);__brelse (bh); - ret =1 ; }else {/* * Important: we are about to write the buffer, and
@@ -318,6 +331,7 @@ int log_do_checkpoint(journal_t *journal* OK, we need to start writing disk blocks. Take one transaction * and write it. */ + result =0 ;spin_lock (&journal->j_list_lock);if (!journal->j_checkpoint_transactions) goto out;
@@ -334,7 +348,7 @@ restart:int batch_count =0 ;struct buffer_head *bhs[NR_BATCH ];struct journal_head *jh; -int retry =0 ; +int retry =0 , err;while (!retry && transaction->t_checkpoint_list) {struct buffer_head *bh;
@@ -347,6 +361,8 @@ restart:break ; } retry =__process_buffer (journal, jh, bhs,&batch_count); +if (retry <0 ) + result = retry;if (!retry && (need_resched () ||spin_needbreak (&journal->j_list_lock))) {spin_unlock (&journal->j_list_lock);
@@ -371,14 +387,18 @@ restart:* Now we have cleaned up the first transaction's checkpoint * list. Let's clean up the second one */ -__wait_cp_io (journal, transaction); + err =__wait_cp_io (journal, transaction); +if (!result) + result = err; } out:spin_unlock (&journal->j_list_lock); - result =cleanup_journal_tail (journal);if (result <0 ) -return result; -return 0 ; +journal_abort (journal, result); +else + result =cleanup_journal_tail (journal); + +return (result <0 ) ? result :0 ; }/*
@@ -394,8 +414,9 @@ out:* This is the only part of the journaling code which really needs to be * aware of transaction aborts. Checkpointing involves writing to the * main filesystem area rather than to the journal, so it can proceed -* even in abort state, but we must not update the journal superblock if -* we have an abort error outstanding. +* even in abort state, but we must not update the super block if +* checkpointing may have failed. Otherwise, we would lose some metadata +* buffers which should be written-back to the filesystem. */ int cleanup_journal_tail (journal_t *journal)
@@ -404,6 +425,9 @@ int cleanup_journal_tail(journal_t *jourtid_t first_tid;unsigned long blocknr, freed; +if (is_journal_aborted (journal)) +return 1 ; +/* OK, work out the oldest transaction remaining in the log, and * the log block it starts at. *
fs/jbd/journal.c +22 -6
@@ -1122,9 +1122,12 @@ recovery_error:* * Release a journal_t structure once it is no longer in use by the * journaled object. +* Return <0 if we couldn't clean up the journal. */ -void journal_destroy (journal_t *journal) +int journal_destroy (journal_t *journal) { +int err =0 ; +/* Wait for the commit thread to wake up and die. */ journal_kill_thread (journal);
@@ -1147,11 +1150,16 @@ void journal_destroy(journal_t *journal)J_ASSERT (journal->j_checkpoint_transactions ==NULL );spin_unlock (&journal->j_list_lock); -/* We can now mark the journal as empty. */ - journal->j_tail =0 ; - journal->j_tail_sequence = ++journal->j_transaction_sequence;if (journal->j_sb_buffer) { -journal_update_superblock (journal,1 ); +if (!is_journal_aborted (journal)) { +/* We can now mark the journal as empty. */ + journal->j_tail =0 ; + journal->j_tail_sequence = + ++journal->j_transaction_sequence; +journal_update_superblock (journal,1 ); + }else { + err = -EIO ; + }brelse (journal->j_sb_buffer); }
@@ -1161,6 +1169,8 @@ void journal_destroy(journal_t *journal)journal_destroy_revoke (journal);kfree (journal->j_wbuf);kfree (journal); + +return err; }
@@ -1360,10 +1370,16 @@ int journal_flush(journal_t *journal)spin_lock (&journal->j_list_lock);while (!err && journal->j_checkpoint_transactions !=NULL ) {spin_unlock (&journal->j_list_lock); +mutex_lock (&journal->j_checkpoint_mutex); err =log_do_checkpoint (journal); +mutex_unlock (&journal->j_checkpoint_mutex);spin_lock (&journal->j_list_lock); }spin_unlock (&journal->j_list_lock); + +if (is_journal_aborted (journal)) +return -EIO ; +cleanup_journal_tail (journal);/* Finally, mark the journal as really needing no recovery.
@@ -1385,7 +1401,7 @@ int journal_flush(journal_t *journal)J_ASSERT (journal->j_head == journal->j_tail);J_ASSERT (journal->j_tail_sequence == journal->j_transaction_sequence);spin_unlock (&journal->j_state_lock); -return err; +return 0 ; }/**
fs/jbd/recovery.c +5 -2
@@ -223,7 +223,7 @@ do { \*/ int journal_recover (journal_t *journal) { -int err; +int err, err2;journal_superblock_t * sb;struct recovery_info info;
@@ -261,7 +261,10 @@ int journal_recover(journal_t *journal) journal->j_transaction_sequence = ++info.end_transaction;journal_clear_revoke (journal); -sync_blockdev (journal->j_fs_dev); + err2 =sync_blockdev (journal->j_fs_dev); +if (!err) + err = err2; +return err; }
include/linux/jbd.h +1 -1
@@ -908,7 +908,7 @@ extern int journal_set_features (journal_t *, unsigned long, unsigned long, unsigned long);extern int journal_create (journal_t *);extern int journal_load (journal_t *journal); -extern void journal_destroy (journal_t *); +extern int journal_destroy (journal_t *);extern int journal_recover (journal_t *journal);extern int journal_wipe (journal_t *,int );extern int journal_skip_recovery (journal_t *);
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Quoted 26 lines · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of
log_do_checkpoint(). To achieve this, we need to do:1. don't remove the failed buffer from the checkpoint list where in the case of
__try_to_free_cp_buf()because it may be released or overwritten by a later transaction
2.log_do_checkpoint()is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't updatej_tailandj_tail_sequenceeither
4. when checkpointing fails, notify this error to the ext3 layer so that ext3 don't clear theneeds_recoveryflag, otherwise the journaled contents are ignored and cleaned in the recovery phase
5. if the recovery fails, keep theneeds_recoveryflag
6. preventcleanup_journal_tail()from being called between__journal_drop_transaction()andjournal_abort()(a race issue betweenjournal_flush()and__log_wait_for_space()
Quoted 1 line
Signed-off-by Hidehiro Kawai <[email protected]>
Just a few minor comments:
Quoted 5 lines
Index:
linux-2.6.26-rc4/fs/jbd/checkpoint.c
===================================================================fs/jbd/checkpoint.c +0
-0
<snip>
Quoted 18 lines
fs/jbd/checkpoint.c +2
-1@@ -318,6 +331,7 @@ int log_do_checkpoint(journal_t *journal* OK, we need to start writing disk blocks. Take one transaction * and write it. */ + result =0 ;spin_lock (&journal->j_list_lock);if (!journal->j_checkpoint_transactions) goto out;@@ -334,7 +348,7 @@ restart:int batch_count =0 ;struct buffer_head *bhs[NR_BATCH ];struct journal_head *jh; -int retry =0 ; +int retry =0 , err;while (!retry && transaction->t_checkpoint_list) {struct buffer_head *bh;
fs/jbd/checkpoint.c restart: · line 365
Quoted 5 lines
fs/jbd/checkpoint.c +2
-0@@ -347,6 +361,8 @@ restart:break ; } retry =__process_buffer (journal, jh, bhs,&batch_count); +if (retry <0 ) + result = retry;
Here you update result whenever retry is < 0 and below when result == 0.
I think it's better to have these two consistent (not that it would be currently any functional difference).
Quoted 20 lines
fs/jbd/checkpoint.c +5
-1if (!retry && (need_resched () ||spin_needbreak (&journal->j_list_lock))) {spin_unlock (&journal->j_list_lock);@@ -371,14 +387,18 @@ restart:* Now we have cleaned up the first transaction's checkpoint * list. Let's clean up the second one */ -__wait_cp_io (journal, transaction); + err =__wait_cp_io (journal, transaction); +if (!result) + result = err; }@@ -1360,10 +1370,16 @@ int journal_flush(journal_t *journal)spin_lock (&journal->j_list_lock);while (!err && journal->j_checkpoint_transactions !=NULL ) {spin_unlock (&journal->j_list_lock); +mutex_lock (&journal->j_checkpoint_mutex); err =log_do_checkpoint (journal); +mutex_unlock (&journal->j_checkpoint_mutex);
fs/jbd/checkpoint.c int journal_flush(journal_t *journal) · line 1385
Quoted 10 lines
fs/jbd/checkpoint.c +4
-0spin_lock (&journal->j_list_lock); }spin_unlock (&journal->j_list_lock); + +if (is_journal_aborted (journal)) +return -EIO ; +cleanup_journal_tail (journal);/* Finally, mark the journal as really needing no recovery.
OK, so this way you've basically serialized all users oflog_do_checkpoint(). That should be fine because performance-wise interesting is only log_wait_for_space() and that was already serialized before. So this change is fine with me. Only please add a comment in front of log_do_checkpoint() that it's supposed to be called with j_checkpoint_mutex held so that EIO propagation works correctly.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Hello,
Thank you for reviewing.
Quoted 10 lines · Jan Kara
fs/jbd/checkpoint.c restart: · line 365
Quoted 6 lines
fs/jbd/checkpoint.c +2
-0@@ -347,6 +361,8 @@ restart:break ; } retry =__process_buffer (journal, jh, bhs,&batch_count); +if (retry <0 ) + result = retry;Here you update result whenever retry is < 0 and below when result == 0.
I think it's better to have these two consistent (not that it would be currently any functional difference).
I fixed it.
Quoted 3 lines
Only please add a comment in front of
log_do_checkpoint()that it's supposed to be called withj_checkpoint_mutexheld so that EIO propagation works correctly.
I added a comment.
I'll send the revised patch soon.
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
PATCH 4/5 jbd: fix error handling for checkpoint io
In reply to Jan KaraSubject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of log_do_checkpoint(). To achieve this, we need to do:
1. don't remove the failed buffer from the checkpoint list where in the case of __try_to_free_cp_buf() because it may be released or overwritten by a later transaction
2. log_do_checkpoint() is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't update j_tail and j_tail_sequence either
4. when checkpointing fails, notify this error to the ext3 layer so that ext3 don't clear the needs_recovery flag, otherwise the journaled contents are ignored and cleaned in the recovery phase
5. if the recovery fails, keep the needs_recovery flag
6. prevent cleanup_journal_tail() from being called between __journal_drop_transaction() and journal_abort() (a race issue between journal_flush() and __log_wait_for_space()
--- fs/jbd/checkpoint.c | 49 +++++++++++++++++++++++++++++++----------- fs/jbd/journal.c | 28 ++++++++++++++++++------ fs/jbd/recovery.c | 7 ++++-- include/linux/jbd.h | 2 - 4 files changed, 65 insertions(+), 21 deletions(-)
fs/jbd/checkpoint.c +37 -12
@@ -93,7 +93,8 @@ static int __try_to_free_cp_buf(struct jint ret =0 ;struct buffer_head *bh =jh2bh (jh); -if (jh->b_jlist == BJ_None && !buffer_locked (bh) && !buffer_dirty (bh)) { +if (jh->b_jlist == BJ_None && !buffer_locked (bh) && + !buffer_dirty (bh) &&buffer_uptodate (bh)) {JBUFFER_TRACE (jh,"remove from checkpoint list" ); ret =__journal_remove_checkpoint (jh) +1 ;jbd_unlock_bh_state (bh);
@@ -160,21 +161,25 @@ static void jbd_sync_bh(journal_t *journ* buffers. Note that we take the buffers in the opposite ordering * from the one in which they were submitted for IO. * +* Return 0 on success, and return <0 if some buffers have failed +* to be written out. +* * Called with j_list_lock held. */ -static void __wait_cp_io (journal_t *journal,transaction_t *transaction) +static int __wait_cp_io (journal_t *journal,transaction_t *transaction) {struct journal_head *jh;struct buffer_head *bh;tid_t this_tid;int released =0 ; +int ret =0 ; this_tid = transaction->t_tid; restart:/* Did somebody clean up the transaction in the meanwhile? */ if (journal->j_checkpoint_transactions != transaction || transaction->t_tid != this_tid) -return ; +return ret;while (!released && transaction->t_checkpoint_io_list) { jh = transaction->t_checkpoint_io_list; bh =jh2bh (jh);
@@ -194,6 +199,9 @@ restart:spin_lock (&journal->j_list_lock); goto restart; } +if (unlikely (!buffer_uptodate (bh))) + ret = -EIO ; +/* * Now in whatever state the buffer currently is, we know that * it has been written out and so we can drop it from the list
@@ -203,6 +211,8 @@ restart:journal_remove_journal_head (bh);__brelse (bh); } + +return ret; }#define NR_BATCH 64
@@ -226,7 +236,8 @@ __flush_batch(journal_t *journal, struct* Try to flush one buffer from the checkpoint list to disk. * * Return 1 if something happened which requires us to abort the current -* scan of the checkpoint list. +* scan of the checkpoint list. Return <0 if the buffer has failed to +* be written out. * * Called with j_list_lock held and drops it if 1 is returned * Called under jbd_lock_bh_state(jh2bh(jh)), and drops it
@@ -256,6 +267,9 @@ static int __process_buffer(journal_t *jlog_wait_commit (journal, tid); ret =1 ; }else if (!buffer_dirty (bh)) { + ret =1 ; +if (unlikely (!buffer_uptodate (bh))) + ret = -EIO ;J_ASSERT_JH (jh, !buffer_jbddirty (bh));BUFFER_TRACE (bh,"remove from checkpoint" );__journal_remove_checkpoint (jh);
@@ -263,7 +277,6 @@ static int __process_buffer(journal_t *jjbd_unlock_bh_state (bh);journal_remove_journal_head (bh);__brelse (bh); - ret =1 ; }else {/* * Important: we are about to write the buffer, and
@@ -295,6 +308,7 @@ static int __process_buffer(journal_t *j* to disk. We submit larger chunks of data at once. * * The journal should be locked before calling this function. +* Called with j_checkpoint_mutex held. */ int log_do_checkpoint (journal_t *journal) {
@@ -318,6 +332,7 @@ int log_do_checkpoint(journal_t *journal* OK, we need to start writing disk blocks. Take one transaction * and write it. */ + result =0 ;spin_lock (&journal->j_list_lock);if (!journal->j_checkpoint_transactions) goto out;
@@ -334,7 +349,7 @@ restart:int batch_count =0 ;struct buffer_head *bhs[NR_BATCH ];struct journal_head *jh; -int retry =0 ; +int retry =0 , err;while (!retry && transaction->t_checkpoint_list) {struct buffer_head *bh;
@@ -347,6 +362,8 @@ restart:break ; } retry =__process_buffer (journal, jh, bhs,&batch_count); +if (retry <0 && !result) + result = retry;if (!retry && (need_resched () ||spin_needbreak (&journal->j_list_lock))) {spin_unlock (&journal->j_list_lock);
@@ -371,14 +388,18 @@ restart:* Now we have cleaned up the first transaction's checkpoint * list. Let's clean up the second one */ -__wait_cp_io (journal, transaction); + err =__wait_cp_io (journal, transaction); +if (!result) + result = err; } out:spin_unlock (&journal->j_list_lock); - result =cleanup_journal_tail (journal);if (result <0 ) -return result; -return 0 ; +journal_abort (journal, result); +else + result =cleanup_journal_tail (journal); + +return (result <0 ) ? result :0 ; }/*
@@ -394,8 +415,9 @@ out:* This is the only part of the journaling code which really needs to be * aware of transaction aborts. Checkpointing involves writing to the * main filesystem area rather than to the journal, so it can proceed -* even in abort state, but we must not update the journal superblock if -* we have an abort error outstanding. +* even in abort state, but we must not update the super block if +* checkpointing may have failed. Otherwise, we would lose some metadata +* buffers which should be written-back to the filesystem. */ int cleanup_journal_tail (journal_t *journal)
@@ -404,6 +426,9 @@ int cleanup_journal_tail(journal_t *jourtid_t first_tid;unsigned long blocknr, freed; +if (is_journal_aborted (journal)) +return 1 ; +/* OK, work out the oldest transaction remaining in the log, and * the log block it starts at. *
fs/jbd/journal.c +22 -6
@@ -1122,9 +1122,12 @@ recovery_error:* * Release a journal_t structure once it is no longer in use by the * journaled object. +* Return <0 if we couldn't clean up the journal. */ -void journal_destroy (journal_t *journal) +int journal_destroy (journal_t *journal) { +int err =0 ; +/* Wait for the commit thread to wake up and die. */ journal_kill_thread (journal);
@@ -1147,11 +1150,16 @@ void journal_destroy(journal_t *journal)J_ASSERT (journal->j_checkpoint_transactions ==NULL );spin_unlock (&journal->j_list_lock); -/* We can now mark the journal as empty. */ - journal->j_tail =0 ; - journal->j_tail_sequence = ++journal->j_transaction_sequence;if (journal->j_sb_buffer) { -journal_update_superblock (journal,1 ); +if (!is_journal_aborted (journal)) { +/* We can now mark the journal as empty. */ + journal->j_tail =0 ; + journal->j_tail_sequence = + ++journal->j_transaction_sequence; +journal_update_superblock (journal,1 ); + }else { + err = -EIO ; + }brelse (journal->j_sb_buffer); }
@@ -1161,6 +1169,8 @@ void journal_destroy(journal_t *journal)journal_destroy_revoke (journal);kfree (journal->j_wbuf);kfree (journal); + +return err; }
@@ -1360,10 +1370,16 @@ int journal_flush(journal_t *journal)spin_lock (&journal->j_list_lock);while (!err && journal->j_checkpoint_transactions !=NULL ) {spin_unlock (&journal->j_list_lock); +mutex_lock (&journal->j_checkpoint_mutex); err =log_do_checkpoint (journal); +mutex_unlock (&journal->j_checkpoint_mutex);spin_lock (&journal->j_list_lock); }spin_unlock (&journal->j_list_lock); + +if (is_journal_aborted (journal)) +return -EIO ; +cleanup_journal_tail (journal);/* Finally, mark the journal as really needing no recovery.
@@ -1385,7 +1401,7 @@ int journal_flush(journal_t *journal)J_ASSERT (journal->j_head == journal->j_tail);J_ASSERT (journal->j_tail_sequence == journal->j_transaction_sequence);spin_unlock (&journal->j_state_lock); -return err; +return 0 ; }/**
fs/jbd/recovery.c +5 -2
@@ -223,7 +223,7 @@ do { \*/ int journal_recover (journal_t *journal) { -int err; +int err, err2;journal_superblock_t * sb;struct recovery_info info;
@@ -261,7 +261,10 @@ int journal_recover(journal_t *journal) journal->j_transaction_sequence = ++info.end_transaction;journal_clear_revoke (journal); -sync_blockdev (journal->j_fs_dev); + err2 =sync_blockdev (journal->j_fs_dev); +if (!err) + err = err2; +return err; }
include/linux/jbd.h +1 -1
@@ -908,7 +908,7 @@ extern int journal_set_features (journal_t *, unsigned long, unsigned long, unsigned long);extern int journal_create (journal_t *);extern int journal_load (journal_t *journal); -extern void journal_destroy (journal_t *); +extern int journal_destroy (journal_t *);extern int journal_recover (journal_t *journal);extern int journal_wipe (journal_t *,int );extern int journal_skip_recovery (journal_t *);
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Quoted 1 line · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
Sorry, I forgot to remove the subject at the first line...
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Quoted 5 lines · On Tue, 03 Jun 2008 14:11:44 +0900 Hidehiro Kawai…
Quoted 1 line · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
Sorry, I forgot to remove the subject at the first line...
Is OK ;)
Sometimes it helps, when people use WeirdRandomStuff in their patch titles, or when the patch comes in reply to some email thread which has an irrelevant Subject:.
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
In reply to Hidehiro KawaiQuoted 26 lines · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of
log_do_checkpoint(). To achieve this, we need to do:1. don't remove the failed buffer from the checkpoint list where in the case of
__try_to_free_cp_buf()because it may be released or overwritten by a later transaction
2.log_do_checkpoint()is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't updatej_tailandj_tail_sequenceeither
4. when checkpointing fails, notify this error to the ext3 layer so that ext3 don't clear theneeds_recoveryflag, otherwise the journaled contents are ignored and cleaned in the recovery phase
5. if the recovery fails, keep theneeds_recoveryflag
6. preventcleanup_journal_tail()from being called between__journal_drop_transaction()andjournal_abort()(a race issue betweenjournal_flush()and__log_wait_for_space()
Quoted 1 line
Signed-off-by Hidehiro Kawai <[email protected]>
You can add:
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Hi,
I noticed a problem of this patch. Please see below.
Quoted 12 lines · Jan Kara
Quoted 10 lines · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of
log_do_checkpoint(). To achieve this, we need to do:
Quoted 8 lines
Quoted 8 lines
1. don't remove the failed buffer from the checkpoint list where in the case of
__try_to_free_cp_buf()because it may be released or overwritten by a later transaction
2.log_do_checkpoint()is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't updatej_tailandj_tail_sequenceeither
3. is implemented as described below.
(1) if log_do_checkpoint() detects an I/O error during checkpointing, it calls journal_abort() to abort the journal
(2) if the journal has aborted, don't update s_start and s_sequence in the on-disk journal superblock
So, if the journal aborts, journaled data will be replayed on the next mount.
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
My idea to resolve this problem is that we don't write out metadata buffers which belong to uncommitted transactions if journal has aborted. Although the latest filesystem updates will be lost, we can ensure the integrity. It will also be effective for the kernel panic in the middle of writing metadata buffers without journaling (this would occur in the `mount -o errors=panic' case.)
Which integrity or latest state should we choose?
--- fs/jbd/commit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
fs/jbd/commit.c +4 -1
@@ -486,9 +486,10 @@ void journal_commit_transaction(journal_ jh = commit_transaction->t_buffers;/* If we're in abort mode, we just un-journal the buffer and -release it for background writing. */ +release it. */ if (is_journal_aborted (journal)) { +clear_buffer_jbddirty (jh2bh (jh));JBUFFER_TRACE (jh,"journal is aborting: refile" );journal_refile_buffer (journal, jh);/* If that was the last one, we need to clean up
@@ -823,6 +824,8 @@ restart_loop:if (buffer_jbddirty (bh)) {JBUFFER_TRACE (jh,"add to new checkpointing trans" );__journal_insert_checkpoint (jh, commit_transaction); +if (is_journal_aborted (journal)) +clear_buffer_jbddirty (bh);JBUFFER_TRACE (jh,"refile for checkpoint writeback" );__journal_refile_buffer (jh);jbd_unlock_bh_state (bh);
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Quoted 36 lines · Hidehiro Kawai
Hi,
I noticed a problem of this patch. Please see below.
Quoted 12 lines · Jan Kara
Quoted 10 lines · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of
log_do_checkpoint(). To achieve this, we need to do:Quoted 8 lines
Quoted 8 lines
1. don't remove the failed buffer from the checkpoint list where in the case of
__try_to_free_cp_buf()because it may be released or overwritten by a later transaction
2.log_do_checkpoint()is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't updatej_tailandj_tail_sequenceeither3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Yes, it would. But how do you think it can happen that a metadata buffer
will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...
Quoted 8 lines
My idea to resolve this problem is that we don't write out metadata buffers which belong to uncommitted transactions if journal has aborted. Although the latest filesystem updates will be lost, we can ensure the integrity. It will also be effective for the kernel panic in the middle of writing metadata buffers without journaling (this would occur in the `mount -o errors=panic' case.)
Which integrity or latest state should we choose?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Quoted 47 lines · Jan Kara
Quoted 38 lines · Hidehiro Kawai
Hi,
I noticed a problem of this patch. Please see below.
Quoted 13 lines · Jan Kara
Quoted 10 lines · Hidehiro Kawai
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of
log_do_checkpoint(). To achieve this, we need to do:Quoted 8 lines
Quoted 8 lines
1. don't remove the failed buffer from the checkpoint list where in the case of
__try_to_free_cp_buf()because it may be released or overwritten by a later transaction
2.log_do_checkpoint()is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't updatej_tailandj_tail_sequenceeither3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Quoted 6 lines
Yes, it would. But how do you think it can happen that a metadata buffer
will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...
Checkpointing code checks it and may call log_wait_commit(), but this problem is caused by transactions which have not started checkpointing.
For example, the tail transaction has an old update for block_B and the running transaction has a new update for block_B. Then, the committing transaction fails to write the commit record, it aborts the journal, and new block_B will be written back to the file system without journaling. Because this patch doesn't separate between normal abort and checkpointing related abort, the tail transaction is left in the journal space. So by replaying the tail transaction, new block_B is overwritten with old one.
It can happen in the case of the checkpointing related abort. For example, assuming the tail transaction has an update for block_A, the next transaction has an old update for block_B, and the running transaction has a new update for block_B.
Now, the running transaction needs more log space, and it calls log_do_checkpoint(). But it aborts the journal because it detected write error on block_A. In this case, new block_B will be overwritten when the old block_B in the second transaction to the tail is replayed.
Does this answer your question?
Quoted 8 lines
Quoted 8 lines
My idea to resolve this problem is that we don't write out metadata buffers which belong to uncommitted transactions if journal has aborted. Although the latest filesystem updates will be lost, we can ensure the integrity. It will also be effective for the kernel panic in the middle of writing metadata buffers without journaling (this would occur in the `mount -o errors=panic' case.)
Which integrity or latest state should we choose?
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Quoted 45 lines · Hidehiro Kawai
Quoted 35 lines
Quoted 28 lines
Quoted 10 lines
Quoted 10 lines
Subject: [PATCH 4/5] jbd: fix error handling for checkpoint io
When a checkpointing IO fails, current JBD code doesn't check the error and continue journaling. This means latest metadata can be lost from both the journal and filesystem.
This patch leaves the failed metadata blocks in the journal space and aborts journaling in the case of
log_do_checkpoint(). To achieve this, we need to do:Quoted 8 lines
Quoted 8 lines
1. don't remove the failed buffer from the checkpoint list where in the case of
__try_to_free_cp_buf()because it may be released or overwritten by a later transaction
2.log_do_checkpoint()is the last chance, remove the failed buffer from the checkpoint list and abort the journal
3. when checkpointing fails, don't update the journal super block to prevent the journaled contents from being cleaned. For safety, don't updatej_tailandj_tail_sequenceeither3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Quoted 6 lines
Yes, it would. But how do you think it can happen that a metadata buffer
will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...Checkpointing code checks it and may call
log_wait_commit(), but this problem is caused by transactions which have not started checkpointing.
Quoted 8 lines
For example, the tail transaction has an old update for
block_Band the running transaction has a new update forblock_B. Then, the committing transaction fails to write the commit record, it aborts the journal, and newblock_Bwill be written back to the file system without journaling. Because this patch doesn't separate between normal abort and checkpointing related abort, the tail transaction is left in the journal space. So by replaying the tail transaction, newblock_Bis overwritten with old one.
Yes, and this is expected an correct. When we cannot properly finish a
transaction, we have to discard everything in it. A bug would be (and I think it could currently happen) if we already checkpointed the previous transaction and then written over block_B new data from the uncommitted transaction. I think we have to avoid that - i.e., in case we abort the journal we should not mark buffers dirty when processing the forget loop. But this is not too serious since fsck has to be run anyway and it will fix the problems.
Quoted 9 lines
It can happen in the case of the checkpointing related abort. For example, assuming the tail transaction has an update for
block_A, the next transaction has an old update forblock_B, and the running transaction has a new update forblock_B.
Now, the running transaction needs more log space, and it callslog_do_checkpoint(). But it aborts the journal because it detected write error onblock_A. In this case, newblock_Bwill be overwritten when the oldblock_Bin the second transaction to the tail is replayed.
Well, the scenario has to be a bit different (if we need more space than
there is in the journal, we commit the running transaction, do checkpoint and start a new transaction) but something like what you describe could happen. But again I think that this is a correct behavior - i.e., discard all the data in the running transaction when the journal is aborted before the transaction is properly committed.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Hi Jan,
Thank you for your reply.
Quoted 37 lines · Jan Kara
Quoted 26 lines · Hidehiro Kawai
Quoted 16 lines
Quoted 9 lines
3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Quoted 6 lines
Yes, it would. But how do you think it can happen that a metadata buffer
will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...Checkpointing code checks it and may call
log_wait_commit(), but this problem is caused by transactions which have not started checkpointing.Quoted 8 lines
For example, the tail transaction has an old update for
block_Band the running transaction has a new update forblock_B. Then, the committing transaction fails to write the commit record, it aborts the journal, and newblock_Bwill be written back to the file system without journaling. Because this patch doesn't separate between normal abort and checkpointing related abort, the tail transaction is left in the journal space. So by replaying the tail transaction, newblock_Bis overwritten with old one.
Quoted 6 lines
Yes, and this is expected an correct. When we cannot properly finish a
transaction, we have to discard everything in it. A bug would be (and I think it could currently happen) if we already checkpointed the previous transaction and then written overblock_Bnew data from the uncommitted transaction. I think we have to avoid that - i.e., in case we abort the journal we should not mark buffers dirty when processing the forget loop.
Yes.
Quoted 2 lines
But this is not too serious since fsck has to be run anyway and it will fix the problems.
Yes. The filesystem should be marked with an error, so fsck will check and recover the filesystem on boot. But this means the filesystem loses some latest updates even if it was cleanly unmounted (although some file data has been lost.) I'm a bit afraid that some people would think of this as a regression due to this PATCH 4/5. At least, to avoid undesirable replay, we had better keep journaled data only when the journal has been aborted for checkpointing related reason.
Quoted 10 lines
Quoted 9 lines
It can happen in the case of the checkpointing related abort. For example, assuming the tail transaction has an update for
block_A, the next transaction has an old update forblock_B, and the running transaction has a new update forblock_B.
Now, the running transaction needs more log space, and it callslog_do_checkpoint(). But it aborts the journal because it detected write error onblock_A. In this case, newblock_Bwill be overwritten when the oldblock_Bin the second transaction to the tail is replayed.
Quoted 6 lines
Well, the scenario has to be a bit different (if we need more space than
there is in the journal, we commit the running transaction, do checkpoint and start a new transaction) but something like what you describe could happen. But again I think that this is a correct behavior - i.e., discard all the data in the running transaction when the journal is aborted before the transaction is properly committed.
I think it is correct behavior to discard _all_ metadata updates in the running transaction on abort. But, we don't hope some of metadata updates are often discarded, do we?
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Hi,
Quoted 51 lines · Hidehiro Kawai
Quoted 37 lines · Jan Kara
Quoted 26 lines · Hidehiro Kawai
Quoted 16 lines
Quoted 9 lines
3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Quoted 6 lines
Yes, it would. But how do you think it can happen that a metadata buffer
will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...Checkpointing code checks it and may call
log_wait_commit(), but this problem is caused by transactions which have not started checkpointing.Quoted 8 lines
For example, the tail transaction has an old update for
block_Band the running transaction has a new update forblock_B. Then, the committing transaction fails to write the commit record, it aborts the journal, and newblock_Bwill be written back to the file system without journaling. Because this patch doesn't separate between normal abort and checkpointing related abort, the tail transaction is left in the journal space. So by replaying the tail transaction, newblock_Bis overwritten with old one.Quoted 6 lines
Yes, and this is expected an correct. When we cannot properly finish a
transaction, we have to discard everything in it. A bug would be (and I think it could currently happen) if we already checkpointed the previous transaction and then written overblock_Bnew data from the uncommitted transaction. I think we have to avoid that - i.e., in case we abort the journal we should not mark buffers dirty when processing the forget loop.Yes.
Quoted 2 lines
But this is not too serious since fsck has to be run anyway and it will fix the problems.
Quoted 7 lines
Yes. The filesystem should be marked with an error, so fsck will check and recover the filesystem on boot. But this means the filesystem loses some latest updates even if it was cleanly unmounted (although some file data has been lost.) I'm a bit afraid that some people would think of this as a regression due to this PATCH 4/5. At least, to avoid undesirable replay, we had better keep journaled data only when the journal has been aborted for checkpointing related reason.
I don't think this makes any difference. Look: We have transaction A
modifying block B fully committed to the journal. Now there is a running (or committing, it does not really matter) transaction R also modifying block B. Until R gets fully committed, no block modified by R is checkpointed to the device - checkpointing code takes care of that and it must be so to satisfy journaling guarantees.
So if we abort journal (for whatever reason) before R is fully committed,
no change in R will be seen on the filesystem regardless whether you cleanup the journal or not.
And you cannot do much differenly - the principle of journaling is that
either all changes in the transaction get to disk or none of them. So until the transaction is properly committed, you have the only way to satisfy this condition - take the "none of them" choice.
Now fsck could of course be clever and try to use updates in not fully
committed transaction but personally I don't think it's worth the effort.
Please correct me if I just misunderstood your point...
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Hi,
Quoted 63 lines · Jan Kara
Quoted 53 lines · Hidehiro Kawai
Quoted 37 lines · Jan Kara
Quoted 26 lines · Hidehiro Kawai
Quoted 16 lines
Quoted 9 lines
3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Quoted 6 lines
Yes, it would. But how do you think it can happen that a metadata buffer will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...
Checkpointing code checks it and may call
log_wait_commit(), but this problem is caused by transactions which have not started checkpointing.Quoted 8 lines
For example, the tail transaction has an old update for
block_Band the running transaction has a new update forblock_B. Then, the committing transaction fails to write the commit record, it aborts the journal, and newblock_Bwill be written back to the file system without journaling. Because this patch doesn't separate between normal abort and checkpointing related abort, the tail transaction is left in the journal space. So by replaying the tail transaction, newblock_Bis overwritten with old one.Quoted 6 lines
Yes, and this is expected an correct. When we cannot properly finish a
transaction, we have to discard everything in it. A bug would be (and I think it could currently happen) if we already checkpointed the previous transaction and then written overblock_Bnew data from the uncommitted transaction. I think we have to avoid that - i.e., in case we abort the journal we should not mark buffers dirty when processing the forget loop.Yes.
Quoted 2 lines
But this is not too serious since fsck has to be run anyway and it will fix the problems.
Quoted 7 lines
Yes. The filesystem should be marked with an error, so fsck will check and recover the filesystem on boot. But this means the filesystem loses some latest updates even if it was cleanly unmounted (although some file data has been lost.) I'm a bit afraid that some people would think of this as a regression due to this PATCH 4/5. At least, to avoid undesirable replay, we had better keep journaled data only when the journal has been aborted for checkpointing related reason.
Quoted 9 lines
I don't think this makes any difference. Look: We have transaction A
modifying block B fully committed to the journal. Now there is a running (or committing, it does not really matter) transaction R also modifying block B. Until R gets fully committed, no block modified by R is checkpointed to the device - checkpointing code takes care of that and it must be so to satisfy journaling guarantees.
So if we abort journal (for whatever reason) before R is fully committed,
no change in R will be seen on the filesystem regardless whether you cleanup the journal or not.
No, changes in R will be seen on the filesystem.
The metadata buffer for block B is marked as dirty when it is unfiled whether the journal has aborted or not. Eventually the buffer will be written-back to the filesystem by pdflush. Actually I have confirmed this behavior by using SystemTap. So if both journal abort and system crash happen at the same time, the filesystem would become inconsistent state. As I stated, replaying the journaled block B in transaction A may also corrupt the filesystem, because changes in transaction R are reflected halfway. That is why I sent a patch to prevent metadata buffers from being dirtied on abort.
Quoted 7 lines
And you cannot do much differenly - the principle of journaling is that
either all changes in the transaction get to disk or none of them. So until the transaction is properly committed, you have the only way to satisfy this condition - take the "none of them" choice.
Now fsck could of course be clever and try to use updates in not fully
committed transaction but personally I don't think it's worth the effort.
Please correct me if I just misunderstood your point...
Thanks,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center
Re: [PATCH 4/5] jbd: fix error handling for checkpoint io
Hello,
Quoted 75 lines · Hidehiro Kawai
Quoted 63 lines · Jan Kara
Quoted 53 lines · Hidehiro Kawai
Quoted 37 lines · Jan Kara
Quoted 26 lines · Hidehiro Kawai
Quoted 16 lines
Quoted 9 lines
3. is implemented as described below.
(1) iflog_do_checkpoint()detects an I/O error during checkpointing, it callsjournal_abort()to abort the journal
(2) if the journal has aborted, don't updates_startands_sequencein the on-disk journal superblockSo, if the journal aborts, journaled data will be replayed on the next mount.
Quoted 6 lines
Now, please remember that some dirty metadata buffers are written back to the filesystem without journaling if the journal aborted. We are happy if all dirty metadata buffers are written to the disk, the integrity of the filesystem will be kept. However, replaying the journaled data can overwrite the latest on-disk metadata blocks partly with old data. It would break the filesystem.
Quoted 6 lines
Yes, it would. But how do you think it can happen that a metadata buffer will be written back to the filesystem when it is a part of running transaction? Note that checkpointing code specifically checks whether the buffer being written back is part of a running transaction and if so, it waits for commit before writing back the buffer. So I don't think this can happen but maybe I miss something...
Checkpointing code checks it and may call
log_wait_commit(), but this problem is caused by transactions which have not started checkpointing.Quoted 8 lines
For example, the tail transaction has an old update for
block_Band the running transaction has a new update forblock_B. Then, the committing transaction fails to write the commit record, it aborts the journal, and newblock_Bwill be written back to the file system without journaling. Because this patch doesn't separate between normal abort and checkpointing related abort, the tail transaction is left in the journal space. So by replaying the tail transaction, newblock_Bis overwritten with old one.Quoted 6 lines
Yes, and this is expected an correct. When we cannot properly finish a
transaction, we have to discard everything in it. A bug would be (and I think it could currently happen) if we already checkpointed the previous transaction and then written overblock_Bnew data from the uncommitted transaction. I think we have to avoid that - i.e., in case we abort the journal we should not mark buffers dirty when processing the forget loop.Yes.
Quoted 2 lines
But this is not too serious since fsck has to be run anyway and it will fix the problems.
Quoted 7 lines
Yes. The filesystem should be marked with an error, so fsck will check and recover the filesystem on boot. But this means the filesystem loses some latest updates even if it was cleanly unmounted (although some file data has been lost.) I'm a bit afraid that some people would think of this as a regression due to this PATCH 4/5. At least, to avoid undesirable replay, we had better keep journaled data only when the journal has been aborted for checkpointing related reason.
Quoted 9 lines
I don't think this makes any difference. Look: We have transaction A
modifying block B fully committed to the journal. Now there is a running (or committing, it does not really matter) transaction R also modifying block B. Until R gets fully committed, no block modified by R is checkpointed to the device - checkpointing code takes care of that and it must be so to satisfy journaling guarantees.
So if we abort journal (for whatever reason) before R is fully committed,
no change in R will be seen on the filesystem regardless whether you cleanup the journal or not.
Quoted 10 lines
No, changes in R will be seen on the filesystem.
The metadata buffer for block B is marked as dirty when it is unfiled whether the journal has aborted or not. Eventually the buffer will be written-back to the filesystem by pdflush. Actually I have confirmed this behavior by using SystemTap. So if both journal abort and system crash happen at the same time, the filesystem would become inconsistent state. As I stated, replaying the journaled block B in transaction A may also corrupt the filesystem, because changes in transaction R are reflected halfway. That is why I sent a patch to prevent metadata buffers from being dirtied on abort.
Ah, ok. Thanks for explanation. Yes, we should not mark buffers dirty
when journal is aborted and we unfile them. That should fix the whole problem.
Bye
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH 5/5 ext3: abort ext3 if the journal has aborted
In reply to Hidehiro KawaiSubject: [PATCH 5/5] ext3: abort ext3 if the journal has aborted
If the journal has aborted due to a checkpointing failure, we have to keep the contents of the journal space. ext3_put_super() detects the journal abort, then it invokes ext3_abort() to make the filesystem read only and keep needs_recovery flag.
--- fs/ext3/ioctl.c | 12 ++++++------ fs/ext3/super.c | 11 +++++++++-- 2 files changed, 15 insertions(+), 8 deletions(-)
fs/ext3/ioctl.c +6 -6
@@ -239,7 +239,7 @@ setrsvsz_out:case EXT3_IOC_GROUP_EXTEND : {ext3_fsblk_t n_blocks_count;struct super_block *sb = inode->i_sb; -int err; +int err, err2;if (!capable (CAP_SYS_RESOURCE ))return -EPERM ;
@@ -254,16 +254,16 @@ setrsvsz_out: } err =ext3_group_extend (sb,EXT3_SB (sb)->s_es, n_blocks_count);journal_lock_updates (EXT3_SB (sb)->s_journal); -journal_flush (EXT3_SB (sb)->s_journal); + err2 =journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); group_extend_out:mnt_drop_write (filp->f_path.mnt); -return err; +return (err) ? err : err2; }case EXT3_IOC_GROUP_ADD : {struct ext3_new_group_data input;struct super_block *sb = inode->i_sb; -int err; +int err, err2;if (!capable (CAP_SYS_RESOURCE ))return -EPERM ;
@@ -280,11 +280,11 @@ group_extend_out: err =ext3_group_add (sb, &input);journal_lock_updates (EXT3_SB (sb)->s_journal); -journal_flush (EXT3_SB (sb)->s_journal); + err2 =journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); group_add_out:mnt_drop_write (filp->f_path.mnt); -return err; +return (err) ? err : err2; }
fs/ext3/super.c +9 -2
@@ -393,7 +393,8 @@ static void ext3_put_super (struct superint i;ext3_xattr_put_super (sb); -journal_destroy (sbi->s_journal); +if (journal_destroy (sbi->s_journal) <0 ) +ext3_abort (sb, __func__,"Couldn't clean up the journal" );if (!(sb->s_flags &MS_RDONLY )) {EXT3_CLEAR_INCOMPAT_FEATURE (sb,EXT3_FEATURE_INCOMPAT_RECOVER ); es->s_state =cpu_to_le16 (sbi->s_mount_state);
@@ -2373,7 +2374,13 @@ static void ext3_write_super_lockfs(stru/* Now we set up the journal barrier. */ journal_lock_updates (journal); -journal_flush (journal); + +/* +* We don't want to clear needs_recovery flag when we failed +* to flush the journal. +*/ +if (journal_flush (journal) <0 ) +return ;/* Journal blocked and flushed, clear needs_recovery flag. */ EXT3_CLEAR_INCOMPAT_FEATURE (sb,EXT3_FEATURE_INCOMPAT_RECOVER );
Re: [PATCH 5/5] ext3: abort ext3 if the journal has aborted
Quoted 8 lines · Hidehiro Kawai
Subject: [PATCH 5/5] ext3: abort ext3 if the journal has aborted
If the journal has aborted due to a checkpointing failure, we have to keep the contents of the journal space.
ext3_put_super()detects the journal abort, then it invokesext3_abort()to make the filesystem read only and keepneeds_recoveryflag.Signed-off-by Hidehiro Kawai <[email protected]>
...
Quoted 5 lines
fs/ext3/super.c +0
-1@@ -2373,7 +2374,13 @@ static void ext3_write_super_lockfs(stru/* Now we set up the journal barrier. */ journal_lock_updates (journal); -journal_flush (journal);
fs/ext3/super.c static void ext3_write_super_lockfs(stru · line 2386
Quoted 10 lines
fs/ext3/super.c +7
-0+ +/* +* We don't want to clear needs_recovery flag when we failed +* to flush the journal. +*/ +if (journal_flush (journal) <0 ) +return ;/* Journal blocked and flushed, clear needs_recovery flag. */ EXT3_CLEAR_INCOMPAT_FEATURE (sb,EXT3_FEATURE_INCOMPAT_RECOVER );
It's a bit scary that LVM will start creating filesystem snapshot whenjournal_flush() failed but since there's no way to propagate error further up, I guess we cannot do anything better.
You can add:
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 1 line · Hidehiro Kawai
Subject: [PATCH 0/5] jbd: possible filesystem corruption fixes (take 2)
Just a small note - repeating the subject in the changelog makes it a bit
harder for Andrew to merge the patches. Standard merging scripts prepend subject of the email to the changelog below and so in your case the message would be there twice... Just that you know when you create the patch series next time.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Hello,
Quoted 9 lines · Jan Kara
Quoted 1 line · Hidehiro Kawai
Subject: [PATCH 0/5] jbd: possible filesystem corruption fixes (take 2)
Just a small note - repeating the subject in the changelog makes it a bit
harder for Andrew to merge the patches. Standard merging scripts prepend subject of the email to the changelog below and so in your case the message would be there twice... Just that you know when you create the patch series next time.
Thank you for letting me know. I'll not do such odd thing next time.
Regards,
Signature · 3 lines
Hidehiro Kawai Hitachi, Systems Development Laboratory Linux Technology Center