Re: splice(SPLICE_F_MOVE) problems

[Oleg Nesterov <oleg@tv-sign.ru>]

I am looking at current fs/splice.c from
	http://kernel.org/git/?p=linux/kernel/git/torvalds/....


	pipe_to_file:

		if ((sd->flags & SPLICE_F_MOVE) && this_len == PAGE_CACHE_SIZE) {
			/*
			 * If steal succeeds, buf->page is now pruned from the vm
			 * side (page cache) and we can reuse it. The page will also
			 * be locked on successful return.
			 */
			if (buf->ops->steal(info, buf))
				goto find_page;

			page = buf->page;
			page_cache_get(page);

Isn't it better to do this after successful successful add_to_page_cache() ?
This way you don't need to do page_cache_release() if add_to_page_cache fails.

			/*
			 * page must be on the LRU for adding to the pagecache.

Is it true? (looking at add_to_page_cache_lru).

			 * Check this without grabbing the zone lock, if it isn't
			 * the do grab the zone lock, recheck, and add if necessary.
			 */
			if (!PageLRU(page)) {
				struct zone *zone = page_zone(page);

				spin_lock_irq(&zone->lru_lock);
				if (!PageLRU(page)) {
					SetPageLRU(page);
					add_page_to_inactive_list(zone, page);
				}
				spin_unlock_irq(&zone->lru_lock);

Why not lru_cache_add() ?


			if (add_to_page_cache(page, mapping, index, gfp_mask)) {
				page_cache_release(page);
				unlock_page(page);
				goto find_page;

This leaves unmapped page on ->inactive_list. page_count(page) == 1. What if
shrink_inactive_list() finds this page, increments page->count, and calls
shrink_page_list()->remove_mapping() again? Hmm... So page_cache_pipe_buf_steal()
has a reason to return a locked page (I am not an expert, please correct me).

However, user_page_pipe_buf_steal() returns unlocked page in PIPE_BUF_FLAG_GIFT
case. So, if add_to_page_cache() fails, unlock_page() will trigger BUG().


		ret = mapping->a_ops->prepare_write(file, page, offset, offset+this_len);
		if (ret == AOP_TRUNCATED_PAGE) {
			page_cache_release(page);
			goto find_page;

We also need to unlock(page) if it was stealed.

Oleg.