linux-kernel - Re: PATCH 3/6 - direct-io: do not merge logically non-contiguous requests

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]

Date:	Tue, 10 Aug 2010 14:40:06 -0400
From:	Jeff Moyer <jmoyer@...hat.com>
To:	Christian Ehrhardt <ehrhardt@...ux.vnet.ibm.com>
Cc:	Christoph Hellwig <hch@...radead.org>,
	Josef Bacik <josef@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-fsdevel@...r.kernel.org, linux-btrfs@...r.kernel.org
Subject: Re: PATCH 3/6 - direct-io: do not merge logically non-contiguous requests

Christian Ehrhardt <ehrhardt@...ux.vnet.ibm.com> writes:

> On 08/06/2010 02:03 PM, Christoph Hellwig wrote:
>> Something is deeply wrong here.  Raw block device access has a 1:1
>> mapping between logical and physical block numbers.  They really should
>> never be non-contiguous.
>
> At least I did nothing I know about to break it :-)

I think Christoph missed that you were using ext2, not the block device.

> As I mentioned just iozone using direct I/O (-I flag of iozone then
> using O_DIRECT for the file) on a ext2 file-system.
> The file system was coming clean out of mkfs the file was written with
> iozone one step before the traced read run.
>
> The only uncommon thing here might be the block device, which is a
> scsi disk on our SAN servers (I'm running on s390) - so the driver in
> charge is zfcp (drivers/s390/scsi/).
> I could use dasd (drivers/s390/block) disks as well, but I have no
> blktrace of them yet - what I already know is that they show a similar
> cost increase. On monday I should be able to get machine resources to
> verify that both disk types are affected.
>
> Let me know if I can do anything else on my system to shed some light
> on the matter.

Well, the problem is pretty obvious.  Inside submit_page_section, you
have this code:

	/*
	 * If there's a deferred page already there then send it.
	 */
	if (dio->cur_page) {
		ret = dio_send_cur_page(dio);
		page_cache_release(dio->cur_page);
		dio->cur_page = NULL;
		if (ret)
			goto out;
	}

	page_cache_get(page);/* It is in dio */
	dio->cur_page = page;
	dio->cur_page_offset = offset;
	dio->cur_page_len = len;
	dio->cur_page_block = blocknr;
	dio->cur_page_fs_offset = dio->block_in_file << dio->blkbits;

Notice that we're processing a new page, so we submit the old page for
I/O.

And in dio_send_cur_page, we have this:

	if (dio->final_block_in_bio != dio->cur_page_block ||
	    cur_offset != bio_next_offset)
		dio_bio_submit(dio);

So, we are actually comparing values between two different pages, and of
course, this doesn't work.  We're always one page behind in the I/O.

Also, the block of code above is immediately followed by this:

	/*
	 * Submit now if the underlying fs is about to perform a
	 * metadata read
	 */
	if (dio->boundary)
		dio_bio_submit(dio);

So, it looks to me like this could result in submitting the same bio
twice if you are unlucky enough.  I'll see what I can do to fix this
up.

Cheers,
Jeff
--
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/