linux-ext4 - Re: ext4: try to improve unwritten extents merges

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: <e4de55ba-716f-bd8c-7da7-05092052ebf4@linux.alibaba.com>
Date:   Tue, 20 Nov 2018 18:48:07 +0800
From:   Xiaoguang Wang <xiaoguang.wang@...ux.alibaba.com>
To:     Jan Kara <jack@...e.cz>
Cc:     darrick.wong@...cle.com, linux-ext4@...r.kernel.org
Subject: Re: ext4: try to improve unwritten extents merges

hi,

> Hello!
> 
> On Tue 20-11-18 17:01:25, Xiaoguang Wang wrote:
>> First sorry to bother you again, recently we meet a
>> "dioread_nolock,nodelalloc" slow writeback issue, Liu Bo has sent a patch to
>> fix this issue. But here I also wonder whether we can merge unwritten
>> extents as far as possible.
>> In current codes:
>>
>> int
>> ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
>> 				struct ext4_extent *ex2)
>> {
>> ...
>> 	if (ext4_ext_is_unwritten(ex1) &&
>> 	    (ext4_test_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN) ||
>> 	     atomic_read(&EXT4_I(inode)->i_unwritten) ||
>> 	     (ext1_ee_len + ext2_ee_len > EXT_UNWRITTEN_MAX_LEN)))
>> 		return 0;
>> ...
>> }
>> This was added by Darrick in 2014:
>> commit a9b8241594adda0a7a4fb3b87bf29d2dff0d997d
>> Author: Darrick J. Wong <darrick.wong@...cle.com>
>> Date:   Thu Feb 20 21:17:35 2014 -0500
>>
>>      ext4: merge uninitialized extents
>>
>>      Allow for merging uninitialized extents.
>>
>>      Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>
>>      Signed-off-by: "Theodore Ts'o" <tytso@....edu>
>>
>> So long as we have a unwritten extents under io(which also means i_unwritten
>> is not zero), then we can not do merge work for unwritten extents, I wonder
>> whether this conditon is too strict. Assume that the
>> begin of the file is under io, but middle or end of the file could not
>> merge unwritten extetns, though they could be.
>>
>> I'm not sure whether we could directly remove
>> "atomic_read(&EXT4_I(inode)->i_unwritten)",if not, here I make a simple
>> patch to respect same semantics. The idea is simple, I use a red-black
>> tree to record unwritten extents under io, when trying to merging
>> unwritten extents, we search this per-inode tree, it not hit, we can
>> merge. I have also run "xfstests quick group test cases", look like that
>> it works well. dio maybe also go to this way.
> 
> The reason why we don't merge unwritten extents if there is IO to unwritten
> extents running is that we split unwritten extents to match exactly the IO
> range on submission and then convert it to written extents on IO
> completion. So we must avoid merging these split out extents while the IO
> is running.
I see, thanks.

> 
> I agree that the condition in ext4_can_extents_be_merged() is rather coarse
> so it would be nice to improve it so that unwritten extents on which IO is
> not running can be merged. I've also observed that unwritten extents get
> fragmented relatively easily under some workloads.
> 
> Rather than introducing new RB-tree for this (which costs additional memory
> and its maintenance costs also CPU time), I'd use extent status tree to
> identify unwritten extent that got split out when preparing the IO (you
> should mark such extent in ext4_map_blocks() when EXT4_GET_BLOCKS_IO_SUBMIT
> flag is set). Then the flag would get cleared on extent conversion to
> written one.
Agree, thanks for your helps and suggestions, I'll try this method.

Regards,
Xiaoguang Wang
> 
> 								Honza
>