linux-kernel - Re: [PATCH 02/11] writeback: Factor writeback_get_batch() out of write_cache

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: <20231215141445.rnt34v6emxulezde@quack3>
Date: Fri, 15 Dec 2023 15:14:45 +0100
From: Jan Kara <jack@...e.cz>
To: Christoph Hellwig <hch@....de>
Cc: linux-mm@...ck.org, "Matthew Wilcox (Oracle)" <willy@...radead.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jan Kara <jack@...e.com>, David Howells <dhowells@...hat.com>
Subject: Re: [PATCH 02/11] writeback: Factor writeback_get_batch() out of
 write_cache_pages()

On Thu 14-12-23 14:25:35, Christoph Hellwig wrote:
> From: "Matthew Wilcox (Oracle)" <willy@...radead.org>
> 
> This simple helper will be the basis of the writeback iterator.
> To make this work, we need to remember the current index
> and end positions in writeback_control.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>
> Signed-off-by: Christoph Hellwig <hch@....de>

Just some nits:

> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -81,6 +81,8 @@ struct writeback_control {
>  
>  	/* internal fields used by the ->writepages implementation: */
>  	struct folio_batch fbatch;
> +	pgoff_t index;
> +	pgoff_t end;			/* inclusive */
>  	pgoff_t done_index;

I don't think we need to cache 'end' since it isn't used that much. In
writeback_get_batch() we can just compute it locally as:

	if (wbc->range_cyclic)
		end = -1;
	else
		end = wbc->range_end >> PAGE_SHIFT;

and in the termination condition of the loop we can have it like:

	while (wbc->range_cyclic || index <= wbc->range_end >> PAGE_SHIFT)

Also I don't think we need both done_index and index since they are closely
related and when spread over several functions it gets a bit confusing
what's for what. So I'd just remove done_index, use index instead for
setting writeback_index and just reset 'index' to the desired value in
those two cases where we break out of the loop early and thus index !=
done_index.

I'm sorry for nitpicking about these state variables but IMO reducing their
amount actually makes things easier to verify (and thus maintain) when they
are spread over several functions.

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