Re: [RFC PATCH v1 05/10] mm: add filemap_dirty_folio_pages() helper

[Jan Kara <jack@suse.cz>]

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@gmail.com>
...
> 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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR