Hi Linus,
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.
Please pull!
The following changes since commit 457391b0380335d5e9a5babdec90ac53928b23b4:
Linux 6.3 (2023-04-23 12:02:52 -0700)
are available in the Git repository at:
git://git.kernel.dk/linux.git tags/pipe-nonblock-2023-05-06
for you to fetch changes up to afed6271f5b0d78ca1a3739c1da4aa3629b26bba:
pipe: set FMODE_NOWAIT on pipes (2023-04-25 14:08:59 -0600)
----------------------------------------------------------------
pipe-nonblock-2023-05-06
----------------------------------------------------------------
Jens Axboe (2):
splice: clear FMODE_NOWAIT on file if splice/vmsplice is used
pipe: set FMODE_NOWAIT on pipes
fs/pipe.c | 3 +++
fs/splice.c | 34 ++++++++++++++++++++++++++++++----
2 files changed, 33 insertions(+), 4 deletions(-)
--
Jens Axboe
On Sat, May 6, 2023 at 3:33 AM Jens Axboe <[email protected]> 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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On 5/6/23 9:27?AM, Linus Torvalds wrote:
> On Sat, May 6, 2023 at 3:33?AM Jens Axboe <[email protected]> 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.
--
Jens Axboe
On Sat, May 6, 2023 at 3:28 PM Jens Axboe <[email protected]> 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.
--
Hilsen Harald