Re: [PATCH 1/5] jbd: strictly check for write errors on data buffers

[Jan Kara <jack@suse.cz>]

On Mon 09-06-08 19:09:25, Hidehiro Kawai wrote:
> Andreas Dilger wrote:
> > On Jun 05, 2008  09:20 -0700, Andrew Morton wrote:
> 
> >>I guess we need to undo this.  And yes, propagating errors into AS_EIO
> >>is the way.  I guess that's safe without holding lock_page(), as long
> >>as the bh is pinned.
> > 
> > Something like the following instead if -EIO and journal abort:
> > 
> > 		if (!buffer_uptodate(bh)) {
> > 			set_bit(AS_EIO, &bh->b_page->mapping->flags);
> > 			SetPageError(bh->b_page);
> > 		}
> > 
> > It seems end_buffer_async_write() does this already, but
> > journal_do_submit_data() uses end_buffer_write_sync() and it does not
> > do either of those operations.
> 
> Thank you for your suggestion.  I wrote an additional patch to do
> that below.  Please apply it as the 6th patch of this patch series.
> 
> BTW, I'm developing a patch which makes "abort the journal if a file
> data buffer has an error" tunable.  I'll send it in another thread
> because it's not a bug fix patch.
> 
> -- 
> Hidehiro Kawai
> Hitachi, Systems Development Laboratory
> Linux Technology Center
> 
> 
> Subject: JBD: don't abort if flushing file data failed
> 
> In ordered mode, it is not appropriate behavior to abort the journal
> when we failed to write file data.  This patch calls printk()
> instead of aborting the journal.  Additionally, set AS_EIO into
> the address_space object of the buffer which is written out by
> journal_do_submit_data() and failed so that fsync() can get -EIO.
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> ---
>  fs/jbd/commit.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.26-rc4/fs/jbd/commit.c
> ===================================================================
> --- linux-2.6.26-rc4.orig/fs/jbd/commit.c
> +++ linux-2.6.26-rc4/fs/jbd/commit.c
> @@ -432,8 +432,11 @@ void journal_commit_transaction(journal_
>  			wait_on_buffer(bh);
>  			spin_lock(&journal->j_list_lock);
>  		}
> -		if (unlikely(!buffer_uptodate(bh)))
> +		if (unlikely(!buffer_uptodate(bh))) {
> +			set_bit(AS_EIO, &bh->b_page->mapping->flags);
> +			SetPageError(bh->b_page);
>  			err = -EIO;
> +		}
  Actually, you should be more careful here because if the data buffer has
been truncated in the currently running transaction, it can happen that
b_page->mapping is NULL. It is a question how to safely access
page->mapping - probably you'll need page lock for that...

>  		if (!inverted_lock(journal, bh)) {
>  			put_bh(bh);
>  			spin_lock(&journal->j_list_lock);
> @@ -452,8 +455,11 @@ void journal_commit_transaction(journal_
>  	}
>  	spin_unlock(&journal->j_list_lock);
>  
> -	if (err)
> -		journal_abort(journal, err);
> +	if (err) {
> +		printk(KERN_WARNING
> +		       "JBD: Detected IO errors during flushing file data\n");
> +		err = 0;
> +	}
>  
>  	journal_write_revoke_records(journal, commit_transaction);

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR