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

[Oliver Sang <oliver.sang@intel.com>]

hi Amir,

On Sun, Sep 24, 2023 at 06:32:30PM +0300, Amir Goldstein wrote:
> On Sun, Sep 24, 2023 at 5:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Sun, Sep 24, 2023 at 02:47:42PM +0300, Amir Goldstein wrote:
> > > Since you joined the discussion, you have the opportunity to agree or
> > > disagree with our decision to change readahead() to ESPIPE.
> > > Judging  by your citing of lseek and posix_fadvise standard,
> > > I assume that you will be on board?
> >
> > I'm fine with returning ESPIPE (it's like ENOTTY in a sense).  but
> > that's not what kbuild reported:
> 
> kbuild report is from v1 patch that was posted to the list
> this is not the patch (v2) that is applied to vfs.misc
> and has been in linux-next for a few days.
> 
> Oliver,
> 
> Can you say the failure (on socket) is reproduced on
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc?
> 
> I would expect the pipe test to fail for getting ESPIPE
> but according to Reuben the socket test does not fail.

I tested on this commit:
15d4000b93539 (brauner-vfs/vfs.misc) vfs: fix readahead(2) on block devices

below is the test output:

<<<test_output>>>
tst_test.c:1558: TINFO: Timeout per run is 0h 02m 30s
readahead01.c:36: TINFO: test_bad_fd -1
readahead01.c:37: TPASS: readahead(-1, 0, getpagesize()) : EBADF (9)
readahead01.c:39: TINFO: test_bad_fd O_WRONLY
readahead01.c:45: TPASS: readahead(fd, 0, getpagesize()) : EBADF (9)
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

Summary:
passed   2
failed   2
broken   0
skipped  0
warnings 0


BTW, I noticed the branch updated, now:
e9168b6800ecd (brauner-vfs/vfs.misc) vfs: fix readahead(2) on block devices

though the patch-id are same. do you want us to test it again?

> 
> >
> > readahead01.c:62: TFAIL: readahead(fd[0], 0, getpagesize()) succeeded
> >
> > 61:     fd[0] = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
> > 62:     TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
> >
> > I think LTP would report 'wrong error code' rather than 'succeeded'
> > if it were returning ESPIPE.
> >
> > I'm not OK with readahead() succeeding on a socket.
> 
> Agree. Reuben reported that this does not happen on v2
> although I cannot explain why it was reported on v1...
> 
> > I think that should
> > also return ESPIPE.  I think posix_fadvise() should return ESPIPE on a
> > socket too, but reporting bugs to the Austin Group seems quite painful.
> > Perhaps somebody has been through this process and can do that for us?
> >
> 
> This is Reuben's first kernel patch.
> Let's agree that changing the standard of posix_fadvise() for socket is
> beyond the scope of his contribution :)
> 
> Thanks,
> Amir.
>