Messages in this thread | | | Date | Fri, 15 Dec 2023 15:14:45 +0100 | From | Jan Kara <> | 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@infradead.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@infradead.org> > Signed-off-by: Christoph Hellwig <hch@lst.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@suse.com> SUSE Labs, CR
| |