Messages in this thread | | | Date | Thu, 21 Dec 2023 12:17:43 +0100 | From | Jan Kara <> | Subject | Re: [PATCH 07/17] writeback: Factor writeback_get_batch() out of write_cache_pages() |
| |
On Mon 18-12-23 16:35:43, 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> > [hch: heavily rebased, add helpers to get the tag and end index, > don't keep the end index in struct writeback_control] > Signed-off-by: Christoph Hellwig <hch@lst.de>
Just two nits below. However you decide about them feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
> +static pgoff_t wbc_end(struct writeback_control *wbc) > +{ > + if (wbc->range_cyclic) > + return -1; > + return wbc->range_end >> PAGE_SHIFT; > +} > + > +static void writeback_get_batch(struct address_space *mapping, > + struct writeback_control *wbc) > +{ > + folio_batch_release(&wbc->fbatch); > + cond_resched();
I'd prefer to have cond_resched() explicitely in the writeback loop instead of hidden here in writeback_get_batch() where it logically does not make too much sense to me...
> + filemap_get_folios_tag(mapping, &wbc->index, wbc_end(wbc), > + wbc_to_tag(wbc), &wbc->fbatch); > +} > + > /** > * write_cache_pages - walk the list of dirty pages of the given address space and write all of them. > * @mapping: address space structure to write > @@ -2419,38 +2442,30 @@ int write_cache_pages(struct address_space *mapping, > void *data) > { > int error; > - int nr_folios; > - pgoff_t index; > pgoff_t end; /* Inclusive */ > - xa_mark_t tag; > > if (wbc->range_cyclic) { > - index = mapping->writeback_index; /* prev offset */ > + wbc->index = mapping->writeback_index; /* prev offset */ > end = -1; > } else { > - index = wbc->range_start >> PAGE_SHIFT; > + wbc->index = wbc->range_start >> PAGE_SHIFT; > end = wbc->range_end >> PAGE_SHIFT; > }
Maybe we should have: end = wbc_end(wbc);
when we have the helper? But I guess this gets cleaned up in later patches anyway so whatever.
Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
| |