On Sat, Sep 9, 2023 at 7:39 AM Reuben Hawkins <reubenhwk@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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.
