Re: [PATCH v5 3/5] mm/gup: Introduce pin_user_pages_fd() for pinning shmem/hugetlbfs file pages (v5)

[Christoph Hellwig <hch@infradead.org>]

> +static struct page *alloc_file_page(struct file *file, pgoff_t idx)

alloc_file_pages seems like a weird name for something that assumes
it is called either on a hugetlbfs or shmemfs file (without any
asserts that this is true).  gup.c also seems like a very odd place
for such a helper.

> + * Attempt to pin pages associated with a file that belongs to either shmem
> + * or hugetlb.

Why do we need a special case for hugetlb or shmemfs?

> +	if (!file)
> +	    return -EINVAL;
> +
> +	if (!shmem_file(file) && !is_file_hugepages(file))
> +	    return -EINVAL;

Indentation is messed up here.

> +		for (i = 0; i < nr_pages; i++) {
> +			/*
> + 			 * In most cases, we should be able to find the page
> +			 * in the page cache. If we cannot find it, we try to
> +			 * allocate one and add it to the page cache.
> +			 */
> +retry:
> +			folio = __filemap_get_folio(file->f_mapping,
> +						    start + i,
> +						    FGP_ACCESSED, 0);

__filemap_get_folio is a very inefficient way to find a
contiguous range of folios, I'd suggest to look into something that
batches instead.

> +			page = IS_ERR(folio) ? NULL: &folio->page;
> +			if (!page) {
> +				page = alloc_file_page(file, start + i);
> +				if (IS_ERR(page)) {
> +					ret = PTR_ERR(page);
> +					if (ret == -EEXIST)
> +						goto retry;
> +					goto err;
> +				}

This mix of folios and pages is odd.  Especially as hugetlbfs by
definitions uses large folios.