Re: [PATCH 3/6] nfs: Eliminate nfs_get_user_pages()

Andrew Morton
2006-05-19T18:15:05Z

Chuck Lever <cel@netapp.com> wrote:
>
> Neil Brown observed that the kmalloc() in nfs_get_user_pages() is more
> likely to fail if the I/O is large enough to require the allocation of more
> than a single page to keep track of all the pinned pages in the user's
> buffer.
>
> Instead of tracking one large page array per dreq/iocb, track pages per
> nfs_read/write_data, just like the cached I/O path does. An array for
> pages is already allocated for us by nfs_readdata_alloc() (and the write
> and commit equivalents).
>
> This is also required for adding support for vectored I/O to the NFS direct
> I/O path.
>
> The original reason to pin the user buffer and allocate all the NFS data
> structures before trying to schedule I/O was to ensure all needed resources
> are allocated on the client before starting to send requests. This reduces
> the chance that resource exhaustion on the client will cause a short read
> or write.
>
> On the other hand, for an application making very large application I/O
> requests, this means that it will be nearly impossible for the application
> to make forward progress on a resource-limited client.
>
> Thus, moving the buffer pinning functionality into the I/O scheduling
> loops should be good for scalability. The next patch will do the same for
> NFS data structure allocation.
>
> +static void nfs_release_user_pages(struct page **pages, int npages)
> {
> - int result = -ENOMEM;
> - unsigned long page_count;
> - size_t array_size;
> -
> - page_count = (user_addr + size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> - page_count -= user_addr >> PAGE_SHIFT;
> -
> - array_size = (page_count * sizeof(struct page *));
> - *pages = kmalloc(array_size, GFP_KERNEL);
> - if (*pages) {
> - down_read(&current->mm->mmap_sem);
> - result = get_user_pages(current, current->mm, user_addr,
> - page_count, (rw == READ), 0,
> - *pages, NULL);
> - up_read(&current->mm->mmap_sem);
> - if (result != page_count) {
> - /*
> - * If we got fewer pages than expected from
> - * get_user_pages(), the user buffer runs off the
> - * end of a mapping; return EFAULT.
> - */
> - if (result >= 0) {
> - nfs_free_user_pages(*pages, result, 0);
> - result = -EFAULT;
> - } else
> - kfree(*pages);
> - *pages = NULL;
> - }
> - }
> - return result;
> + int i;
> + for (i = 0; i < npages; i++)
> + page_cache_release(pages[i]);
> }

If `npages' is negative, this does the right thing.

> + result = get_user_pages(current, current->mm, user_addr,
> + data->npages, 1, 0, data->pagevec, NULL);
> + up_read(&current->mm->mmap_sem);
> + if (unlikely(result < data->npages))
> + goto out_err;
> ...
> +out_err:
> + nfs_release_user_pages(data->pagevec, result);

And `npages' can indeed be negative.

So. No bug there, but the code is a little unobvious and fragile - if
someone were to alter a type then subtle bugs would happen.

Perhaps

if (result > 0)
nfs_release_user_pages(...);

would be cleaner. Or at least a loud comment in nfs_release_user_pages().