Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6be969d0-e941-c8fa-aca7-c6c96f2c1ba2@kernel.dk>
Date:   Sat, 6 May 2023 16:28:21 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [GIT PULL] Pipe FMODE_NOWAIT support
Content-Language: en-US
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <26aba1b5-8393-a20a-3ce9-f82425673f4d@kernel.dk>
 <CAHk-=wj=LwLDYrjHpMM+QnE2T+u4P9-UXhXGkAMXiyfGjGnNEA@mail.gmail.com>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <CAHk-=wj=LwLDYrjHpMM+QnE2T+u4P9-UXhXGkAMXiyfGjGnNEA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

-- 
Jens Axboe