ext3: ext3_commit_super should always mark super uptodate
PATCH 1/1 ext3: ext3_commit_super should always mark super uptodate
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
After a superblock write failure, the buffer_head is marked !uptodate. Since the superblock is something of an exception -- read once and a reference kept for the duration of the mount -- it is by definition always uptodate.
This is somewhat academic for the most part until we encounter error conditions. For example, if a disk goes away in a SAN environment, the write failure will occur and it will be followed by others. ext3_error wants to mark the superblock dirty via ext3_commit_super, but once the first write fails, the subsequent mark_buffer_dirty calls will issue warnings because the buffer is not uptodate.
This patch silences those warnings by marking it uptodate in ext3_commit_super before calling mark_buffer_dirty. This doesn't really change anything other than silencing the warning in mark_buffer_dirty. If the write succeeds, good. Otherwise, it will just have uptodate cleared again.
- ---
fs/ext3/super.c | 7 +++++++ 1 file changed, 7 insertions(+)
- --- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2291,6 +2291,13 @@ static void ext3_commit_super (struct su
es->s_free_blocks_count = cpu_to_le32(ext3_count_free_blocks(sb)); es->s_free_inodes_count = cpu_to_le32(ext3_count_free_inodes(sb)); BUFFER_TRACE(sbh, "marking dirty");
+
+ /* We only read the superblock once. The in-memory version is
+ * always the most recent. If ext3_error is called after a
+ * superblock write failure, it will be !uptodate. This write
+ * will likely fail also, but it avoids the WARN_ON in
+ * mark_buffer_dirty. */
+ set_buffer_uptodate(sbh);
mark_buffer_dirty(sbh);
if (sync)
sync_dirty_buffer(sbh);
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org
iEYEARECAAYFAklH784ACgkQLPWxlyuTD7LsHgCeJjBBmkTeB4ZAxPL0TY7qhRO7 6c8Anj/MbtPcAXBqCdYMFCqcj8Rjl1gI
=2LYo
-----END PGP SIGNATURE-----
Quoted 4 lines · Jeff Mahoney
After a superblock write failure, the
buffer_headis marked !uptodate. Since the superblock is something of an exception -- read once and a reference kept for the duration of the mount -- it is by definition always uptodate.
Not if it has been modified and not yet written back to disk. Then it is dirty by definition.
Quoted 6 lines
This is somewhat academic for the most part until we encounter error conditions. For example, if a disk goes away in a SAN environment, the write failure will occur and it will be followed by others.
ext3_errorwants to mark the superblock dirty viaext3_commit_super, but once the first write fails, the subsequentmark_buffer_dirtycalls will issue warnings because the buffer is not uptodate.
Why on earth is a warning issued for marking an already dirty buffer dirty again? Surly buffers can be modified again and thus, marked as dirty a second time before pdflush gets around to syncing them all the time.
Review
Quoted 4 lines
(quoted diff) +1
-0+ set_buffer_uptodate(sbh); mark_buffer_dirty(sbh); if (sync) sync_dirty_buffer(sbh);
Are you sure this only matters when there is an IO error? It looks there like if !sync then the sb can continue to sit in memory marked as dirty for some time before someone tries to flush it to disk. Eventually can't we end up back through this code patch and calling mark_buffer_dirty again before it was ever flushed in the first place, even without an IO error?
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 8 lines · Phillip Susi
Quoted 4 lines · Jeff Mahoney
After a superblock write failure, the
buffer_headis marked !uptodate. Since the superblock is something of an exception -- read once and a reference kept for the duration of the mount -- it is by definition always uptodate.Not if it has been modified and not yet written back to disk. Then it is dirty by definition.
I think you're missing the distinction between uptodate and dirty here.
Quoted 11 lines
Quoted 6 lines
This is somewhat academic for the most part until we encounter error conditions. For example, if a disk goes away in a SAN environment, the write failure will occur and it will be followed by others.
ext3_errorwants to mark the superblock dirty viaext3_commit_super, but once the first write fails, the subsequentmark_buffer_dirtycalls will issue warnings because the buffer is not uptodate.Why on earth is a warning issued for marking an already dirty buffer dirty again? Surly buffers can be modified again and thus, marked as dirty a second time before pdflush gets around to syncing them all the time.
No, it issues a warning when you mark a !uptodate buffer dirty. The idea behind it is that the buffer contents may be stale with respect to disk contents. In the case of the superblock, the in-memory contents are always the most recent and it *never* re-reads it from disk.
Quoted 11 lines
Quoted 4 lines
+ set_buffer_uptodate(sbh); mark_buffer_dirty(sbh); if (sync) sync_dirty_buffer(sbh);Are you sure this only matters when there is an IO error? It looks there like if !sync then the sb can continue to sit in memory marked as dirty for some time before someone tries to flush it to disk. Eventually can't we end up back through this code patch and calling
mark_buffer_dirtyagain before it was ever flushed in the first place, even without an IO error?
Again, uptodate != dirty.
- -Jeff
- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org
iEYEARECAAYFAklLweIACgkQLPWxlyuTD7KbKACfTn4daFFerN0X/V5rpt+eSK6C p7MAnjDOIZAF2EUb6gWqWk+Mw4/6ECPl
=GPkb
-----END PGP SIGNATURE-----