Re: [PATCH] splice: add support for SPLICE_F_MOVE flag

Re: [PATCH] splice: add support for SPLICE_F_MOVE flag

[Andrew Morton]

Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
>
> commit 5abc97aa25b2c41413b3a520faee83f2282d9f18
> tree 4ba13ae0e91f15d02986df7cdca5e9455212d7d4
> parent 5274f052e7b3dbd81935772eb551dfd0325dfa9d
> author Jens Axboe <axboe@suse.de> Thu, 30 Mar 2006 15:16:46 +0200
> committer Linus Torvalds <torvalds@g5.osdl.org> Fri, 31 Mar 2006 04:28:18 -0800
> 
> [PATCH] splice: add support for SPLICE_F_MOVE flag
> 
> This enables the caller to migrate pages from one address space page
> cache to another.  In buzz word marketing, you can do zero-copy file
> copies!
> 
> ...
>  
> +static int page_cache_pipe_buf_steal(struct pipe_inode_info *info,
> +				     struct pipe_buffer *buf)
> +{
> +	struct page *page = buf->page;
> +
> +	WARN_ON(!PageLocked(page));
> +	WARN_ON(!PageUptodate(page));
> +
> +	if (!remove_mapping(page_mapping(page), page))
> +		return 1;
> +
> +	if (PageLRU(page)) {
> +		struct zone *zone = page_zone(page);
> +
> +		spin_lock_irq(&zone->lru_lock);
> +		BUG_ON(!PageLRU(page));
> +		__ClearPageLRU(page);
> +		del_page_from_lru(zone, page);
> +		spin_unlock_irq(&zone->lru_lock);
> +	}
> +
> +	buf->stolen = 1;
> +	return 0;
> +}

hm.  There's a reason why it is no longer necessary to recheck PG_lru after
taking the zone->lock, but I'm too lazy to go back through the changelogs
and we seem to have forgotten to add comments, so I'll cc Nick instead ;)

I worry that the page might still be under writeback when we get here. 
Probably we'll get lucky because whoever is writing the page probably holds
a ref on it (BIOs will do this), but a wait_on_page_writeback() after
locking the page might be prudent.

>  static void page_cache_pipe_buf_unmap(struct pipe_inode_info *info,
>  				      struct pipe_buffer *buf)
>  {
> -	unlock_page(buf->page);
> +	if (!buf->stolen)
> +		unlock_page(buf->page);
>  	kunmap(buf->page);
>  }

There go our chances of ever getting rid of kmap().  Is it not feasible to
use atomic kmaps throughout this code?

Re: [PATCH] splice: add support for SPLICE_F_MOVE flag

[Andrew Morton]

Andrew Morton <akpm@osdl.org> wrote:
>
>   static void page_cache_pipe_buf_unmap(struct pipe_inode_info *info,
>  >  				      struct pipe_buffer *buf)
>  >  {
>  > -	unlock_page(buf->page);
>  > +	if (!buf->stolen)
>  > +		unlock_page(buf->page);
>  >  	kunmap(buf->page);
>  >  }
> 
>  There go our chances of ever getting rid of kmap().  Is it not feasible to
>  use atomic kmaps throughout this code?

What are the kmaps for, anyway?  afaict they're doing the
kmap-the-page-while-we-run-some-a_ops thing which ceased being a
requirement 3-4 years ago.

The general approach we should take is that the code which actually
modifies a page's contents is the code which is responsible for kmapping
that page.  Use an atomic kmap, memcpy-or-memset, atomic kunmap.  Just four
or five lines.

If we can do that, pipe_buf_operations.map/unmap can be removed.

Re: [PATCH] splice: add support for SPLICE_F_MOVE flag

[Jens Axboe]

On Thu, Mar 30 2006, Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> >
> >   static void page_cache_pipe_buf_unmap(struct pipe_inode_info *info,
> >  >  				      struct pipe_buffer *buf)
> >  >  {
> >  > -	unlock_page(buf->page);
> >  > +	if (!buf->stolen)
> >  > +		unlock_page(buf->page);
> >  >  	kunmap(buf->page);
> >  >  }
> > 
> >  There go our chances of ever getting rid of kmap().  Is it not feasible to
> >  use atomic kmaps throughout this code?
> 
> What are the kmaps for, anyway?  afaict they're doing the
> kmap-the-page-while-we-run-some-a_ops thing which ceased being a
> requirement 3-4 years ago.

Not quite so, it's because the ->map serve as both a pin and address map
helper. The splice stuff doesn't actually need the kmap(), only for the
case where we have to copy the page. And it would be just as easy to
map that ourselves in that case.

There's a comment about that in the splice file.

> The general approach we should take is that the code which actually
> modifies a page's contents is the code which is responsible for kmapping
> that page.  Use an atomic kmap, memcpy-or-memset, atomic kunmap.  Just four
> or five lines.
> 
> If we can do that, pipe_buf_operations.map/unmap can be removed.

The page cache stuff still needs to sanity check the page, before
allowing the caller to map it, so it cannot go. But it can be optimized.

-- 
Jens Axboe