[lkml]   [2020]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 22/23] fs: default to generic_file_splice_read for files having ->read_iter
On Thu, Jul 30, 2020 at 04:08:26PM +0100, Al Viro wrote:
> > I think we need to fix that in the instances, as we really expect
> > ->splice_read to just work instead of the caller knowing what could
> > work and what might not.
> Er... generic_file_splice_read() is a library helper; the decision to use
> is up to the filesystem/driver/protocol in question, and so's making sure
> it's not used with ->read_iter() that isn't fit for it.

Yes, but.. The problem is that while right now generic_file_splice_read
is the only user of ITER_PIPE there is absolutely not guarantee that
it remains the only user. Having ->read_iter instances lingering that
can't deal with it is at best a mine field waiting for victims.

Fortunately I think the fix is pretty easy - remove the special pipe
zero copy optimization from copy_page_to_iter, and just have the
callers actually want it because they have pagecache or similar
refcountable pages use it explicitly for the ITER_PIPE case. That gives
us a safe default with an opt-in into the optimized variant. I'm
currently auditing all the users of for how it is used and that looks
pretty promising.

> Note that we *do* have instances where we have different ->splice_read()
> (sometimes using generic_file_splice_read(), sometimes not) even though
> ->read_iter() is there.
> Your patch ignores those (thankfully), but commit message is rather
> misleading - it strongly implies that generic_file_splice_read() is
> *always* the right thing when ->read_iter() is there, not just that
> in such cases it makes a better fallback than default_file_splice_read().

I don't think it always is right. Not without a major audit and more
work at least.

 \ /
  Last update: 2020-07-30 17:21    [W:0.088 / U:10.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site