Re: [PATCH 1/9] mm: Improve memalloc_nofs_save() documentation

[Jan Kara]

On Sun 30-08-26 05:18:48, Matthew Wilcox (Oracle) wrote:
> Talk about why someone should call this function instead of
> what this function does.  This makes the long comment in mm/readahead.c
> obsolete; replace it with a comment about which case we're protecting
> against.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>

Yeah, I think the documentation is better. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  include/linux/sched/mm.h | 31 +++++++++++++++++++++++--------
>  mm/readahead.c           | 14 ++------------
>  2 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index d7c6a942aa7e..f76be1141b06 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -372,13 +372,26 @@ static inline void memalloc_noio_restore(unsigned int flags)
>  }
>  
>  /**
> - * memalloc_nofs_save - Marks implicit GFP_NOFS allocation scope.
> + * memalloc_nofs_save - Prevent recursion into the filesystem.
>   *
> - * This functions marks the beginning of the GFP_NOFS allocation scope.
> - * All further allocations will implicitly drop __GFP_FS flag and so
> - * they are safe for the FS critical section from the allocation recursion
> - * point of view. Use memalloc_nofs_restore to end the scope with flags
> - * returned by this function.
> + * All memory allocations between calling this function and calling
> + * memalloc_nofs_restore() will be prevented from calling into filesystems
> + * to reclaim memory.  Clean page cache memory can still be reclaimed,
> + * but (for example) inodes will not be.
> + *
> + * The primary reason to do this is that the caller has taken a lock
> + * which would be needed by FS reclaim.  While we could theoretically
> + * call into a different filesystem in this case, it can be a deep call
> + * stack so it is better to avoid all filesystems.
> + *
> + * Filesystems often choose to incorporate a call to this function as part
> + * of starting a journal transaction.  While not a lock in the normal
> + * sense, it has much the same effect as nested journal transactions
> + * are either prohibited or expensive.
> + *
> + * Also call this function if you need to allocate memory while holding
> + * a file folio locked.  High order allocations (such as those requested
> + * by slab) can trigger compaction which will attempt to lock the folio.
>   *
>   * Context: This function is safe to be used from any context.
>   * Return: The saved flags to be passed to memalloc_nofs_restore.
> @@ -389,10 +402,12 @@ static inline unsigned int memalloc_nofs_save(void)
>  }
>  
>  /**
> - * memalloc_nofs_restore - Ends the implicit GFP_NOFS scope.
> + * memalloc_nofs_restore - End filesystem reclaim scope.
>   * @flags: Flags to restore.
>   *
> - * Ends the implicit GFP_NOFS scope started by memalloc_nofs_save function.
> + * Ends the implicit memory allocation scope started by
> + * memalloc_nofs_save().  This may not enable access to filesystem reclaim
> + * if it was already disabled at the time memalloc_nofs_save() was called.
>   * Always make sure that the given flags is the return value from the
>   * pairing memalloc_nofs_save call.
>   */
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 6e5563290287..9c116d4ba963 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -227,17 +227,7 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
>  	gfp_t gfp_mask = readahead_gfp_mask(mapping);
>  	unsigned long mark = ULONG_MAX, i = 0;
>  	unsigned int min_nrpages = mapping_min_folio_nrpages(mapping);
> -
> -	/*
> -	 * Partway through the readahead operation, we will have added
> -	 * locked pages to the page cache, but will not yet have submitted
> -	 * them for I/O.  Adding another page may need to allocate memory,
> -	 * which can trigger memory reclaim.  Telling the VM we're in
> -	 * the middle of a filesystem operation will cause it to not
> -	 * touch file-backed pages, preventing a deadlock.  Most (all?)
> -	 * filesystems already specify __GFP_NOFS in their mapping's
> -	 * gfp_mask, but let's be explicit here.
> -	 */
> +	/* Allocating with locked folios */
>  	unsigned int nofs = memalloc_nofs_save();
>  
>  	lockdep_assert_held(&mapping->invalidate_lock);
> @@ -512,7 +502,7 @@ void page_cache_ra_order(struct readahead_control *ractl,
>  
>  	ra->order = new_order;
>  
> -	/* See comment in page_cache_ra_unbounded() */
> +	/* Allocating with locked folios */
>  	nofs = memalloc_nofs_save();
>  	filemap_invalidate_lock_shared(mapping);
>  	/*
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR