linux-kernel - Re: [PATCH 24.5/30] jbd2: Modify ASYNC_COMMIT code to not rely on queue draining on barrier

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: <08ADE50E-9B33-4834-B08A-AD342D63D1F9@dilger.ca>
Date:	Mon, 6 Sep 2010 13:15:52 +0200
From:	Andreas Dilger <adilger@...ger.ca>
To:	Tejun Heo <teheo@...e.de>
Cc:	jaxboe@...ionio.com, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-scsi@...r.kernel.org,
	linux-ide@...r.kernel.org, linux-raid@...r.kernel.org, hch@....de,
	James.Bottomley@...e.de, tytso@....edu, chris.mason@...cle.com,
	swhiteho@...hat.com, konishi.ryusuke@....ntt.co.jp,
	dm-devel@...hat.com, vst@...b.net, jack@...e.cz,
	rwheeler@...hat.com, hare@...e.de, neilb@...e.de,
	rusty@...tcorp.com.au, mst@...hat.com, jeremy@...p.org,
	snitzer@...hat.com, k-ueda@...jp.nec.com
Subject: Re: [PATCH 24.5/30] jbd2: Modify ASYNC_COMMIT code to not rely on queue draining on barrier

On 2010-08-26, at 10:23, Tejun Heo wrote:
> From 49f4cef00a1bd3c79fb2fe1f982c5157f0792867 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@...e.cz>
> 
> Currently JBD2 relies blkdev_issue_flush() draining the queue when ASYNC_COMMIT
> feature is set. This property is going away so make JBD2 wait for buffers it
> needs on its own before submitting the cache flush.

I finally had a chance to look at this patch more closely, and I think it may be breaking the ASYNC_COMMIT functionality, by forcing a wait for all of the data blocks _before_ the journal commit block is even submitted, even though ASYNC_COMMIT is enabled.

When ASYNC_COMMIT is enabled, it means that the journal transaction coherency is handled by the commit block checksum of the transaction data blocks, so the commit block can be submitted to the journal at the same time as the transaction data blocks.  The flush on the journal device (and the filesystem device, if they are separate) should happen after both are submitted.

However, if ASYNC_COMMIT is NOT enabled, then the transaction data blocks should be submitted and flushed before the journal commit block is submitted, and then there should be a second cache flush afterward.

---
> This patch is necessary before enabling flush/fua support in jbd2.
> The flush-fua git tree has been udpated to included this between patch
> 24 and 25.
> 
> Thanks.
> 
> fs/jbd2/commit.c |   29 ++++++++++++++++-------------
> 1 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 7c068c1..8797fd1 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -701,6 +701,16 @@ start_journal_io:
> 		}
> 	}
> 
> +	err = journal_finish_inode_data_buffers(journal, commit_transaction);
> +	if (err) {
> +		printk(KERN_WARNING
> +			"JBD2: Detected IO errors while flushing file data "
> +		       "on %s\n", journal->j_devname);
> +		if (journal->j_flags & JBD2_ABORT_ON_SYNCDATA_ERR)
> +			jbd2_journal_abort(journal, err);
> +		err = 0;
> +	}
> +
> 	/*
> 	 * If the journal is not located on the file system device,
> 	 * then we must flush the file system device before we issue
> @@ -719,19 +729,6 @@ start_journal_io:
> 						 &cbh, crc32_sum);
> 		if (err)
> 			__jbd2_journal_abort_hard(journal);
> -		if (journal->j_flags & JBD2_BARRIER)
> -			blkdev_issue_flush(journal->j_dev, GFP_KERNEL, NULL,
> -				BLKDEV_IFL_WAIT);
> -	}
> -
> -	err = journal_finish_inode_data_buffers(journal, commit_transaction);
> -	if (err) {
> -		printk(KERN_WARNING
> -			"JBD2: Detected IO errors while flushing file data "
> -		       "on %s\n", journal->j_devname);
> -		if (journal->j_flags & JBD2_ABORT_ON_SYNCDATA_ERR)
> -			jbd2_journal_abort(journal, err);
> -		err = 0;
> 	}
> 
> 	/* Lo and behold: we have just managed to send a transaction to
> @@ -845,6 +842,12 @@ wait_for_iobuf:
> 	}
> 	if (!err && !is_journal_aborted(journal))
> 		err = journal_wait_on_commit_record(journal, cbh);
> +	if (JBD2_HAS_INCOMPAT_FEATURE(journal,
> +				      JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT) &&
> +	    journal->j_flags & JBD2_BARRIER) {
> +			blkdev_issue_flush(journal->j_dev, GFP_KERNEL, NULL,
> +				BLKDEV_IFL_WAIT);
> +	}
> 
> 	if (err)
> 		jbd2_journal_abort(journal, err);
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas





--
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/