On Tue, Nov 19, 2024 at 10:46:54AM -0500, Brian Foster wrote:
> +loff_t
> +iomap_fill_dirty_folios(
> + struct inode *inode,
> + struct iomap *iomap,
If you pass in the batch directly instead of the iomap this is
completely generic and could go into filemap.c. Adding willy
and linux-mm for these kinds of things also tends to help to
get good review feedback and often improvements.
> + loff_t offset,
> + loff_t length)
> +{
> + struct address_space *mapping = inode->i_mapping;
> + struct folio_batch fbatch;
> + pgoff_t start, end;
> + loff_t end_pos;
> +
> + folio_batch_init(&fbatch);
> + folio_batch_init(&iomap->fbatch);
> +
> + end_pos = offset + length;
> + start = offset >> PAGE_SHIFT;
> + end = (end_pos - 1) >> PAGE_SHIFT;
Nit: initializing these at declaration time make the code easier to
read (at least for me :)).
> +
> + while (filemap_get_folios(mapping, &start, end, &fbatch) &&
> + folio_batch_space(&iomap->fbatch)) {
> + struct folio *folio;
> + while ((folio = folio_batch_next(&fbatch))) {
> + if (folio_trylock(folio)) {
> + bool clean = !folio_test_dirty(folio) &&
> + !folio_test_writeback(folio);
> + folio_unlock(folio);
> + if (clean)
> + continue;
What does the lock protect here given that it can become stale as soon
as we unlock?
Note that there also is a filemap_get_folios_tag that only looks up
folios with the right tag (dirty or writeback). Currently it only
supports a single tag, which wuld not be helpful here, but this might
be worth talking to the pagecache and xarray maintainer.
