On Sat, Sep 23, 2023 at 5:41 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Sat, Sep 23, 2023 at 08:56:28AM +0300, Amir Goldstein wrote: > > We decided to deliberately try the change of behavior > > from EINVAL to ESPIPE, to align with fadvise behavior, > > so eventually the LTP test should be changed to allow both. > > > > It was the test failure on the socket that alarmed me. > > However, if we will have to special case socket in > > readahead() after all, we may as well also special case > > pipe with it and retain the EINVAL behavior - let's see > > what your findings are and decide. > > If I read it correctly, LTP is reporting that readhaead() on a socket > returned success instead of an error. Sockets do have a_ops, right? > It's set to empty_aops in inode_init_always, I think. > Yeh, you are right. I guess the check !f.file->f_mapping->a_ops is completely futile in that code. It's the only place I could find this sort of check except for places like: if (f->f_mapping->a_ops && f->f_mapping->a_ops->direct_IO) which just looks like a coding habit. > It would be nice if we documented somewhere which pointers should be > checked for NULL for which cases ... it doesn't really make sense for > a socket inode to have an i_mapping since it doesn't have pagecache. > But maybe we rely on i_mapping always being set. > I can't imagine that a socket has f_mapping. There must have been something off with this specific bug report, because it was reported on a WIP patch. > Irritatingly, POSIX specifies ESPIPE for pipes, but does not specify > what to do with sockets. It's kind of a meaningless syscall for > any kind of non-seekable fd. lseek() returns ESPIPE for sockets > as well as pipes, so I'd see this as an oversight. > https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fadvise.html > https://pubs.opengroup.org/onlinepubs/9699919799/functions/lseek.html > Indeed, we thought it wouldn't be too bad to align the readahead errors with those of posix_fadvise. That's why we asked to remove the S_ISFIFO check for v2. But looking again, pipe will get EINVAL for !f_mapping, so the UAPI wasn't changed at all and we were just talking BS all along. Let's leave the UAPI as is. > Of course readahead() is a Linux-specific syscall, so we can do whatever > we want here, but I'm really tempted to just allow readahead() for > regular files and block devices. Hmm. Can we check FMODE_LSEEK > instead of (S_ISFILE || S_ISBLK)? I think the f_mapping check should be good. Reuben already said he could not reproduce the LTP failure with v2 that is on Christian's vfs.misc branch. The S_ISREG check I put in the Fixes commit was completely unexplained in the commit message and completely unneeded. Just removing it as was done in v2 is enough. However, v2 has this wrong comment in the commit message: "The change also means that readahead will return -ESPIPE on FIFO files instead of -EINVAL." We need to remove this comment and could also remove the unneeded !f.file->f_mapping->a_ops check while at it. Thanks, Amir.
