On Sun, Sep 10, 2023 at 1:02 PM Christian Brauner <brauner@xxxxxxxxxx> 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@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. > > 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.
