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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Oct 03, 2023 at 12:44:45AM -0700, Vivek Kasireddy wrote:

> +/**
> + * pin_user_pages_fd() - pin user pages associated with a file
> + * @fd:         the fd whose pages are to be pinned
> + * @start:      starting file offset
> + * @nr_pages:   number of pages from start to pin
> + * @gup_flags:  flags modifying pin behaviour
> + * @pages:      array that receives pointers to the pages pinned.
> + *              Should be at least nr_pages long.
> + *
> + * Attempt to pin (and migrate) pages associated with a file belonging to
> + * either shmem or hugetlbfs. An error is returned if pages associated with
> + * hugetlbfs files are not present in the page cache. However, shmem pages
> + * are swapped in or allocated if they are not present in the page cache.
> + *
> + * Returns number of pages pinned. This would be equal to the number of
> + * pages requested.
> + * If nr_pages is 0 or negative, returns 0. If no pages were pinned, returns
> + * -errno.
> + */
> +long pin_user_pages_fd(int fd, pgoff_t start, unsigned long nr_pages,
> +		       unsigned int gup_flags, struct page **pages)
> +{
> +	struct page *page;
> +	struct file *filep;
> +	unsigned int flags, i;
> +	long ret;
> +
> +	if (nr_pages <= 0)
> +		return 0;
> +	if (!is_valid_gup_args(pages, NULL, &gup_flags, FOLL_PIN))
> +		return 0;
> +
> +	if (start < 0)
> +		return -EINVAL;
> +
> +	filep = fget(fd);
> +	if (!filep)
> +	    return -EINVAL;

I think the caller should pass in the file *

In some cases we will need to hold a reference on it for a long time.

> +	if (!shmem_file(filep) && !is_file_hugepages(filep))
> +	    return -EINVAL;
> +
> +	flags = memalloc_pin_save();
> +	do {
> +		for (i = 0; i < nr_pages; i++) {
> +			if (shmem_mapping(filep->f_mapping)) {
> +				page = shmem_read_mapping_page(filep->f_mapping,
> +							       start + i);
> +				if (IS_ERR(page)) {
> +					ret = PTR_ERR(page);
> +					goto err;
> +				}
> +			} else {
> +				page = find_get_page_flags(filep->f_mapping,
> +							   start + i,
> +							   FGP_ACCESSED);
> +				if (!page) {
> +					ret = -EINVAL;
> +					goto err;
> +				}

I don't know these APIs at all, but I admit to being surprised we need
the special case for shmem ?

> +			ret = try_grab_page(page, FOLL_PIN);
> +			if (unlikely(ret))
> +				goto err;
> +
> +			pages[i] = page;
> +			put_page(pages[i]);
> +		}
> +
> +		ret = check_and_migrate_movable_pages(nr_pages, pages);
> +	} while (ret == -EAGAIN);

It seems OK, but I do wish it was faster :) Maybe for another day.

Jason