linux-ext4 - Re: [PATCH] ext4: fix dirty pages writback regression.

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130910090044.GB894@quack.suse.cz>
Date:	Tue, 10 Sep 2013 11:00:44 +0200
From:	Jan Kara <jack@...e.cz>
To:	"Yan, Zheng" <zheng.z.yan@...el.com>
Cc:	linux-ext4@...r.kernel.org, jack@...e.cz, tytso@....edu,
	lkp@...ux.intel.com
Subject: Re: [PATCH] ext4: fix dirty pages writback regression.

On Tue 10-09-13 10:02:58, Yan, Zheng wrote:
> From: "Yan, Zheng" <zheng.z.yan@...el.com>
> 
> Our Linux Kernel Performance project found that commit 4e7ea81db5
> (ext4: restructure writeback path) indroduced regression. After
> the commit, ext4 does not merge adjacent mapped dirty pages during
> writeback. The "!buffer_delay(bh) && !buffer_unwritten(bh)" check
> in mpage_add_bh_to_extent() prevents the merging.
>
> Signed-off-by: Yan, Zheng <zheng.z.yan@...el.com>
> ---
>  fs/ext4/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c79fd7d..bfeb8b2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1944,8 +1944,7 @@ static bool mpage_add_bh_to_extent(struct mpage_da_data *mpd, ext4_lblk_t lblk,
>  	struct ext4_map_blocks *map = &mpd->map;
>  
>  	/* Buffer that doesn't need mapping for writeback? */
> -	if (!buffer_dirty(bh) || !buffer_mapped(bh) ||
> -	    (!buffer_delay(bh) && !buffer_unwritten(bh))) {
> +	if (!buffer_dirty(bh) || !buffer_mapped(bh)) {
  Sadly it isn't that easy. The condition is there for a reason... The
reason is that we are looking for an extent to map. When we already have
some buffer to map and then there is buffer which doesn't need mapping we
cannot just add it to the extent because then we would allocate too many
blocks. Also the transaction credits we have reserved are just for
allocation of one extent and its possible conversion from unwritten to
written extent. So that's another reason why you cannot arbitrarily merge
allocated and unallocated buffers or written and unwritten buffers.

Now also I'm somewhat surprised that this condition is causing a regression
because it was also present in the previous version of the code although it
was there in a different place and in a slightly different form. I'll try to
reproduce results using your fio script and will have a look at what is
causing the problem.

								Honza

>  		/* So far no extent to map => we write the buffer right away */
>  		if (map->m_len == 0)
>  			return true;
> -- 
> 1.8.1.4
> 
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html