Re: [PATCH] mm/hugetlb: simplify refs in memfd_alloc_folio

[Matthew Wilcox <willy@infradead.org>]

On Wed, Sep 04, 2024 at 12:41:08PM -0700, Steve Sistare wrote:
> The folio_try_get in memfd_alloc_folio is not necessary.  Delete it, and
> delete the matching folio_put in memfd_pin_folios.  This also avoids
> leaking a ref if the memfd_alloc_folio call to hugetlb_add_to_page_cache
> fails, which would otherwise need an additional folio_put.  This is a
> continuation of the fix
>   "mm/hugetlb: fix memfd_pin_folios free_huge_pages leak"

I think you're right, but don't we also need to get rid of the
folio_put() call in the 'if (err)' case after calling
hugetlb_add_to_page_cache()?

> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning memfd folios")
> 
> Suggested-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  mm/gup.c   | 4 +---
>  mm/memfd.c | 2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index bccabaa..947881ff 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -3618,7 +3618,7 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
>  	pgoff_t start_idx, end_idx, next_idx;
>  	struct folio *folio = NULL;
>  	struct folio_batch fbatch;
> -	struct hstate *h = NULL;
> +	struct hstate *h;
>  	long ret = -EINVAL;
>  
>  	if (start < 0 || start > end || !max_folios)
> @@ -3662,8 +3662,6 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
>  							     &fbatch);
>  			if (folio) {
>  				folio_put(folio);
> -				if (h)
> -					folio_put(folio);
>  				folio = NULL;
>  			}
>  
> diff --git a/mm/memfd.c b/mm/memfd.c
> index bcb131d..f715301 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -89,7 +89,7 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>  						    numa_node_id(),
>  						    NULL,
>  						    gfp_mask);
> -		if (folio && folio_try_get(folio)) {
> +		if (folio) {
>  			err = hugetlb_add_to_page_cache(folio,
>  							memfd->f_mapping,
>  							idx);
> -- 
> 1.8.3.1
>