On Mon, Sep 11, 2017 at 09:07:13PM +0100, Al Viro wrote: > On Mon, Sep 11, 2017 at 04:44:40PM +1000, Dave Chinner wrote: > > > > iov_iter_get_pages() for pipe-backed destination does page allocation > > > and inserts freshly allocated pages into pipe. > > > > Oh, it's hidden more layers down than the code implied I needed to > > look. > > > > i.e. there's no obvious clue in the function names that there is > > allocation happening in these paths (get_pipe_pages -> > > __get_pipe_pages -> push_pipe -> page allocation). The function > > names imply it's getting a reference to pages (like > > (get_user_pages()) and the fact it does allocation is inconsistent > > with it's naming. Worse, when push_pipe() fails to allocate pages, > > the error __get_pipe_pages() returns is -EFAULT, which further hides > > the fact push_pipe() does memory allocation that can fail.... > > > > And then there's the companion interface that implies page > > allocation: pipe_get_pages_alloc(). Which brings me back to there > > being no obvious clue while reading the code from the top down that > > pages are being allocated in push_pipe().... > > > > Comments and documentation for this code would help, but I can't > > find any of that, either. Hence I assumed naming followed familiar > > patterns and so mistook these interfaces being one that does page > > allocation and the other for getting references to pre-existing > > pages..... > > _NONE_ of those is a public interface - they are all static, to start > with. It still requires comments to explain *why* the code is doing what it's doing. You wrote the code, the whole explaination of it is in your head. I can't see any of that, so when I read the code I sit there thinking "why the fuck is it doing this?" because there's no explanations of the WTF? moments in the code... > The whole damn point is to have normal ->read_iter() work for read-to-pipe > without changes. That's why -EFAULT as error (rather than some other > mechanism for saying that pipe is full), etc. ... like this one. That needs a *fucking big comment* because it's not at all obvious why ENOMEM conditions are being hidden with EFAULT. Comments and documentation are not for the person who writes the code - they are for the stupid morons like me that need all the help they can get to understand complex code that does tricksy, subtle, non-obvious shit to work correctly. > Filesystem should *not* be changed to use that. At all. As far as it is > concerned, > copy_to_iter() > copy_page_to_iter() > iov_iter_get_pages() > iov_iter_get_pages_alloc() > iov_iter_advance() > are black boxes. The implementation may be a black box, but the operations the black box is performing for the callers still needs to be explained. > Note that one of the bugs there applies to normal read() as well - if you > are reading from a hole in file into an array with a read-only page in > the middle, you want a short read. And there's another WTF? moment..... How do we get a read only page in the middle of an array of pages we've been told to write data into? And why isn't that a bug in the code that supplied us with those pages? > Ignoring return value from iov_iter_zero() > is wrong for iovec-backed case as well as for pipes. > > Another one manages to work for iovec-backed case, albeit with rather odd > resulting semantics. readv(2) is underspecified (to put it politely) enough > for compliance, but it's still bloody strange. Namely, if you have a contiguous > 50Mb chunk of file on disk and run into e.g. a failure to fault the destination > pages in halfway through that extent, you act as if *nothing* in the damn thing > had been read, nevermind that 25Mb had been actually already read and that had > there been a discontinuity 5Mb prior, the first 20Mb would've been reported > read just fine. > > Strictly speaking that behaviour doesn't violate POSIX. This is direct IO. POSIX compliant behaviour went out the window long ago..... :/ > It is, however, > atrocious from the QoI standpoint, and for no good reason whatsoever. > It's quite easy to do better, and doing so would've eliminated the problems > in pipe-backed case as well (see below). In addition to that, I would > consider teaching bio_iov_iter_get_pages() to take the maximal bio size > as an explict argument. That would've killed the need of copying the > iterator and calling iov_iter_advance() in iomap_dio_actor() at all. > Anyway, the minimal candidate fix follows; it won't do anything about > the WARN_ON() in there, seeing that those are deliberate "program is > doing something bogus" things, but it should eliminate all crap with > ->splice_read() misreporting the amount of data it has copied. I'll run your updated patch through my testing, but seeing as I have nothing that tests splice+direct IO I'm not going to be able to test that right now. I have slightly more important things to that need urgent attention than writing splice+DIO test cases.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
