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

login
register
mail settings
Submitter Jan Kara
Date March 29, 2013, 5:15 p.m.
Message ID <20130329171536.GF5913@quack.suse.cz>
Download mbox | patch
Permalink /patch/232465/
State New
Headers show

Comments

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

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