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