Re: [PATCH v14 022/138] mm/filemap: Add __folio_lock_or_retry()

[Vlastimil Babka]

On 7/15/21 5:35 AM, Matthew Wilcox (Oracle) wrote:
> Convert __lock_page_or_retry() to __folio_lock_or_retry().  This actually
> saves 4 bytes in the only caller of lock_page_or_retry() (due to better
> register allocation) and saves the 14 byte cost of calling page_folio()
> in __folio_lock_or_retry() for a total saving of 18 bytes.  Also use
> a bool for the return type.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Reviewed-by: William Kucharski <william.kucharski@xxxxxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

Nit:

> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1625,48 +1625,46 @@ static int __folio_lock_async(struct folio *folio, struct wait_page_queue *wait)
>  
>  /*
>   * Return values:
> - * 1 - page is locked; mmap_lock is still held.
> - * 0 - page is not locked.
> + * true - folio is locked; mmap_lock is still held.
> + * false - folio is not locked.
>   *     mmap_lock has been released (mmap_read_unlock(), unless flags had both
>   *     FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_RETRY_NOWAIT set, in
>   *     which case mmap_lock is still held.
>   *
>   * If neither ALLOW_RETRY nor KILLABLE are set, will always return 1

s/1/true/ ? :)

> - * with the page locked and the mmap_lock unperturbed.
> + * with the folio locked and the mmap_lock unperturbed.
>   */
> -int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
> +bool __folio_lock_or_retry(struct folio *folio, struct mm_struct *mm,
>  			 unsigned int flags)
>  {
> -	struct folio *folio = page_folio(page);
> -
>  	if (fault_flag_allow_retry_first(flags)) {
>  		/*
>  		 * CAUTION! In this case, mmap_lock is not released
>  		 * even though return 0.
>  		 */
>  		if (flags & FAULT_FLAG_RETRY_NOWAIT)
> -			return 0;
> +			return false;
>  
>  		mmap_read_unlock(mm);
>  		if (flags & FAULT_FLAG_KILLABLE)
>  			folio_wait_locked_killable(folio);
>  		else
>  			folio_wait_locked(folio);
> -		return 0;
> +		return false;
>  	}
>  	if (flags & FAULT_FLAG_KILLABLE) {
> -		int ret;
> +		bool ret;
>  
>  		ret = __folio_lock_killable(folio);
>  		if (ret) {
>  			mmap_read_unlock(mm);
> -			return 0;
> +			return false;
>  		}
>  	} else {
>  		__folio_lock(folio);
>  	}
>  
> -	return 1;
> +	return true;
>  }
>  
>  /**