[LTP] [PATCH] vfs: fix readahead(2) on block devices

[Christian Brauner]

On Mon, Sep 25, 2023 at 12:43:42PM +0300, Amir Goldstein wrote:
> On Mon, Sep 25, 2023 at 9:42 AM Matthew Wilcox <willy@infradead.org> 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.

My level of confusion is rather high at the moment.
I'll leave the readahead fix in vfs.misc (In fact, I just rebased it on
top everytime I picked up a patch so as to not invalidate the whole tree
when it changes.) and then please send the preparatory fix. Don't resend
the readahead fix if nothing has changed.