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

[Amir Goldstein <amir73il@gmail.com>]

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.

Thanks,
Amir.