Message ID | 26aba1b5-8393-a20a-3ce9-f82425673f4d@kernel.dk (mailing list archive) |
---|---|
State | Mainlined, archived |
Headers | show |
Series | [GIT,PULL] Pipe FMODE_NOWAIT support | expand |
On Sat, May 6, 2023 at 3:33 AM Jens Axboe <axboe@kernel.dk> wrote: > > Here's the revised edition of the FMODE_NOWAIT support for pipes, in > which we just flag it as such supporting FMODE_NOWAIT unconditionally, > but clear it if we ever end up using splice/vmsplice on the pipe. The > pipe read/write side is perfectly fine for nonblocking IO, however > splice and vmsplice can potentially wait for IO with the pipe lock held. Ok, pulled. It does strike me that one of the (few) users is the io_uring __io_file_supports_nowait() thing, and that thing is *disgusting*. Wouldn't it be much nicer if FMODE_NOWAIT ended up working automatically on a block file descriptor too? You did all this "make pipes set FMODE_NOWAIT", but then that io_uring code does if (S_ISBLK(mode)) { if (IS_ENABLED(CONFIG_BLOCK) && io_bdev_nowait(I_BDEV(file->f_mapping->host))) return true; return false; } rather than just rely on FMODE_NOWAIT for block devices too... And it's a bit odd in other ways, because the kiocb code for RWF_NOWAIT - which is the other user of FMODE_NOWAIT - does *not* seem to do any of those bdev games. So that other user does seem to just rely on the file mode bit having been set up by open. In fact, I see 'blkdev_open()' doing filp->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC; so I really don't see why __io_file_supports_nowait() then does that special check for S_ISBLK(). Something is very rotten in the state of Denmark. But that's all independent of this pipe side, which now looks fine to me. Linus
The pull request you sent on Sat, 6 May 2023 04:33:17 -0600:
> git://git.kernel.dk/linux.git tags/pipe-nonblock-2023-05-06
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7644c8231987288e7aae378d2ff3c56a980d1988
Thank you!
On 5/6/23 9:27?AM, Linus Torvalds wrote: > On Sat, May 6, 2023 at 3:33?AM Jens Axboe <axboe@kernel.dk> wrote: >> >> Here's the revised edition of the FMODE_NOWAIT support for pipes, in >> which we just flag it as such supporting FMODE_NOWAIT unconditionally, >> but clear it if we ever end up using splice/vmsplice on the pipe. The >> pipe read/write side is perfectly fine for nonblocking IO, however >> splice and vmsplice can potentially wait for IO with the pipe lock held. > > Ok, pulled. > > It does strike me that one of the (few) users is the io_uring > __io_file_supports_nowait() thing, and that thing is *disgusting*. Hah yes, will not claim that's a thing of beauty. It has been getting better though, at least. > Wouldn't it be much nicer if FMODE_NOWAIT ended up working > automatically on a block file descriptor too? You did all this "make > pipes set FMODE_NOWAIT", but then that io_uring code does > > if (S_ISBLK(mode)) { > if (IS_ENABLED(CONFIG_BLOCK) && > io_bdev_nowait(I_BDEV(file->f_mapping->host))) > return true; > return false; > } > > rather than just rely on FMODE_NOWAIT for block devices too... > > And it's a bit odd in other ways, because the kiocb code for > RWF_NOWAIT - which is the other user of FMODE_NOWAIT - does *not* seem > to do any of those bdev games. So that other user does seem to just > rely on the file mode bit having been set up by open. > > In fact, I see 'blkdev_open()' doing > > filp->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC; > > so I really don't see why __io_file_supports_nowait() then does that > special check for S_ISBLK(). I need to double check if we can just do the io_bdev_nowait() check early and in the block code, so we cn make FMODE_NOWAIT reliable there. With that, yeah we could totally get rid of that odd checking and move it to the slower open path instead which would obviously be better. We should just set it for socket as well and just ultimately end up with: static bool __io_file_supports_nowait(struct file *file) { if (file->f_flags & O_NONBLOCK) return true; return file->f_mode & FMODE_NOWAIT; } and be done with it. Then we could also stop caching this state in the io_kiocb flags. > Something is very rotten in the state of Denmark. It's the Norwegians, always troublesome. > But that's all independent of this pipe side, which now looks fine to me. Thanks, I'll give the nowait side a once-over for the next kernel release and we can get that looking better too.
On Sat, May 6, 2023 at 3:28 PM Jens Axboe <axboe@kernel.dk> wrote: > > We should just set it for socket as well and just ultimately end up > with: > > static bool __io_file_supports_nowait(struct file *file) > { > if (file->f_flags & O_NONBLOCK) > return true; > return file->f_mode & FMODE_NOWAIT; > } Yup. > > Something is very rotten in the state of Denmark. > > It's the Norwegians, always troublesome. Something all Nordic people can agree on. Even the Norwegians, because they are easily confused. Linus
Linus Torvalds [07/05/2023 02.55]: >>> Something is very rotten in the state of Denmark. >> It's the Norwegians, always troublesome. > Something all Nordic people can agree on. Even the Norwegians, because > they are easily confused. You are all just jealous of our oil and gas. It's a result of being occupied first by the Danes from 1537 to 1814, then by the Swedes from 1814 to 1905. And the Finns moved through Sweden to Norway, where they burned down the forests in order to grow rye in the ashes ("svedjebruk" in the district of Finnskogen in eastern Norway). Actually, my family on my mother's side descend from some of these "skogfinner" ("forest Finns" for those on the list without language skills), as we call them.