Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] pipe: honor IOCB_NOWAIT
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <cedfa436-47a3-7cbc-1948-75d0e28cfdc5@kernel.dk>
 <20200911011205.GG1236603@ZenIV.linux.org.uk>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <719f095d-43fa-7e93-e8c8-bb22f2505501@kernel.dk>
Date:   Fri, 11 Sep 2020 05:21:59 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200911011205.GG1236603@ZenIV.linux.org.uk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 9/10/20 7:12 PM, Al Viro wrote:
> On Mon, Sep 07, 2020 at 09:21:02AM -0600, Jens Axboe wrote:
>> Pipe only looks at O_NONBLOCK for non-blocking operation, which means that
>> io_uring can't easily poll for it or attempt non-blocking issues. Check for
>> IOCB_NOWAIT in locking the pipe for reads and writes, and ditto when we
>> decide on whether or not to block or return -EAGAIN.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> If this is acceptable, then I can add S_ISFIFO to the whitelist on file
>> descriptors we can IOCB_NOWAIT try for, then poll if we get -EAGAIN
>> instead of using thread offload.
> 
> Will check.

Thanks!

> In the meanwhile, blacklist eventpoll again.  Because your
> attempts at "nonblocking" there had been both ugly as hell *AND* fail
> to prevent blocking.  And frankly, I'm very tempted to rip that crap
> out entirely.  Seriously, *look* at the code you've modified in
> do_epoll_ctl().  And tell me why the hell is grabbing ->mtx in that
> function needs to be infested with trylocks, while exact same mutex
> taken in loop_check_proc() called under those is fine with mutex_lock().
> Ditto for calls of vfs_poll() inside ep_insert(), GFP_KERNEL allocations
> in ep_ptable_queue_proc(), synchronize_rcu() callable from ep_modify()
> (from the same function), et sodding cetera.

Ugh missed the loop_check_proc() part :/

The original patch wasn't that pretty, in my defense the whole thing is
pretty ugly to begin with. It'd need serious rewriting to become
palatable. If you want to yank the patch, I've got no problem with that.

> No, this is _not_ an invitation to spread the same crap over even more
> places in there; I just want to understand where had that kind of voodoo
> approach comes from.  And that's directly relevant for this patch,
> because it looks like the same kind of thing.

The pipe patch is way cleaner, and pretty much to the point. Don't think
that's a fair comparison.

> What is your semantics for IOCB_NOWAIT?  What should and what should _not_
> be waited for?

Basically it's don't wait for space (if write) if it's full, don't wait
for data if it's empty. O_NONBLOCK for the operation, instead of as a
file state. The locking isn't strictly required, and it's basically
impossible to avoid various deeper down items like memory allocations
potentially blocking, so...

-- 
Jens Axboe