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
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/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
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/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
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(-)
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
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(-)
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
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/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
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(-)
>
> 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.
On Wed 28-05-08 20:59:13, Andrew Morton wrote:
> 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(-)
> >
> > 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 <[email protected]>
SUSE Labs, CR