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]

Message-ID: <x49zkwtb7ou.fsf@segfault.boston.devel.redhat.com>
Date:	Wed, 11 Aug 2010 09:27:45 -0400
From:	Jeff Moyer <jmoyer@...hat.com>
To:	Josef Bacik <josef@...hat.com>
Cc:	Christian Ehrhardt <ehrhardt@...ux.vnet.ibm.com>,
	Christoph Hellwig <hch@...radead.org>,
	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

Josef Bacik <josef@...hat.com> writes:

> So say blocksize of 4k, we do dio to 12k, the first time around
> dio->block_in_file is 0, we set dio->cur_page, and move on to the next page, and
> bio->block_in_file is set to 1.  We find that dio->cur_page is set, so we do
> dio_send_cur_page().  Since !dio->bio we create a new bio, and set
> dio->logical_offset_in_bio to 0, since thats the offset of dio->cur_page.  Then
> we setup the next cur_page as the page for logical block 1, and
> dio->block_in_file gets bumped to 2.  We map the next block and come into
> dio_send_cur_page() again.  At this point cur_offset would be 8192...and shit I
> just realized what was wrong.  If you change
>
> loff_t cur_offset = dio->block_in_file << dio->blkbits;
>
> to
>
> loff_t cur_offset = dio->cur_page_fs_offset << dio->blkbits;

Sorry, I wasn't very clear in my description, but you figured it out.
;-)  Of course, cur_page_fs_offset is already in bytes, so that left
shift is not necessary.

> That should fix the problem.  Sorry guys, I screwed that up.  I'll look at this
> again tomorrow after I've had my 2 hours of sleep and see if this all still
> makes sense, but I think the above should fixe the performance thing.  As for
> the dio->boundary thing, dio_bio_submit() sets dio->boundary to 0, so the same
> bio won't be submitted twice.

While I don't doubt that you are right, I will sleep better at night if
we do an else if.  (To be fair, this ambiguity was not introduced by
you).

I've tested this patch, added printk's and watched blktrace to verify
that we don't split up I/Os.  So long as no one objects, I'll post this
for inclusion in a new thread.

Thanks for looking into it, Josef.

Cheers,
Jeff

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 7600aac..445901c 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -632,7 +632,7 @@ static int dio_send_cur_page(struct dio *dio)
 	int ret = 0;
 
 	if (dio->bio) {
-		loff_t cur_offset = dio->block_in_file << dio->blkbits;
+		loff_t cur_offset = dio->cur_page_fs_offset;
 		loff_t bio_next_offset = dio->logical_offset_in_bio +
 			dio->bio->bi_size;
 
@@ -657,7 +657,7 @@ static int dio_send_cur_page(struct dio *dio)
 		 * Submit now if the underlying fs is about to perform a
 		 * metadata read
 		 */
-		if (dio->boundary)
+		else if (dio->boundary)
 			dio_bio_submit(dio);
 	}
 
--
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/