On Mon, Sep 25, 2023 at 6:36 PM Reuben Hawkins <reubenhwk@xxxxxxxxx> wrote: > > > > On Mon, Sep 25, 2023 at 4:43 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> >> On Mon, Sep 25, 2023 at 9:42 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.
