Hello. There is a number of complaints: people yunk out usb cables of their external hard drives and see kernel warnings, since BH_Uptodate flag is cleared up because of io error: http://bugzilla.redhat.com/show_bug.cgi?id=679930 There was a related commit 7bf0dc9b0ca9e9b6524b1f70e0898c7f11eb10be however it fixes nothing: after ext2_clear_super_error() the buffer can successfully become not-uptodate again, so that the warnings are easily observed in the latest upstream kernels. Also the problem takes place not only for superblock's buffers: any buffer can become not-uptodate because of io errors. Please consider more substantial fixup below. I performed regression testing for the patched stuff: mongo benchmark doesn't show any performance drop: http://bugzilla.redhat.com/attachment.cgi?id=550660 Thanks, Edward. ext2: avoid WARN() messages when failing to write to buffers ( more substantial fixup than 7bf0dc9b0ca9e9b6524b1f70e0898c7f11eb10be ) Signed-off-by: Edward Shishkin <edward@xxxxxxxxxx> --- fs/ext2/balloc.c | 12 +++++++++++- fs/ext2/ext2.h | 2 ++ fs/ext2/ialloc.c | 17 +++++++++++++++++ fs/ext2/inode.c | 25 +++++++++++++++++++++---- fs/ext2/super.c | 23 +++++++++++++---------- fs/ext2/xattr.c | 10 +++++++++- 6 files changed, 73 insertions(+), 16 deletions(-) Index: linux-3.2-rc6/fs/ext2/balloc.c =================================================================== --- linux-3.2-rc6.orig/fs/ext2/balloc.c +++ linux-3.2-rc6/fs/ext2/balloc.c @@ -181,7 +181,10 @@ static void group_adjust_blocks(struct s desc->bg_free_blocks_count = cpu_to_le16(free_blocks + count); spin_unlock(sb_bgl_lock(sbi, group_no)); sb->s_dirt = 1; + lock_buffer(bh); + ext2_clear_buffer_error(sb, bh, "updating group descriptor"); mark_buffer_dirty(bh); + unlock_buffer(bh); } } @@ -555,8 +558,11 @@ do_more: group_freed++; } } - + lock_buffer(bitmap_bh); + ext2_clear_buffer_error(sb, bitmap_bh, "updating bitmap"); mark_buffer_dirty(bitmap_bh); + unlock_buffer(bitmap_bh); + if (sb->s_flags & MS_SYNCHRONOUS) sync_dirty_buffer(bitmap_bh); @@ -1411,7 +1417,11 @@ allocated: group_adjust_blocks(sb, group_no, gdp, gdp_bh, -num); percpu_counter_sub(&sbi->s_freeblocks_counter, num); + lock_buffer(bitmap_bh); + ext2_clear_buffer_error(sb, bitmap_bh, "updating bitmap"); mark_buffer_dirty(bitmap_bh); + unlock_buffer(bitmap_bh); + if (sb->s_flags & MS_SYNCHRONOUS) sync_dirty_buffer(bitmap_bh); Index: linux-3.2-rc6/fs/ext2/ialloc.c =================================================================== --- linux-3.2-rc6.orig/fs/ext2/ialloc.c +++ linux-3.2-rc6/fs/ext2/ialloc.c @@ -82,7 +82,10 @@ static void ext2_release_inode(struct su if (dir) percpu_counter_dec(&EXT2_SB(sb)->s_dirs_counter); sb->s_dirt = 1; + lock_buffer(bh); + ext2_clear_buffer_error(sb, bh, "updating group descriptor"); mark_buffer_dirty(bh); + unlock_buffer(bh); } /* @@ -145,7 +148,12 @@ void ext2_free_inode (struct inode * ino "bit already cleared for inode %lu", ino); else ext2_release_inode(sb, block_group, is_directory); + + lock_buffer(bitmap_bh); + ext2_clear_buffer_error(sb, bitmap_bh, "updating bitmap"); mark_buffer_dirty(bitmap_bh); + unlock_buffer(bitmap_bh); + if (sb->s_flags & MS_SYNCHRONOUS) sync_dirty_buffer(bitmap_bh); @@ -512,7 +520,11 @@ repeat_in_this_group: err = -ENOSPC; goto fail; got: + lock_buffer(bitmap_bh); + ext2_clear_buffer_error(sb, bitmap_bh, "updating bitmap"); mark_buffer_dirty(bitmap_bh); + unlock_buffer(bitmap_bh); + if (sb->s_flags & MS_SYNCHRONOUS) sync_dirty_buffer(bitmap_bh); brelse(bitmap_bh); @@ -544,7 +556,12 @@ got: spin_unlock(sb_bgl_lock(sbi, group)); sb->s_dirt = 1; + + lock_buffer(bh2); + ext2_clear_buffer_error(sb, bh2, "updating group descriptor"); mark_buffer_dirty(bh2); + unlock_buffer(bh2); + if (test_opt(sb, GRPID)) { inode->i_mode = mode; inode->i_uid = current_fsuid(); Index: linux-3.2-rc6/fs/ext2/inode.c =================================================================== --- linux-3.2-rc6.orig/fs/ext2/inode.c +++ linux-3.2-rc6/fs/ext2/inode.c @@ -514,8 +514,8 @@ static int ext2_alloc_branch(struct inod *(branch[n].p + i) = cpu_to_le32(++current_block); } set_buffer_uptodate(bh); - unlock_buffer(bh); mark_buffer_dirty_inode(bh, inode); + unlock_buffer(bh); /* We used to sync bh here if IS_SYNC(inode). * But we now rely upon generic_write_sync() * and b_inode_buffers. But not for directories. @@ -577,9 +577,13 @@ static void ext2_splice_branch(struct in /* We are done with atomic stuff, now do the rest of housekeeping */ /* had we spliced it onto indirect block? */ - if (where->bh) + if (where->bh) { + lock_buffer(where->bh); + ext2_clear_buffer_error(inode->i_sb, where->bh, + "updating indirect block"); mark_buffer_dirty_inode(where->bh, inode); - + unlock_buffer(where->bh); + } inode->i_ctime = CURRENT_TIME_SEC; mark_inode_dirty(inode); } @@ -1105,8 +1109,13 @@ static void __ext2_truncate_blocks(struc if (nr) { if (partial == chain) mark_inode_dirty(inode); - else + else { + lock_buffer(partial->bh); + ext2_clear_buffer_error(inode->i_sb, partial->bh, + "updating indirect block"); mark_buffer_dirty_inode(partial->bh, inode); + unlock_buffer(partial->bh); + } ext2_free_branches(inode, &nr, &nr+1, (chain+n-1) - partial); } /* Clear the ends of indirect blocks on the shared branch */ @@ -1115,7 +1124,12 @@ static void __ext2_truncate_blocks(struc partial->p + 1, (__le32*)partial->bh->b_data+addr_per_block, (chain+n-1) - partial); + lock_buffer(partial->bh); + ext2_clear_buffer_error(inode->i_sb, partial->bh, + "updating indirect block"); mark_buffer_dirty_inode(partial->bh, inode); + unlock_buffer(partial->bh); + brelse (partial->bh); partial--; } @@ -1504,7 +1518,10 @@ static int __ext2_write_inode(struct ino } } else for (n = 0; n < EXT2_N_BLOCKS; n++) raw_inode->i_block[n] = ei->i_data[n]; + lock_buffer(bh); + ext2_clear_buffer_error(sb, bh, "writing inode"); mark_buffer_dirty(bh); + unlock_buffer(bh); if (do_sync) { sync_dirty_buffer(bh); if (buffer_req(bh) && !buffer_uptodate(bh)) { Index: linux-3.2-rc6/fs/ext2/super.c =================================================================== --- linux-3.2-rc6.orig/fs/ext2/super.c +++ linux-3.2-rc6/fs/ext2/super.c @@ -1129,37 +1129,40 @@ failed_unlock: return ret; } -static void ext2_clear_super_error(struct super_block *sb) +void ext2_clear_buffer_error(struct super_block *sb, struct buffer_head *bh, + const char *when) { - struct buffer_head *sbh = EXT2_SB(sb)->s_sbh; - - if (buffer_write_io_error(sbh)) { + if (buffer_write_io_error(bh)) { /* - * Oh, dear. A previous attempt to write the - * superblock failed. This could happen because the + * Oh, dear. A previous attempt to write the buffer + * failed. This could happen because the * USB device was yanked out. Or it could happen to * be a transient write error and maybe the block will * be remapped. Nothing we can do but to retry the * write and hope for the best. */ ext2_msg(sb, KERN_ERR, - "previous I/O error to superblock detected\n"); - clear_buffer_write_io_error(sbh); - set_buffer_uptodate(sbh); + "previous I/O error to buffer detected when %s\n", + when); + clear_buffer_write_io_error(bh); + set_buffer_uptodate(bh); } } static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es, int wait) { - ext2_clear_super_error(sb); spin_lock(&EXT2_SB(sb)->s_lock); 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()); /* unlock before we do IO */ spin_unlock(&EXT2_SB(sb)->s_lock); + + lock_buffer(EXT2_SB(sb)->s_sbh); + ext2_clear_buffer_error(sb, EXT2_SB(sb)->s_sbh, "writing superblock"); mark_buffer_dirty(EXT2_SB(sb)->s_sbh); + unlock_buffer(EXT2_SB(sb)->s_sbh); if (wait) sync_dirty_buffer(EXT2_SB(sb)->s_sbh); sb->s_dirt = 0; Index: linux-3.2-rc6/fs/ext2/xattr.c =================================================================== --- linux-3.2-rc6.orig/fs/ext2/xattr.c +++ linux-3.2-rc6/fs/ext2/xattr.c @@ -341,7 +341,10 @@ static void ext2_xattr_update_super_bloc EXT2_SET_COMPAT_FEATURE(sb, EXT2_FEATURE_COMPAT_EXT_ATTR); spin_unlock(&EXT2_SB(sb)->s_lock); sb->s_dirt = 1; + lock_buffer(EXT2_SB(sb)->s_sbh); + ext2_clear_buffer_error(sb, EXT2_SB(sb)->s_sbh, "updating super"); mark_buffer_dirty(EXT2_SB(sb)->s_sbh); + unlock_buffer(EXT2_SB(sb)->s_sbh); } /* @@ -678,7 +681,10 @@ ext2_xattr_set2(struct inode *inode, str ext2_xattr_update_super_block(sb); } + lock_buffer(new_bh); + ext2_clear_buffer_error(sb, new_bh, "setting xattrs"); mark_buffer_dirty(new_bh); + unlock_buffer(new_bh); if (IS_SYNC(inode)) { sync_dirty_buffer(new_bh); error = -EIO; @@ -734,6 +740,7 @@ ext2_xattr_set2(struct inode *inode, str mb_cache_entry_release(ce); dquot_free_block_nodirty(inode, 1); mark_inode_dirty(inode); + ext2_clear_buffer_error(sb, old_bh, "setting xattrs"); mark_buffer_dirty(old_bh); ea_bdebug(old_bh, "refcount now=%d", le32_to_cpu(HDR(old_bh)->h_refcount)); @@ -792,8 +799,9 @@ ext2_xattr_delete_inode(struct inode *in mb_cache_entry_release(ce); ea_bdebug(bh, "refcount now=%d", le32_to_cpu(HDR(bh)->h_refcount)); - unlock_buffer(bh); + ext2_clear_buffer_error(inode->i_sb, bh, "deleting xattr"); mark_buffer_dirty(bh); + unlock_buffer(bh); if (IS_SYNC(inode)) sync_dirty_buffer(bh); dquot_free_block_nodirty(inode, 1); Index: linux-3.2-rc6/fs/ext2/ext2.h =================================================================== --- linux-3.2-rc6.orig/fs/ext2/ext2.h +++ linux-3.2-rc6/fs/ext2/ext2.h @@ -141,6 +141,8 @@ extern __printf(3, 4) void ext2_msg(struct super_block *, const char *, const char *, ...); extern void ext2_update_dynamic_rev (struct super_block *sb); extern void ext2_write_super (struct super_block *); +void ext2_clear_buffer_error(struct super_block *sb, struct buffer_head *bh, + const char *when); /* * Inodes and files operations -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
