On Mon, Sep 25, 2023 at 9:42 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Sun, Sep 24, 2023 at 11:35:48PM -0500, Reuben Hawkins wrote: > > The v2 patch does NOT return ESPIPE on a socket. It succeeds. > > > > readahead01.c:54: TINFO: test_invalid_fd pipe > > readahead01.c:56: TFAIL: readahead(fd[0], 0, getpagesize()) expected > > EINVAL: ESPIPE (29) > > readahead01.c:60: TINFO: test_invalid_fd socket > > readahead01.c:62: TFAIL: readahead(fd[0], 0, getpagesize()) succeeded > > <-------here > > Thanks! I am of the view that this is wrong (although probably > harmless). I suspect what happens is that we take the > 'bdi == &noop_backing_dev_info' condition in generic_fadvise() > (since I don't see anywhere in net/ setting f_op->fadvise) and so > return 0 without doing any work. > > The correct solution is probably your v2, combined with: > > inode = file_inode(file); > - if (S_ISFIFO(inode->i_mode)) > + if (S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) > return -ESPIPE; > > in generic_fadvise(), but that then changes the return value from > posix_fadvise(), as I outlined in my previous email. And I'm OK with > that, because I think it's what POSIX intended. Amir may well disagree > ;-) I really have no problem with that change to posix_fadvise(). I only meant to say that we are not going to ask Reuben to talk to the standard committee, but that's obvious ;-) A patch to man-pages, that I would recommend as a follow up. FWIW, I checked and there is currently no test for posix_fadvise() on socket in LTP AFAIK. Maybe Cyril will follow your suggestion and this will add test coverage for socket in posix_fadvise(). Reuben, The actionable item, if all agree with Matthew's proposal, is not to change the v2 patch to readahead(), but to send a new patch for generic_fadvise(). When you send the patch to Christian, you should specify the dependency - it needs to be applied before the readahead patch. If the readahead patch was not already in the vfs tree, you would have needed to send a patch series with a cover letter, where you would leave the Reviewed-by on the unchanged [2/2] readahead patch. Sending a patch series is a good thing to practice, but it is not strictly needed in this case, so I'll leave it up to you to decide. Thanks, Amir.
