Re: [PATCH v1 3/6] memcg-v1: no need for memcg locking for dirty tracking

[Michal Hocko <mhocko@suse.com>]

On Thu 24-10-24 18:23:00, Shakeel Butt wrote:
> During the era of memcg charge migration, the kernel has to be make sure
> that the dirty stat updates do not race with the charge migration.
> Otherwise it might update the dirty stats of the wrong memcg. Now with
> the memcg charge migration deprecated, there is no more race for dirty

s@deprecated@gone@

> stat updates and the previous locking can be removed.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

LGTM otherwise
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  fs/buffer.c         |  5 -----
>  mm/page-writeback.c | 16 +++-------------
>  2 files changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 1fc9a50def0b..88e765b0699f 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -736,15 +736,12 @@ bool block_dirty_folio(struct address_space *mapping, struct folio *folio)
>  	 * Lock out page's memcg migration to keep PageDirty
>  	 * synchronized with per-memcg dirty page counters.
>  	 */
> -	folio_memcg_lock(folio);
>  	newly_dirty = !folio_test_set_dirty(folio);
>  	spin_unlock(&mapping->i_private_lock);
>  
>  	if (newly_dirty)
>  		__folio_mark_dirty(folio, mapping, 1);
>  
> -	folio_memcg_unlock(folio);
> -
>  	if (newly_dirty)
>  		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
>  
> @@ -1194,13 +1191,11 @@ void mark_buffer_dirty(struct buffer_head *bh)
>  		struct folio *folio = bh->b_folio;
>  		struct address_space *mapping = NULL;
>  
> -		folio_memcg_lock(folio);
>  		if (!folio_test_set_dirty(folio)) {
>  			mapping = folio->mapping;
>  			if (mapping)
>  				__folio_mark_dirty(folio, mapping, 0);
>  		}
> -		folio_memcg_unlock(folio);
>  		if (mapping)
>  			__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
>  	}
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 1d7179aba8e3..a76a73529fd9 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2743,8 +2743,6 @@ EXPORT_SYMBOL(noop_dirty_folio);
>  /*
>   * Helper function for set_page_dirty family.
>   *
> - * Caller must hold folio_memcg_lock().
> - *
>   * NOTE: This relies on being atomic wrt interrupts.
>   */
>  static void folio_account_dirtied(struct folio *folio,
> @@ -2777,7 +2775,6 @@ static void folio_account_dirtied(struct folio *folio,
>  /*
>   * Helper function for deaccounting dirty page without writeback.
>   *
> - * Caller must hold folio_memcg_lock().
>   */
>  void folio_account_cleaned(struct folio *folio, struct bdi_writeback *wb)
>  {
> @@ -2795,9 +2792,8 @@ void folio_account_cleaned(struct folio *folio, struct bdi_writeback *wb)
>   * If warn is true, then emit a warning if the folio is not uptodate and has
>   * not been truncated.
>   *
> - * The caller must hold folio_memcg_lock().  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
> + * 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).
> @@ -2841,14 +2837,10 @@ void __folio_mark_dirty(struct folio *folio, struct address_space *mapping,
>   */
>  bool filemap_dirty_folio(struct address_space *mapping, struct folio *folio)
>  {
> -	folio_memcg_lock(folio);
> -	if (folio_test_set_dirty(folio)) {
> -		folio_memcg_unlock(folio);
> +	if (folio_test_set_dirty(folio))
>  		return false;
> -	}
>  
>  	__folio_mark_dirty(folio, mapping, !folio_test_private(folio));
> -	folio_memcg_unlock(folio);
>  
>  	if (mapping->host) {
>  		/* !PageAnon && !swapper_space */
> @@ -2975,14 +2967,12 @@ void __folio_cancel_dirty(struct folio *folio)
>  		struct bdi_writeback *wb;
>  		struct wb_lock_cookie cookie = {};
>  
> -		folio_memcg_lock(folio);
>  		wb = unlocked_inode_to_wb_begin(inode, &cookie);
>  
>  		if (folio_test_clear_dirty(folio))
>  			folio_account_cleaned(folio, wb);
>  
>  		unlocked_inode_to_wb_end(inode, &cookie);
> -		folio_memcg_unlock(folio);
>  	} else {
>  		folio_clear_dirty(folio);
>  	}
> -- 
> 2.43.5

-- 
Michal Hocko
SUSE Labs