linux-ext4 - Re: bio splits unnecessarily due to BH

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: <20130307104854.GB6723@quack.suse.cz>
Date:	Thu, 7 Mar 2013 11:48:54 +0100
From:	Jan Kara <jack@...e.cz>
To:	Kazuya Mio <k-mio@...jp.nec.com>
Cc:	jack@...e.cz, akpm@...ux-foundation.org, adilger.kernel@...ger.ca,
	linux-ext4@...r.kernel.org
Subject: Re: bio splits unnecessarily due to BH_Boundary in ext3 direct I/O

  Hello,

On Thu 07-03-13 17:36:07, Kazuya Mio wrote:
> I found the performance problem that ext3 direct I/O sends large number of bio
> unnecessarily when buffer_head is set BH_Boundary flag.
> 
> When we read/write a file sequentially, we will read/write not only
> the data blocks but also the indirect blocks that may not be physically
> adjacent to the data blocks. So ext3 sets BG_Boundary flag to submit
> the previous I/O before reading/writing an indirect block.
> 
> However, in the case of direct I/O, the size of buffer_head
> could be more than the blocksize. dio_send_cur_page() checks BH_Boundary flag
> and then calls submit_bio() without calling dio_bio_add_page().
> As a result, submit_bio() is called every one page and cause of high CPU usage.
  Yes, you are right that this is a bug. Thank you for reporting it!

> The following patch fixes this problem only for ext3. At least ext2/3/4
> don't need BH_Boundary flag for direct I/O because submit_bio() will be called
> when the offset of buffer_head is discontinuous about the previous one.
> 
> ---
> @@ -926,7 +926,8 @@ int ext3_get_blocks_handle(handle_t *handle, struct inode *inode,
>     set_buffer_new(bh_result);
>  got_it:
>     map_bh(bh_result, inode->i_sb, le32_to_cpu(chain[depth-1].key));
> -   if (count > blocks_to_boundary)
> +   /* set bourdary flag for buffered I/O */
> +   if (maxblocks == 1 && count > blocks_to_boundary)
>         set_buffer_boundary(bh_result);
>     err = count;
>     /* Clean up and exit */
> ---
  But I'm afraid your fix isn't quite correct. Because as I read the code
we will accumulate the bio, then read indirect block from get_more_blocks()
and only after that we find out bio won't be contiguous so we would submit
that. But the desired sequence is like:
  * accumulate the bio
  * find out it will not be contiguous so submit it
  * get_more_blocks() - submits read

I think the proper fix should be in fs/direct-io.c:
...
-		sdio->boundary = buffer_boundary(map_bh);
+		if (sdio->blocks_available == this_chunk_blocks)
+			sdio->boundary = buffer_boundary(map_bh);
...

Then we properly mark bio should be submitted only if we are mapping last
part of the mapped extent from the filesystem. Can you give this change a
try (full patch with changelog attached)?

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR

View attachment "0001-direct-io-Fix-boundary-block-handling.patch" of type "text/x-patch" (1492 bytes)