[PATCH] Silence warnings about non-uptodate buffers

[PATCH] Silence warnings about non-uptodate buffers

[Jan Kara]

Hello,

the series of patches below gets rid of warning messages in mark_buffer_dirty()
when underlying block device becomes unavailable. Andrew, would you merge them
please?

								Honza

[PATCH] ext2: Silence warnings about non-uptodate buffers

[Jan Kara]

When underlying block device becomes unavailable (e.g. someone pulling an
USB stick from under us), kernel produces warning about non-uptodate buffer
(superblock) being marked dirty. Silence these warnings by making buffer
uptodate before marking it dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index ef50cbc..2941bc3 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1092,6 +1092,7 @@ static void ext2_commit_super (struct super_block * sb,
 			       struct ext2_super_block * es)
 {
 	es->s_wtime = cpu_to_le32(get_seconds());
+	set_buffer_uptodate(EXT2_SB(sb)->s_sbh);
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
 	sb->s_dirt = 0;
 }
@@ -1101,6 +1102,7 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
 	es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb));
 	es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb));
 	es->s_wtime = cpu_to_le32(get_seconds());
+	set_buffer_uptodate(EXT2_SB(sb)->s_sbh);
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
 	sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
 	sb->s_dirt = 0;
-- 
1.5.2.4

[PATCH] ext3: Silence warnings about non-uptodate buffers

[Jan Kara]

When underlying block device becomes unavailable (e.g. someone pulling an
USB stick from under us), kernel produces warning about non-uptodate buffer
(superblock) being marked dirty. Silence these warnings by making buffer
uptodate before marking it dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index fe3119a..19d2fb4 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -398,6 +398,7 @@ static void ext3_put_super (struct super_block * sb)
 		EXT3_CLEAR_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
 		BUFFER_TRACE(sbi->s_sbh, "marking dirty");
+		set_buffer_uptodate(sbi->s_sbh);
 		mark_buffer_dirty(sbi->s_sbh);
 		ext3_commit_super(sb, es, 1);
 	}
@@ -2248,6 +2249,7 @@ static void ext3_commit_super (struct super_block * sb,
 	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");
+	set_buffer_uptodate(sbh);
 	mark_buffer_dirty(sbh);
 	if (sync)
 		sync_dirty_buffer(sbh);
-- 
1.5.2.4

[PATCH] jbd: Silence warnings about non-uptodate buffers

[Jan Kara]

When underlying block device becomes unavailable (e.g. someone pulling an
USB stick from under us), kernel produces warning about non-uptodate buffer
(superblock) being marked dirty. Silence these warnings by making buffer
uptodate before marking it dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd/journal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
index b99c3b3..9303608 100644
--- a/fs/jbd/journal.c
+++ b/fs/jbd/journal.c
@@ -959,6 +959,7 @@ void journal_update_superblock(journal_t *journal, int wait)
 	spin_unlock(&journal->j_state_lock);
 
 	BUFFER_TRACE(bh, "marking dirty");
+	set_buffer_uptodate(bh);
 	mark_buffer_dirty(bh);
 	if (wait)
 		sync_dirty_buffer(bh);
-- 
1.5.2.4

[PATCH] jbd2: Silence warnings about non-uptodate buffers

[Jan Kara]

When underlying block device becomes unavailable (e.g. someone pulling an
USB stick from under us), kernel produces warning about non-uptodate buffer
(superblock) being marked dirty. Silence these warnings by making buffer
uptodate before marking it dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 2e24567..55de8f7 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1261,6 +1261,7 @@ void jbd2_journal_update_superblock(journal_t *journal, int wait)
 	spin_unlock(&journal->j_state_lock);
 
 	BUFFER_TRACE(bh, "marking dirty");
+	set_buffer_uptodate(bh);
 	mark_buffer_dirty(bh);
 	if (wait)
 		sync_dirty_buffer(bh);
-- 
1.5.2.4

[PATCH] ext4: Silence warnings about non-uptodate buffers

[Jan Kara]

When underlying block device becomes unavailable (e.g. someone pulling an
USB stick from under us), kernel produces warning about non-uptodate buffer
(superblock) being marked dirty. Silence these warnings by making buffer
uptodate before marking it dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 09d9359..298c94f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -510,6 +510,7 @@ static void ext4_put_super (struct super_block * sb)
 		EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
 		BUFFER_TRACE(sbi->s_sbh, "marking dirty");
+		set_buffer_uptodate(sbi->s_sbh);
 		mark_buffer_dirty(sbi->s_sbh);
 		ext4_commit_super(sb, es, 1);
 	}
@@ -2659,6 +2660,7 @@ static void ext4_commit_super (struct super_block * sb,
 	ext4_free_blocks_count_set(es, ext4_count_free_blocks(sb));
 	es->s_free_inodes_count = cpu_to_le32(ext4_count_free_inodes(sb));
 	BUFFER_TRACE(sbh, "marking dirty");
+	set_buffer_uptodate(sbh);
 	mark_buffer_dirty(sbh);
 	if (sync)
 		sync_dirty_buffer(sbh);
-- 
1.5.2.4

Re: [PATCH] Silence warnings about non-uptodate buffers

[Jan Kara]

On Wed 28-05-08 23:56:08, Jan Kara wrote:
> Hello,
> 
> the series of patches below gets rid of warning messages in mark_buffer_dirty()
> when underlying block device becomes unavailable. Andrew, would you merge them
> please?
> 
  And just for reference a similar patch for UDF which I'll merge through
my git tree.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
---

>From 756bc361a065b65df9977c2632472f587659a6f7 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 28 May 2008 23:39:19 +0200
Subject: [PATCH] udf: Silence warnings about non-uptodate buffers

When underlying block device becomes unavailable (e.g. someone pulling an
USB stick from under us), kernel produces warning about non-uptodate buffer
(superblock) being marked dirty. Silence these warnings by making buffer
uptodate before marking it dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 7a5f69b..dd295b8 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1770,6 +1770,7 @@ static void udf_open_lvid(struct super_block *sb)
 			le16_to_cpu(lvid->descTag.descCRCLength)));
 
 	lvid->descTag.tagChecksum = udf_tag_checksum(&lvid->descTag);
+	set_buffer_uptodate(bh);
 	mark_buffer_dirty(bh);
 }
 
@@ -1805,6 +1806,7 @@ static void udf_close_lvid(struct super_block *sb)
 				le16_to_cpu(lvid->descTag.descCRCLength)));
 
 	lvid->descTag.tagChecksum = udf_tag_checksum(&lvid->descTag);
+	set_buffer_uptodate(bh);
 	mark_buffer_dirty(bh);
 }
 
-- 
1.5.2.4

Re: [PATCH] jbd2: Silence warnings about non-uptodate buffers

[Andrew Morton]

On Wed, 28 May 2008 23:56:12 +0200 Jan Kara <jack@suse.cz> wrote:

> When underlying block device becomes unavailable (e.g. someone pulling an
> USB stick from under us), kernel produces warning about non-uptodate buffer
> (superblock) being marked dirty. Silence these warnings by making buffer
> uptodate before marking it dirty.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/jbd2/journal.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 2e24567..55de8f7 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1261,6 +1261,7 @@ void jbd2_journal_update_superblock(journal_t *journal, int wait)
>  	spin_unlock(&journal->j_state_lock);
>  
>  	BUFFER_TRACE(bh, "marking dirty");
> +	set_buffer_uptodate(bh);
>  	mark_buffer_dirty(bh);
>  	if (wait)
>  		sync_dirty_buffer(bh);

I have issues....

- Are we really really sure that we aren't about to wreck people's
  filesystems when this happens?  I mean, a non-uptodate buffer might
  have random garbage in it, and it would be sad to write that to disk.

  Either way, I do think that potentially falsely setting BH_Uptodate
  just to squish a WARN_ON_ONCE() is not a good solution.  Better to
  set a new BH_Nowarn, or to call a new mark_buffer_dirty_nowarn() here.

- Did the reads of these buffers encounter an IO error?  If so,
  perhaps we could set a new BH_GotIOError or something.

Even if I'm completely wrong about everything as usual, I do think that
the code change should at least include a comment explaining why the
filesystem is doing set_buffer_uptodate() in such a weird place.

One nice way of adding that comment would be to implement a new

/*
 * comment goes here
 */
set_buffer_uptodate_for_mark_buffer_dirty(struct buffer_head *bh); /* needs better name */

and call that.

But I agree with me: this looks like abuse of buffer_uptodate(), and a
mark_buffer_dirty_nowarn() would be a cleaner solution.

Re: [PATCH] jbd2: Silence warnings about non-uptodate buffers

[Jan Kara]

On Wed 28-05-08 20:59:13, Andrew Morton wrote:
> On Wed, 28 May 2008 23:56:12 +0200 Jan Kara <jack@suse.cz> wrote:
> 
> > When underlying block device becomes unavailable (e.g. someone pulling an
> > USB stick from under us), kernel produces warning about non-uptodate buffer
> > (superblock) being marked dirty. Silence these warnings by making buffer
> > uptodate before marking it dirty.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/jbd2/journal.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 2e24567..55de8f7 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -1261,6 +1261,7 @@ void jbd2_journal_update_superblock(journal_t *journal, int wait)
> >  	spin_unlock(&journal->j_state_lock);
> >  
> >  	BUFFER_TRACE(bh, "marking dirty");
> > +	set_buffer_uptodate(bh);
> >  	mark_buffer_dirty(bh);
> >  	if (wait)
> >  		sync_dirty_buffer(bh);
> 
> I have issues....
> 
> - Are we really really sure that we aren't about to wreck people's
>   filesystems when this happens?  I mean, a non-uptodate buffer might
>   have random garbage in it, and it would be sad to write that to disk.
  Well, as far as I've looked into the code, we already write the buffer
anyway. So far we just warned that we are writing non-uptodate buffer. So
in this regard, I don't think there's any difference with/without my patch.

>   Either way, I do think that potentially falsely setting BH_Uptodate
>   just to squish a WARN_ON_ONCE() is not a good solution.  Better to
>   set a new BH_Nowarn, or to call a new mark_buffer_dirty_nowarn() here.
> 
> - Did the reads of these buffers encounter an IO error?  If so,
>   perhaps we could set a new BH_GotIOError or something.
  I've changed only the "superblock buffers". Those have the property that
we read that just once during mount (and if this fails, we won't try to
write them anyway), keep pointer to them and occasionally try to write
them. Also we shouldn't really have garbage in them - they can be !uptodate
only because underlying device was not able to write them... For most other
buffers, when we fail to read them, we just bail out and don't try marking
them dirty, so we don't issue warning for them.

> Even if I'm completely wrong about everything as usual, I do think that
> the code change should at least include a comment explaining why the
> filesystem is doing set_buffer_uptodate() in such a weird place.
  Agreed, I'll add some comment to the code.

> One nice way of adding that comment would be to implement a new
> 
> /*
>  * comment goes here
>  */
> set_buffer_uptodate_for_mark_buffer_dirty(struct buffer_head *bh); /* needs better name */
> 
> and call that.
> 
> But I agree with me: this looks like abuse of buffer_uptodate(), and a
> mark_buffer_dirty_nowarn() would be a cleaner solution.
  OK. I've thought about one more option - if buffer isn't uptodate, just
avoid marking it dirty and writing it. That has the advantage that if the
spotted error is real bad block, then we won't retry writing it which
usually takes non-trivial time. But at the same time it is a disadvantage
because in case there's a transient error writing super-block, we'd never
retry writing it. So I decided to keep the current behavior...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR