linux-kernel - Re: Unexpected splice "always copy" behavior observed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 19 May 2010 01:51:35 +1000
From:	Nick Piggin <npiggin@...e.de>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Pierre Tardy <tardyp@...il.com>, Ingo Molnar <mingo@...e.hu>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Tom Zanussi <tzanussi@...il.com>,
	Paul Mackerras <paulus@...ba.org>,
	linux-kernel@...r.kernel.org, arjan@...radead.org,
	ziga.mahkovec@...il.com, davem <davem@...emloft.net>,
	linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>, Jens Axboe <jens.axboe@...cle.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: Unexpected splice "always copy" behavior observed

Hi,

The basic problem is that the filesystem APIs were never designed with
this usage in mind, so we had to disable the SPLICE_F_MOVE support by
default.

So short answer is that this is expected.

What would be needed is to have filesystem maintainers go through and
enable it on a case by case basis. It's trivial for tmpfs/ramfs type
filesystems and I have a patch for those, but I never posted it on.yet.
Even basic buffer head filesystems IIRC get a little more complex --
but we may get some milage just out of invalidating the existing
pagecache rather than getting fancy and trying to move buffers over
to the new page.

Nick

On Tue, May 18, 2010 at 11:34:40AM -0400, Mathieu Desnoyers wrote:
> Hi,
> 
> I'm currently digging into the splice code to figure out why it's always in copy
> mode even though I specified the SPLICE_F_MOVE flag and released the page
> references from the LTTng ring buffer. I'm splicing to a pipe and then from the
> pipe to an ext3 filesystem (2.6.33.4 kernel). I've got the feeling I'm missing
> something and I don't like that.
> 
> My simple test case is to add a printk around the splice copy:
> 
> fs/splice.c: pipe_to_file()
>        if (buf->page != page) {
>                 /*
>                  * Careful, ->map() uses KM_USER0!
>                  */
>                 char *src = buf->ops->map(pipe, buf, 1);
>                 char *dst = kmap_atomic(page, KM_USER1);
> 
>                 printk(KERN_WARNING "SPLICE COPY!!!\n");
>                 memcpy(dst + offset, src + buf->offset, this_len);
>                 flush_dcache_page(page);
>                 kunmap_atomic(dst, KM_USER1);
>                 buf->ops->unmap(pipe, buf, src);
>         }
> 
> I'll start with a disclaimer that I only recently improved my splice
> understanding, so AFAIU:
> 
> * pipe_to_file() allocates a struct page *page on its stack.
> 
> * It is passed, uninitialized, to
> 
>         ret = pagecache_write_begin(file, mapping, sd->pos, this_len,
>                                 AOP_FLAG_UNINTERRUPTIBLE, &page, &fsdata);
> 
>     that looks already odd to me, as I would expect pipe_to_file to populate
>     this page pointer with buf->page initially if the proper conditions are met.
> 
> * Looking at the ext2 and ext3 write_begin code, neither are using the pagep
>   parameter:
> 
>   ext2:
> 
> static int
> ext2_write_begin(struct file *file, struct address_space *mapping,
>                 loff_t pos, unsigned len, unsigned flags,
>                 struct page **pagep, void **fsdata)
> {
>         *pagep = NULL;
>         return __ext2_write_begin(file, mapping, pos, len, flags, pagep,fsdata);
> }
> 
> 
>   ext3:
> 
> static int ext3_write_begin(struct file *file, struct address_space *mapping,
>                                 loff_t pos, unsigned len, unsigned flags,
>                                 struct page **pagep, void **fsdata)
> {
>         struct page *page;
>         ....
> 
> retry:
>         page = grab_cache_page_write_begin(mapping, index, flags);
>         if (!page)
>                 return -ENOMEM;
>         *pagep = page;
> 
> * So, considering the test to check if the page content must be copied:
> 
>        if (buf->page != page) {
> 
>   how is it ever possible that buf->page == page ?
> 
> Thanks,
> 
> Mathieu
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/