Hello, On Wed 04-01-12 15:28:14, Edward Shishkin wrote: > 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 for the patch Edward but I have to say I don't quite like it. Wouldn't it be cleaner to not clear the uptodate flag in the first place? My idea would be that filesystems that properly check for buffer_write_io_error (instead of buffer_uptodate) set a flag in file_system_type structure and for these filesystems we don't clear the uptodate flag. I believe that would be a move in a better direction. I'll post in a minute three patches that do this for generic code and ext2. Honza > 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 > > -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- 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
