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

[Reuben Hawkins]

On Mon, Sep 25, 2023 at 4:43 AM Amir Goldstein <amir73il@gmail.com> 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.
>

I'm having a bit of a time coming up with a commit message for this
change to fadvise...It just doesn't sound like something I would want
to merge...

"Change fadvise to return -ESPIPE for sockets.  This is a new failure
mode that didn't previously exist.  Applications _may_ have to add new
error handling logic to accommodate the new return value.  It needs to
be fixed in fadvise so that readahead will also return new/unexpected
error codes."

It just doesn't feel right.  Nonetheless, here's the test results with
the fadvise change + the v2 readahead patch...

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()) expected
EINVAL: ESPIPE (29)

It seems to me like I fixed something in readahead that once worked,
readahead on block devices, and I'm now exchanging that once working
behavior to a new failure to socket, which previously succeeded...even
if it didn't do anything.

Should I instead just check for S_ISSOCK in readahead so that both pipes
and sockets will return EINVAL in readahead, and leave fadvise as is?


>
> 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.
>