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

[Amir Goldstein]

On Sun, Sep 24, 2023 at 6:48 AM Reuben Hawkins <reubenhwk@gmail.com> wrote:
>
>
>
> On Sat, Sep 23, 2023 at 10:48 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Sat, Sep 23, 2023 at 5:41 PM Matthew Wilcox <willy@infradead.org> 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.
>
>
> It looks to me like the following will fix the problem without breaking the tests...
>
> - if (!f.file->f_mapping || !f.file->f_mapping->a_ops ||
> -    !S_ISREG(file_inode(f.file)->i_mode))
> + if (!(f.file->f_mode & FMODE_LSEEK))
>
> ...I'll put this in a v3 patch soon unless somebody can spot any reasons why
> this is no good.

I am confused.
I thought you wrote that v2 did not break the test.
Why then is this change to use FMODE_LSEEK needed?
Why is it not enough to leave just
   if (!f.file->f_mapping)

Perhaps my comment to remove the unneeded
 !f.file->f_mapping->a_ops was misunderstood?

Also, in patch v3, you added RVB of Jan, but this is not the
version that Jan has reviewed.

Thanks,
Amir.