Re: [patch] ext2: avoid WARN() messages when failing to write to buffers

[Jan Kara <jack@suse.cz>]

  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@redhat.com>
> ---
>  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@suse.cz>
SUSE Labs, CR