Silence warnings about non-uptodate buffers

PATCH In review Jan Kara 2 participants 9 messages May 28, 2008 – May 29, 2008

Subsystems (4)

ext4 file system ext2 file system Journalling layer for block devices (JBD2) UDF filesystem

lore

Share & tools

lore

Jan Kara

author reviewer

2008-05-28 21:56 UTC #0 lore raw

PATCH 1/1 Silence warnings about non-uptodate buffers

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

Jan Kara

author reviewer

2008-05-28 21:56 UTC #1 lore raw

PATCH 1/1 ext3: 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 <[email protected]>

---
 fs/ext3/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/ext3/super.c +2 -0

index fe3119a..19d2fb4 100644
@@ -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);

Signature · 1 line

1.5.2.4

Jan Kara

author reviewer

2008-05-28 21:56 UTC #2 lore raw

PATCH 1/1 ext2: Silence warnings about non-uptodate buffers

In reply to Jan Kara

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext2/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/ext2/super.c +2 -0

index ef50cbc..2941bc3 100644
@@ -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;

Signature · 1 line

1.5.2.4

Jan Kara

author reviewer

2008-05-28 21:56 UTC #3 lore raw

PATCH 1/1 jbd2: Silence warnings about non-uptodate buffers

In reply to Jan Kara

Signed-off-by Jan Kara <[email protected]>

---
 fs/jbd2/journal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/jbd2/journal.c +1 -0

index 2e24567..55de8f7 100644
@@ -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);

Signature · 1 line

1.5.2.4

Andrew Morton

2008-05-29 03:59 UTC #4 lore raw

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

Quoted 15 lines · On Wed, 28 May 2008 23:56:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
---
 fs/jbd2/journal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
fs/jbd2/journal.c +0 -0
index 2e24567..55de8f7 100644

fs/jbd2/journal.c void jbd2_journal_update_superblock(journal_t *journal, int wait) · line 1267

Quoted 7 lines

fs/jbd2/journal.c +1 -0

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

Jan Kara

author reviewer

2008-05-29 13:09 UTC #5 lore raw

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

Quoted 27 lines · Andrew Morton

Quoted 15 lines · On Wed, 28 May 2008 23:56:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
---
 fs/jbd2/journal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
fs/jbd2/journal.c +0 -0
index 2e24567..55de8f7 100644
fs/jbd2/journal.c void jbd2_journal_update_superblock(journal_t *journal, int wait) · line 1267
Quoted 7 lines
fs/jbd2/journal.c +1 -0
@@ -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....

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 11 lines

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

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Jan Kara

author reviewer

2008-05-28 21:56 UTC #6 lore raw

PATCH 1/1 jbd: Silence warnings about non-uptodate buffers

In reply to Jan Kara

Signed-off-by Jan Kara <[email protected]>

---
 fs/jbd/journal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/jbd/journal.c +1 -0

index b99c3b3..9303608 100644
@@ -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);

Signature · 1 line

1.5.2.4

Jan Kara

author reviewer

2008-05-28 21:56 UTC #7 lore raw

PATCH 1/1 ext4: Silence warnings about non-uptodate buffers

In reply to Jan Kara

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/ext4/super.c +2 -0

index 09d9359..298c94f 100644
@@ -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);

Signature · 1 line

1.5.2.4

Jan Kara

author reviewer

2008-05-28 22:07 UTC #8 lore raw

In reply to Jan Kara

Quoted 6 lines · 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?

And just for reference a similar patch for UDF which I'll merge through
my git tree.

Honza

Signature · 3 lines

Jan Kara <[email protected]>
SUSE Labs, CR
---

Quoted 1 line

From 756bc361a065b65df9977c2632472f587659a6f7 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Wed, 28 May 2008 23:39:19 +0200
Subject: [PATCH] udf: Silence warnings about non-uptodate buffers

Signed-off-by Jan Kara <[email protected]>

---
 fs/udf/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/udf/super.c +2 -0

index 7a5f69b..dd295b8 100644
@@ -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);
 }

Signature · 1 line

1.5.2.4