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

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

[Reuben Hawkins]

Readahead was factored to call generic_fadvise.  That refactor broke
readahead on block devices.

The fix is to check F_ISFIFO rather than F_ISREG.  It would also work to
not check and let generic_fadvise to do the checking, but then the
generic_fadvise return value would have to be checked and changed from
-ESPIPE to -EINVAL to comply with the readahead(2) man-pages.

Fixes: 3d8f7615319b ("vfs: implement readahead(2) using POSIX_FADV_WILLNEED")
Signed-off-by: Reuben Hawkins <reubenhwk@gmail.com>
---
 mm/readahead.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 47afbca1d122..877ddcb61c76 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -749,7 +749,7 @@ ssize_t ksys_readahead(int fd, loff_t offset, size_t count)
 	 */
 	ret = -EINVAL;
 	if (!f.file->f_mapping || !f.file->f_mapping->a_ops ||
-	    !S_ISREG(file_inode(f.file)->i_mode))
+	    S_ISFIFO(file_inode(f.file)->i_mode))
 		goto out;
 
 	ret = vfs_fadvise(f.file, offset, count, POSIX_FADV_WILLNEED);
-- 
2.34.1

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

[Amir Goldstein]

On Sat, Sep 9, 2023 at 7:39 AM Reuben Hawkins <reubenhwk@gmail.com> wrote:
>
> Readahead was factored to call generic_fadvise.  That refactor broke
> readahead on block devices.

More accurately: That refactor added a S_ISREG restriction to the syscall
that broke readahead on block devices.

>
> The fix is to check F_ISFIFO rather than F_ISREG.  It would also work to
> not check and let generic_fadvise to do the checking, but then the
> generic_fadvise return value would have to be checked and changed from
> -ESPIPE to -EINVAL to comply with the readahead(2) man-pages.
>

We do not treat the man-pages as a holy script :)

It is quite likely that the code needs to change and the man-page will
also be changed to reflect the fact that ESPIPE is a possible return value.
In fact, see what the man page says about posix_fadvise(2):

       ESPIPE The specified file descriptor refers to a pipe or FIFO.
(ESPIPE is the error specified by POSIX, but before kernel version
2.6.16, Linux returned EINVAL in this case.)

My opinion is that we should drop the ISREG/ISFIFO altogether,
let the error code change to ESPIPE, and send a patch to man-pages
to reflect that change (after it was merged and released),
but I would like to hear what other people think.

> Fixes: 3d8f7615319b ("vfs: implement readahead(2) using POSIX_FADV_WILLNEED")
> Signed-off-by: Reuben Hawkins <reubenhwk@gmail.com>
> ---
>  mm/readahead.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 47afbca1d122..877ddcb61c76 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -749,7 +749,7 @@ ssize_t ksys_readahead(int fd, loff_t offset, size_t count)
>          */
>         ret = -EINVAL;
>         if (!f.file->f_mapping || !f.file->f_mapping->a_ops ||
> -           !S_ISREG(file_inode(f.file)->i_mode))
> +           S_ISFIFO(file_inode(f.file)->i_mode))

If this remains, it needs to be explained in the comment above
not only in the commit message, so developers reading the code
can understand the non obvious purpose.

Nice job with your first kernel patch Reuben :)

The process now is to wait for other developers to weigh in
on the question at hand.

When there is consensus, you may send a v2 patch
(git format-patch -v2) with review comments addressed.
Before sending the patch you may add notes below the
"---" line that are relevant to the context of the review but
not for git log, most notably, it is useful to list in v2 the
Changes since v1.

Thanks,
Amir.

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

[Christian Brauner]

On Sat, Sep 09, 2023 at 09:36:10AM +0300, Amir Goldstein wrote:
> On Sat, Sep 9, 2023 at 7:39 AM Reuben Hawkins <reubenhwk@gmail.com> wrote:
> >
> > Readahead was factored to call generic_fadvise.  That refactor broke
> > readahead on block devices.
> 
> More accurately: That refactor added a S_ISREG restriction to the syscall
> that broke readahead on block devices.
> 
> >
> > The fix is to check F_ISFIFO rather than F_ISREG.  It would also work to
> > not check and let generic_fadvise to do the checking, but then the
> > generic_fadvise return value would have to be checked and changed from
> > -ESPIPE to -EINVAL to comply with the readahead(2) man-pages.
> >
> 
> We do not treat the man-pages as a holy script :)
> 
> It is quite likely that the code needs to change and the man-page will
> also be changed to reflect the fact that ESPIPE is a possible return value.
> In fact, see what the man page says about posix_fadvise(2):
> 
>        ESPIPE The specified file descriptor refers to a pipe or FIFO.
> (ESPIPE is the error specified by POSIX, but before kernel version
> 2.6.16, Linux returned EINVAL in this case.)
> 
> My opinion is that we should drop the ISREG/ISFIFO altogether,
> let the error code change to ESPIPE, and send a patch to man-pages
> to reflect that change (after it was merged and released),
> but I would like to hear what other people think.

Probably fine with the understanding that if we get regression reports
it needs to be reverted quickly and the two of you are around to take
care of that... ;) 

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

[Amir Goldstein]

On Sun, Sep 10, 2023 at 1:02 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sat, Sep 09, 2023 at 09:36:10AM +0300, Amir Goldstein wrote:
> > On Sat, Sep 9, 2023 at 7:39 AM Reuben Hawkins <reubenhwk@gmail.com> wrote:
> > >
> > > Readahead was factored to call generic_fadvise.  That refactor broke
> > > readahead on block devices.
> >
> > More accurately: That refactor added a S_ISREG restriction to the syscall
> > that broke readahead on block devices.
> >
> > >
> > > The fix is to check F_ISFIFO rather than F_ISREG.  It would also work to
> > > not check and let generic_fadvise to do the checking, but then the
> > > generic_fadvise return value would have to be checked and changed from
> > > -ESPIPE to -EINVAL to comply with the readahead(2) man-pages.
> > >
> >
> > We do not treat the man-pages as a holy script :)
> >
> > It is quite likely that the code needs to change and the man-page will
> > also be changed to reflect the fact that ESPIPE is a possible return value.
> > In fact, see what the man page says about posix_fadvise(2):
> >
> >        ESPIPE The specified file descriptor refers to a pipe or FIFO.
> > (ESPIPE is the error specified by POSIX, but before kernel version
> > 2.6.16, Linux returned EINVAL in this case.)
> >
> > My opinion is that we should drop the ISREG/ISFIFO altogether,
> > let the error code change to ESPIPE, and send a patch to man-pages
> > to reflect that change (after it was merged and released),
> > but I would like to hear what other people think.
>
> Probably fine with the understanding that if we get regression reports
> it needs to be reverted quickly and the two of you are around to take
> care of that... ;)

Sure. Hopefully, if there are regressions, they will be reported sooner
than 5 years, as this one was...

Reuben,

Please post v2 just removing the S_ISREG restriction and mention
the change minor of behavior in the commit message.

There is no need to change the comment in readahead code,
because the comment does not mention the S_ISREG restriction.

Thanks,
Amir.

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

[kernel test robot]

Hello,

kernel test robot noticed "ltp.readahead01.fail" on:

commit: f49a20c992d7fed16e04c4cfa40e9f28f18f81f7 ("[PATCH] vfs: fix readahead(2) on block devices")
url: https://github.com/intel-lab-lkp/linux/commits/Reuben-Hawkins/vfs-fix-readahead-2-on-block-devices/20230909-124349
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 32bf43e4efdb87e0f7e90ba3883e07b8522322ad
patch link: https://lore.kernel.org/all/20230909043806.3539-1-reubenhwk@gmail.com/
patch subject: [PATCH] vfs: fix readahead(2) on block devices

in testcase: ltp
version: ltp-x86_64-14c1f76-1_20230715
with following parameters:

	disk: 1HDD
	fs: ext4
	test: syscalls-00/readahead01



compiler: gcc-12
test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz (Ivy Bridge) with 8G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202309191018.68ec87d7-oliver.sang@intel.com



COMMAND:    /lkp/benchmarks/ltp/bin/ltp-pan   -e -S   -a 3917     -n 3917 -p -f /fs/sdb2/tmpdir/ltp-R8Bqhtsv5t/alltests -l /lkp/benchmarks/ltp/results/LTP_RUN_ON-2023_09_13-20h_17m_53s.log  -C /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.failed -T /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.tconf
LOG File: /lkp/benchmarks/ltp/results/LTP_RUN_ON-2023_09_13-20h_17m_53s.log
FAILED COMMAND File: /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.failed
TCONF COMMAND File: /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.tconf
Running tests.......
<<<test_start>>>
tag=readahead01 stime=1694636274
cmdline="readahead01"
contacts=""
analysis=exit
<<<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: TPASS: readahead(fd[0], 0, getpagesize()) : EINVAL (22)
readahead01.c:60: TINFO: test_invalid_fd socket
readahead01.c:62: TFAIL: readahead(fd[0], 0, getpagesize()) succeeded

Summary:
passed   3
failed   1
broken   0
skipped  0
warnings 0
incrementing stop
<<<execution_status>>>
initiation_status="ok"
duration=0 termination_type=exited termination_id=1 corefile=no
cutime=0 cstime=1
<<<test_end>>>
INFO: ltp-pan reported some tests FAIL
LTP Version: 20230516-75-g2e582e743

       ###############################################################

            Done executing testcases.
            LTP Version:  20230516-75-g2e582e743
       ###############################################################




The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230919/202309191018.68ec87d7-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Amir Goldstein]

On Tue, Sep 19, 2023 at 5:47 AM kernel test robot <oliver.sang@intel.com> wrote:
>
>
>
> Hello,
>
> kernel test robot noticed "ltp.readahead01.fail" on:
>
> commit: f49a20c992d7fed16e04c4cfa40e9f28f18f81f7 ("[PATCH] vfs: fix readahead(2) on block devices")
> url: https://github.com/intel-lab-lkp/linux/commits/Reuben-Hawkins/vfs-fix-readahead-2-on-block-devices/20230909-124349
> base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 32bf43e4efdb87e0f7e90ba3883e07b8522322ad
> patch link: https://lore.kernel.org/all/20230909043806.3539-1-reubenhwk@gmail.com/
> patch subject: [PATCH] vfs: fix readahead(2) on block devices
>
> in testcase: ltp
> version: ltp-x86_64-14c1f76-1_20230715
> with following parameters:
>
>         disk: 1HDD
>         fs: ext4
>         test: syscalls-00/readahead01
>
>
>
> compiler: gcc-12
> test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz (Ivy Bridge) with 8G memory
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202309191018.68ec87d7-oliver.sang@intel.com
>
>
>
> COMMAND:    /lkp/benchmarks/ltp/bin/ltp-pan   -e -S   -a 3917     -n 3917 -p -f /fs/sdb2/tmpdir/ltp-R8Bqhtsv5t/alltests -l /lkp/benchmarks/ltp/results/LTP_RUN_ON-2023_09_13-20h_17m_53s.log  -C /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.failed -T /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.tconf
> LOG File: /lkp/benchmarks/ltp/results/LTP_RUN_ON-2023_09_13-20h_17m_53s.log
> FAILED COMMAND File: /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.failed
> TCONF COMMAND File: /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.tconf
> Running tests.......
> <<<test_start>>>
> tag=readahead01 stime=1694636274
> cmdline="readahead01"
> contacts=""
> analysis=exit
> <<<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: TPASS: readahead(fd[0], 0, getpagesize()) : EINVAL (22)
> readahead01.c:60: TINFO: test_invalid_fd socket
> readahead01.c:62: TFAIL: readahead(fd[0], 0, getpagesize()) succeeded
>

Reuben,

This report is on an old version of your patch.
However:
1. LTP test readahead01 will need to be fixed to accept also ESPIPE
2. I am surprised that with the old patch readahead on socket did not
    fail. Does socket have aops?

Please try to run LTP test readahead01 on the patch that Christian queued
and see how it behaves and if anything needs to be fixed wrt sockets.

Thanks,
Amir.

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

[Amir Goldstein]

On Thu, Sep 21, 2023 at 4:01 PM Reuben Hawkins <reubenhwk@gmail.com> wrote:
>
>
> On Tue, Sep 19, 2023 at 3:43 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Tue, Sep 19, 2023 at 5:47 AM kernel test robot <oliver.sang@intel.com> wrote:
>> >
>> >
>> >
>> > Hello,
>> >
>> > kernel test robot noticed "ltp.readahead01.fail" on:
>> >
>> > commit: f49a20c992d7fed16e04c4cfa40e9f28f18f81f7 ("[PATCH] vfs: fix readahead(2) on block devices")
>> > url: https://github.com/intel-lab-lkp/linux/commits/Reuben-Hawkins/vfs-fix-readahead-2-on-block-devices/20230909-124349
>> > base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 32bf43e4efdb87e0f7e90ba3883e07b8522322ad
>> > patch link: https://lore.kernel.org/all/20230909043806.3539-1-reubenhwk@gmail.com/
>> > patch subject: [PATCH] vfs: fix readahead(2) on block devices
>> >
>> > in testcase: ltp
>> > version: ltp-x86_64-14c1f76-1_20230715
>> > with following parameters:
>> >
>> >         disk: 1HDD
>> >         fs: ext4
>> >         test: syscalls-00/readahead01
>> >
>> >
>> >
>> > compiler: gcc-12
>> > test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz (Ivy Bridge) with 8G memory
>> >
>> > (please refer to attached dmesg/kmsg for entire log/backtrace)
>> >
>> >
>> >
>> >
>> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> > the same patch/commit), kindly add following tags
>> > | Reported-by: kernel test robot <oliver.sang@intel.com>
>> > | Closes: https://lore.kernel.org/oe-lkp/202309191018.68ec87d7-oliver.sang@intel.com
>> >
>> >
>> >
>> > COMMAND:    /lkp/benchmarks/ltp/bin/ltp-pan   -e -S   -a 3917     -n 3917 -p -f /fs/sdb2/tmpdir/ltp-R8Bqhtsv5t/alltests -l /lkp/benchmarks/ltp/results/LTP_RUN_ON-2023_09_13-20h_17m_53s.log  -C /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.failed -T /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.tconf
>> > LOG File: /lkp/benchmarks/ltp/results/LTP_RUN_ON-2023_09_13-20h_17m_53s.log
>> > FAILED COMMAND File: /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.failed
>> > TCONF COMMAND File: /lkp/benchmarks/ltp/output/LTP_RUN_ON-2023_09_13-20h_17m_53s.tconf
>> > Running tests.......
>> > <<<test_start>>>
>> > tag=readahead01 stime=1694636274
>> > cmdline="readahead01"
>> > contacts=""
>> > analysis=exit
>> > <<<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: TPASS: readahead(fd[0], 0, getpagesize()) : EINVAL (22)
>> > readahead01.c:60: TINFO: test_invalid_fd socket
>> > readahead01.c:62: TFAIL: readahead(fd[0], 0, getpagesize()) succeeded
>> >
>>
>> Reuben,
>>
>> This report is on an old version of your patch.
>> However:
>> 1. LTP test readahead01 will need to be fixed to accept also ESPIPE
>> 2. I am surprised that with the old patch readahead on socket did not
>>     fail. Does socket have aops?
>>
>> Please try to run LTP test readahead01 on the patch that Christian queued
>> and see how it behaves and if anything needs to be fixed wrt sockets.
>>
>> Thanks,
>> Amir.
>
>
> ack.  Will try to test.  My Ubuntu 22.04 system wasn't able to find packages called
> for by the test case, so it'll take me a little while to figure out how to get the
> test case working...

Heh! you can write a small C program instead, you don't even need to
build the LTP test.

It is clear what the failed test is doing:

static void test_invalid_fd(void)
{
        int fd[2];

        tst_res(TINFO, "%s pipe", __func__);
        SAFE_PIPE(fd);
        TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
        SAFE_CLOSE(fd[0]);
        SAFE_CLOSE(fd[1]);

        tst_res(TINFO, "%s socket", __func__);
        fd[0] = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
        TST_EXP_FAIL(readahead(fd[0], 0, getpagesize()), EINVAL);
        SAFE_CLOSE(fd[0]);
}

The report claims that readahead on socket succeeds
and this is surprising.

Thanks,
Amir.

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

[Cyril Hrubis]

Hi!
> ack.  Will try to test.  My Ubuntu 22.04 system wasn't able to find
> packages called
> for by the test case, so it'll take me a little while to figure out how to
> get the
> test case working...

Huh? The test is a simple C binary you shouldn't need anything more
than:

$ git clone https://github.com/linux-test-project/ltp.git
$ cd ltp
$ make autotools
$ ./configure

$ cd testcases/kernel/syscalls/readahead
$ make
$ ./readahead01

And this is well described in the readme at:

https://github.com/linux-test-project/ltp/

And the packages required for the compilation are make, C compiler and
autotools nothing extraordinary.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Amir Goldstein]

On Fri, Sep 22, 2023 at 11:29 PM Reuben Hawkins <reubenhwk@gmail.com> wrote:
>
>
>
> On Fri, Sep 22, 2023 at 4:09 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>>
>> Hi!
>> > ack.  Will try to test.  My Ubuntu 22.04 system wasn't able to find
>> > packages called
>> > for by the test case, so it'll take me a little while to figure out how to
>> > get the
>> > test case working...
>>
>> Huh? The test is a simple C binary you shouldn't need anything more
>> than:
>>
>> $ git clone https://github.com/linux-test-project/ltp.git
>> $ cd ltp
>> $ make autotools
>> $ ./configure
>>
>> $ cd testcases/kernel/syscalls/readahead
>> $ make
>> $ ./readahead01
>>
>> And this is well described in the readme at:
>>
>> https://github.com/linux-test-project/ltp/
>>
>> And the packages required for the compilation are make, C compiler and
>> autotools nothing extraordinary.
>>
> Awesome.  That was simpler than whatever it was I was trying.  I've
> reproduced the failed test and will try a few variations on the patch.

Cool.

For people that were not following the patch review,
the goal is not to pass the existing test.

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.

Thanks,
Amir.

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

[Matthew Wilcox]

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.

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.

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

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)?

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

[Amir Goldstein]

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.

Re: [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.

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

[Amir Goldstein]

On Sun, Sep 24, 2023 at 9:46 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> 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)
>

Christian,

I cleared the confusion with Reuben off-list.

V2 on current vfs.misc is good as is, in the sense that it does
what we expected it to do - it breaks the LTP test for pipe because
the error value has changed from EINVAL to ESPIPE.
The error value for socket (EINVAL) has not changed.

Matthew,

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 think that the FMODE_LSEEK test would have been a good idea if
we wanted to preserve the EINVAL error code for pipe, but
I don't think that is the case?

Thanks,
Amir.

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

[Matthew Wilcox]

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:

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

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

[Amir Goldstein]

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.

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

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

[Matthew Wilcox]

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.

Ah!  I was confused.

> > 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 :)

Thank you for shepherding his first contribution.  Unfortunately, this
is rather the way of it when you start to pick at something ... you find
more things that are broken.  It's rather unusual that this one turned
out to be "The POSIX spec has a defect" ;-)

But yes, I'm content with v2 if v2 does in fact return ESPIPE for
readahead() on a socket.  Let's wait to find out.  We can address the
POSIX defect later.

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

[Matthew Wilcox]

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
;-)

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

[Amir Goldstein]

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.

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

[Christian Brauner]

On Mon, Sep 25, 2023 at 12:43:42PM +0300, Amir Goldstein 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.
> 
> 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.

My level of confusion is rather high at the moment.
I'll leave the readahead fix in vfs.misc (In fact, I just rebased it on
top everytime I picked up a patch so as to not invalidate the whole tree
when it changes.) and then please send the preparatory fix. Don't resend
the readahead fix if nothing has changed.

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

[Amir Goldstein]

On Mon, Sep 25, 2023 at 6:36 PM Reuben Hawkins <reubenhwk@gmail.com> wrote:
>
>
>
> 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?
>

What you are saying makes sense.
And if we are being honest, I think that the right thing to do from the
beginning was to separate the bug fix commit from the UAPI change.

The minimal bug fix is S_ISREG || S_ISBLK, which
mentions the Fixes commit and will be picked for stable kernels.

Following up with another one or two patches that change
the behavior of posix_fadvise on socket and readahead on
socket and pipe.

The UAPI change is not something that has to go to stable
and it should be easily revertable independently of the bug fix.
Doing it otherwise would make our lives much harder if regressions
turn up from the UAPI change.

Christian, Matthew,

Do you agree?

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

Reuben,

If there is agreement on the above, you may still get your chance
to send a patch set ;)

Thanks,
Amir.

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

[Christian Brauner]

> What you are saying makes sense.
> And if we are being honest, I think that the right thing to do from the
> beginning was to separate the bug fix commit from the UAPI change.
> 
> The minimal bug fix is S_ISREG || S_ISBLK, which
> mentions the Fixes commit and will be picked for stable kernels.
> 
> Following up with another one or two patches that change
> the behavior of posix_fadvise on socket and readahead on
> socket and pipe.
> 
> The UAPI change is not something that has to go to stable
> and it should be easily revertable independently of the bug fix.
> Doing it otherwise would make our lives much harder if regressions
> turn up from the UAPI change.
> 
> Christian, Matthew,
> 
> Do you agree?

Fine by me.

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

[Oliver Sang]

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

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

[Amir Goldstein]

On Tue, Sep 26, 2023 at 4:56 AM Oliver Sang <oliver.sang@intel.com> wrote:
>
> 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
>
>

Thank you!
We had some confusion about patch of reported bug vs. current patch,
but these results are matching the other reports wrt current patch.


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

It's the same patch. no need to re-test.

Thanks,
Amir.