On Thu 14-12-23 14:25:35, Christoph Hellwig wrote:
> From: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
