linux-kernel - Re: [PATCH] jbd: don't abort if flushing file data failed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20080619163044.GB7874@skywalker>
Date:	Thu, 19 Jun 2008 22:00:44 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, sct@...hat.com,
	adilger@...sterfs.com, linux-kernel@...r.kernel.org,
	linux-ext4@...r.kernel.org, Jan Kara <jack@...e.cz>,
	Josef Bacik <jbacik@...hat.com>, Mingming Cao <cmm@...ibm.com>,
	"Theodore Ts'o" <tytso@....edu>,
	sugita <yumiko.sugita.yf@...achi.com>,
	Satoshi OSHIMA <satoshi.oshima.fk@...achi.com>
Subject: Re: [PATCH] jbd: don't abort if flushing file data failed

On Thu, Jun 19, 2008 at 03:32:34PM +0900, Hidehiro Kawai wrote:
> In ordered mode, the current jbd aborts the journal if a file data
> buffer has an error.  But this behavior is unintended, and we found
> that it has been adopted accidentally.

Do you have more information on this ?  Isn't ordered mode required to
guarantee that file data hit the disk and if there are errors in sending
data to the disk, should we not mark the file system readonly by aborting
the journal ?


> 
> This patch undoes it and just calls printk() instead of aborting
> the journal.  Additionally, set AS_EIO into the address_space
> object of the failed buffer which is submitted by
> journal_do_submit_data() so that fsync() can get -EIO.
> 
> Missing error checkings are also added to inform errors on file
> data buffers to the user.  The following buffers are targeted.
> 
>   (a) the buffer which has already been written out by pdflush
>   (b) the buffer which has been unlocked before scanned in the
>       t_locked_list loop
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>


We may want a similar patch for jbd2 also. IF you are doing that can you
make sure  you do that against the patch queue at
http://repo.or.cz/w/ext4-patch-queue.git. There are some changes in the
same area in the patch queue.

> ---
>  fs/jbd/commit.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6.26-rc5-mm3/fs/jbd/commit.c
> ===================================================================
> --- linux-2.6.26-rc5-mm3.orig/fs/jbd/commit.c
> +++ linux-2.6.26-rc5-mm3/fs/jbd/commit.c
> @@ -172,7 +172,7 @@ static void journal_do_submit_data(struc
>  /*
>   *  Submit all the data buffers to disk
>   */
> -static void journal_submit_data_buffers(journal_t *journal,
> +static int journal_submit_data_buffers(journal_t *journal,
>  				transaction_t *commit_transaction)
>  {
>  	struct journal_head *jh;
> @@ -180,6 +180,7 @@ static void journal_submit_data_buffers(
>  	int locked;
>  	int bufs = 0;
>  	struct buffer_head **wbuf = journal->j_wbuf;
> +	int err = 0;
> 
>  	/*
>  	 * Whenever we unlock the journal and sleep, things can get added
> @@ -253,6 +254,8 @@ write_out_data:
>  			put_bh(bh);
>  		} else {
>  			BUFFER_TRACE(bh, "writeout complete: unfile");
> +			if (unlikely(!buffer_uptodate(bh)))
> +				err = -EIO;
>  			__journal_unfile_buffer(jh);
>  			jbd_unlock_bh_state(bh);
>  			if (locked)
> @@ -271,6 +274,8 @@ write_out_data:
>  	}
>  	spin_unlock(&journal->j_list_lock);
>  	journal_do_submit_data(wbuf, bufs);
> +
> +	return err;
>  }
> 
>  /*
> @@ -410,8 +415,7 @@ void journal_commit_transaction(journal_
>  	 * Now start flushing things to disk, in the order they appear
>  	 * on the transaction lists.  Data blocks go first.
>  	 */
> -	err = 0;
> -	journal_submit_data_buffers(journal, commit_transaction);
> +	err = journal_submit_data_buffers(journal, commit_transaction);
> 
>  	/*
>  	 * Wait for all previously submitted IO to complete.
> @@ -426,10 +430,21 @@ void journal_commit_transaction(journal_
>  		if (buffer_locked(bh)) {
>  			spin_unlock(&journal->j_list_lock);
>  			wait_on_buffer(bh);
> -			if (unlikely(!buffer_uptodate(bh)))
> -				err = -EIO;
>  			spin_lock(&journal->j_list_lock);
>  		}
> +		if (unlikely(!buffer_uptodate(bh))) {
> +			if (TestSetPageLocked(bh->b_page)) {
> +				spin_unlock(&journal->j_list_lock);
> +				lock_page(bh->b_page);
> +				spin_lock(&journal->j_list_lock);
> +			}
> +			if (bh->b_page->mapping)
> +				set_bit(AS_EIO, &bh->b_page->mapping->flags);
> +
> +			unlock_page(bh->b_page);
> +			SetPageError(bh->b_page);
> +			err = -EIO;
> +		}
>  		if (!inverted_lock(journal, bh)) {
>  			put_bh(bh);
>  			spin_lock(&journal->j_list_lock);
> @@ -448,8 +463,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);
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/