generic_file_splice_read() issues

generic_file_splice_read() issues

[Miklos Szeredi]

| 	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

generic_file_splice_read() issues

[Miklos Szeredi]

| 	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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: generic_file_splice_read() issues

[Jens Axboe]

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

Re: generic_file_splice_read() issues

[Jens Axboe]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: generic_file_splice_read() issues

[Miklos Szeredi]

> 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

Re: generic_file_splice_read() issues

[Miklos Szeredi]

> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>