On Fri, Aug 1, 2025 at 10:07 AM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 31-07-25 17:21:26, Joanne Koong wrote:
> > Add filemap_dirty_folio_pages() which takes in the number of pages to dirty.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ...
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index b0ae10a6687d..a3805988f3ad 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -2732,7 +2732,7 @@ void folio_account_cleaned(struct folio *folio, struct bdi_writeback *wb)
> > * try_to_free_buffers() to fail.
> > */
> > void __folio_mark_dirty(struct folio *folio, struct address_space *mapping,
> > - int warn, long nr_pages)
> > + int warn, long nr_pages, bool newly_dirty)
> > {
> > unsigned long flags;
> >
> > @@ -2740,12 +2740,29 @@ void __folio_mark_dirty(struct folio *folio, struct address_space *mapping,
> > if (folio->mapping) { /* Race with truncate? */
> > WARN_ON_ONCE(warn && !folio_test_uptodate(folio));
> > folio_account_dirtied(folio, mapping, nr_pages);
> > - __xa_set_mark(&mapping->i_pages, folio_index(folio),
> > - PAGECACHE_TAG_DIRTY);
> > + if (newly_dirty)
> > + __xa_set_mark(&mapping->i_pages, folio_index(folio),
> > + PAGECACHE_TAG_DIRTY);
> > }
> > xa_unlock_irqrestore(&mapping->i_pages, flags);
>
> I think this is a dangerous coding pattern. What is making sure that by the
> time you get here newly_dirty is still valid? I mean the dirtying can race
> e.g. with writeback and so it can happen that the page is clean by the time
> we get here but newly_dirty is false. We are often protected by page lock
> when dirtying a folio but not always... So if nothing else this requires a
> careful documentation about correct use.
>
> Honza
I think races against writeback and truncation could already exist
here prior to this patch. afaict from the function documentation for
__folio_mark_dirty(), it's up to the caller to prevent this:
* It is the caller's responsibility to prevent the folio from being truncated
* while this function is in progress, although it may have been truncated
* before this function is called. Most callers have the folio locked.
* A few have the folio blocked from truncation through other means (e.g.
* zap_vma_pages() has it mapped and is holding the page table lock).
The documentation doesn't mention anything about writeback but I think
it applies here similarly.
I'm happy to do this another way though if there's a better approach here.
Thanks,
Joanne
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
