Messages in this thread | | | Date | Fri, 19 May 2006 15:18:04 -0400 | From | Chuck Lever <> | Subject | Re: [PATCH 3/6] nfs: Eliminate nfs_get_user_pages() |
| |
Andrew Morton wrote: > 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(¤t->mm->mmap_sem); >> - result = get_user_pages(current, current->mm, user_addr, >> - page_count, (rw == READ), 0, >> - *pages, NULL); >> - up_read(¤t->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(¤t->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.
I fixed this by making all of these an "unsigned long". get_user_pages() returns an unsigned long result, so all these comparisons should always work correctly.
nfs_count_pages() now also returns an unsigned long, but I don't see how it is possible for it to compute a negative value.
> 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().
-- corporate: cel at netapp dot com personal: chucklever at bigfoot dot com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |