[PATCH] ext3: ext3_commit_super should always mark super uptodate

[PATCH] ext3: ext3_commit_super should always mark super uptodate

[Jeff Mahoney]

-----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.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
- ---
 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-----

Re: [PATCH] ext3: ext3_commit_super should always mark super uptodate

[Phillip Susi]

Jeff Mahoney wrote:
>  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.

Not if it has been modified and not yet written back to disk.  Then it 
is dirty by definition.

>  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.

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.

> +	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?

Re: [PATCH] ext3: ext3_commit_super should always mark super uptodate

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Phillip Susi wrote:
> Jeff Mahoney wrote:
>>  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.
> 
> 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.

>>  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.
> 
> 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.

>> +    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?

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-----