bio splits unnecessarily due to BH_Boundary in ext3 direct I/O

Submitted by Jan Kara on March 29, 2013, 5:15 p.m.

Details

Message ID 20130329171536.GF5913@quack.suse.cz
State Accepted, archived
Headers show

Commit Message

Jan Kara March 29, 2013, 5:15 p.m.
On Thu 21-03-13 17:43:52, Kazuya Mio wrote:
> 2013/03/20 4:31, Jan Kara wrote:
> >  I'm not sure I understand. Looking into dio_send_cur_page() it seems may
> >prematurely submit the bio if sdio->boundary is set - in that case we
> >should probably first try to add the page to the bio and submit the bio
> >only after that. Is that what you mean?
> 
> I think the direct I/O works for each page into buffer_head by the following
> three steps:
> 1. submit sdio->bio if sdio->boudary is set
> 2. add sdio->cur_page to sdio->bio by dio_new_bio() or dio_bio_add_page()
> 3. set the curret page to sdio->cur_page in submit_page_section()
> 
> It is true that dio_send_cur_page() submits the bio if sdio->boudary is set.
> However, at this time, this bio does not contain sdio->cur_page and
> the current page do_direct_IO() passed.
  Sorry for not getting to you earlier. So we agree in our analysis. Do you
agree with the attached patch? Does it help your workload?

									Honza

Comments

Kazuya Mio April 1, 2013, 8:25 a.m.
2013/03/30 2:15, Jan Kara wrote:
>   Sorry for not getting to you earlier. So we agree in our analysis. Do you
> agree with the attached patch? Does it help your workload?

According to your two patches, direct I/O works as the following steps:
1. add sdio->cur_page to sdio->bio by dio_new_bio() or dio_bio_add_page()
2. submit sdio->bio if sdio->boudary is set
3. set the curret page to sdio->cur_page in submit_page_section()

Only one page is submitted separately because of submitting bio before step 3.

For example, we write 52KB data with O_DIRECT, it is ideal that filesystem
submits a bio twice (48KB and 4KB). However, after applying your two patches,
52KB data is split into three bios (44KB, 4KB and 4KB).

Regards,
Kazuya Mio
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

From d9ef6ae45c80b298f7f3b718a101956071709b02 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Fri, 29 Mar 2013 18:05:01 +0100
Subject: [PATCH] direct-io: Submit bio after boundary buffer is added to it

Currently, dio_send_cur_page() submits bio before current page
(sdio->cur_page) is added to the bio if sdio->boundary is set. This is
actually wrong because sdio->boundary means the current buffer is the
last one before metadata needs to be read. So we should rather submit
the bio *after* the current page is added to it.

Reported-by: Kazuya Mio <k-mio@sx.jp.nec.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/direct-io.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index e666854..2ccde31 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -672,12 +672,6 @@  static inline int dio_send_cur_page(struct dio *dio, struct dio_submit *sdio,
 		if (sdio->final_block_in_bio != sdio->cur_page_block ||
 		    cur_offset != bio_next_offset)
 			dio_bio_submit(dio, sdio);
-		/*
-		 * Submit now if the underlying fs is about to perform a
-		 * metadata read
-		 */
-		else if (sdio->boundary)
-			dio_bio_submit(dio, sdio);
 	}
 
 	if (sdio->bio == NULL) {
@@ -694,6 +688,12 @@  static inline int dio_send_cur_page(struct dio *dio, struct dio_submit *sdio,
 			BUG_ON(ret != 0);
 		}
 	}
+	/*
+	 * Submit now if the underlying fs is about to perform a
+	 * metadata read
+	 */
+	if (sdio->boundary)
+		dio_bio_submit(dio, sdio);
 out:
 	return ret;
 }
-- 
1.7.1