| error = 0;
| while (spd.nr_pages < nr_pages) {
| /*
| * Page could be there, find_get_pages_contig() breaks on
| * the first hole.
| */
| page = find_get_page(mapping, index);
| if (!page) {
| /*
| * page didn't exist, allocate one.
| */
| page = page_cache_alloc_cold(mapping);
| if (!page)
error = -ENOMEM?
| break;
|
| error = add_to_page_cache_lru(page, mapping, index,
| mapping_gfp_mask(mapping));
| if (unlikely(error)) {
| page_cache_release(page);
| if (error == -EEXIST)
error = 0? It may not matter, but leaving error as EEXIST is
confusing at best (and coupled with the above missing ENOMEM could
result in really weird errors for splice() ;).
| continue;
| break;
| }
| /*
| * add_to_page_cache() locks the page, unlock it
| * to avoid convoluting the logic below even more.
| */
| unlock_page(page);
| }
|
| pages[spd.nr_pages++] = page;
| index++;
| }
...
|
| /*
| * need to read in the page
| */
| error = mapping->a_ops->readpage(in, page);
| if (unlikely(error)) {
| /*
| * We really should re-lookup the page here,
| * but it complicates things a lot. Instead
| * lets just do what we already stored, and
| * we'll get it the next time we are called.
| */
| if (error == AOP_TRUNCATED_PAGE)
| error = 0;
This may also cause similar issues as the invalidatation race. I'd
think it would be better not to be sloppy here.
|
| break;
| }
| }
| fill_it:
Miklos
On Wed, Jun 25 2008, Miklos Szeredi wrote:
> | error = 0;
> | while (spd.nr_pages < nr_pages) {
> | /*
> | * Page could be there, find_get_pages_contig() breaks on
> | * the first hole.
> | */
> | page = find_get_page(mapping, index);
> | if (!page) {
> | /*
> | * page didn't exist, allocate one.
> | */
> | page = page_cache_alloc_cold(mapping);
> | if (!page)
>
> error = -ENOMEM?
Looks like it, indeed.
> | break;
> |
> | error = add_to_page_cache_lru(page, mapping, index,
> | mapping_gfp_mask(mapping));
> | if (unlikely(error)) {
> | page_cache_release(page);
> | if (error == -EEXIST)
>
> error = 0? It may not matter, but leaving error as EEXIST is
> confusing at best (and coupled with the above missing ENOMEM could
> result in really weird errors for splice() ;).
Lets move the error assignment inside that loop, ala
- error = 0;
while (spd.nr_pages < nr_pages) {
+ error = 0;
But yes, that also looks like a bug. Good spotting!
> |
> | /*
> | * need to read in the page
> | */
> | error = mapping->a_ops->readpage(in, page);
> | if (unlikely(error)) {
> | /*
> | * We really should re-lookup the page here,
> | * but it complicates things a lot. Instead
> | * lets just do what we already stored, and
> | * we'll get it the next time we are called.
> | */
> | if (error == AOP_TRUNCATED_PAGE)
> | error = 0;
>
> This may also cause similar issues as the invalidatation race. I'd
> think it would be better not to be sloppy here.
Perhaps we can abstract that bit out into a small helper function, tied
in with your previous patch.
--
Jens Axboe
> Perhaps we can abstract that bit out into a small helper function, tied
> in with your previous patch.
If you don't mind, I'd leave this to you. I don't have the means (and
time) to test these changes, and anyway my preferred solution to all
known and unknown problems of generic_file_splice_read() would be to
move it to do_generic_file_read(), which you and Linus unfortunately
don't like :/
Miklos