diff mbox series

pipe: read/write_iter() handler should check for IOCB_NOWAIT

Message ID 273d8294-2508-a4c2-f96e-a6a394f94166@kernel.dk (mailing list archive)
State New, archived
Headers show
Series pipe: read/write_iter() handler should check for IOCB_NOWAIT | expand

Commit Message

Jens Axboe April 30, 2020, 4:24 p.m. UTC 
Pipe read/write only checks for the file O_NONBLOCK flag, but we should
also check for IOCB_NOWAIT for whether or not we should handle this read
or write in a non-blocking fashion. If we don't, then we will block on
data or space for iocbs that explicitly asked for non-blocking
operation. This messes up callers that explicitly ask for non-blocking
operations.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

Comments

Matthew Wilcox April 30, 2020, 5:58 p.m. UTC | #1 
On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
> also check for IOCB_NOWAIT for whether or not we should handle this read
> or write in a non-blocking fashion. If we don't, then we will block on
> data or space for iocbs that explicitly asked for non-blocking
> operation. This messes up callers that explicitly ask for non-blocking
> operations.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Wouldn't this be better?

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

diff --git a/fs/pipe.c b/fs/pipe.c
index 16fb72e9abf7..d4cf3ea9ad49 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -363,7 +363,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 			break;
 		if (ret)
 			break;
-		if (filp->f_flags & O_NONBLOCK) {
+		if (iocb->ki_flags & IOCB_NOWAIT) {
 			ret = -EAGAIN;
 			break;
 		}
@@ -566,7 +566,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 			continue;
 
 		/* Wait for buffer space to become available. */
-		if (filp->f_flags & O_NONBLOCK) {
+		if (iocb->ki_flags & IOCB_NOWAIT) {
 			if (!ret)
 				ret = -EAGAIN;
 			break;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4f6f59b4f22a..2790c956bd4f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3429,6 +3429,8 @@ static inline int iocb_flags(struct file *file)
 		res |= IOCB_DSYNC;
 	if (file->f_flags & __O_SYNC)
 		res |= IOCB_SYNC;
+	if (file->f_flags & O_NONBLOCK)
+		res |= IOCB_NOWAIT;
 	return res;
 }
Jens Axboe April 30, 2020, 6:47 p.m. UTC | #2 
On 4/30/20 11:58 AM, Matthew Wilcox wrote:
> On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
>> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
>> also check for IOCB_NOWAIT for whether or not we should handle this read
>> or write in a non-blocking fashion. If we don't, then we will block on
>> data or space for iocbs that explicitly asked for non-blocking
>> operation. This messes up callers that explicitly ask for non-blocking
>> operations.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> Wouldn't this be better?

Yeah, that's probably a better idea. Care to send a "proper" patch?
Jens Axboe April 30, 2020, 7:51 p.m. UTC | #3 
On 4/30/20 12:47 PM, Jens Axboe wrote:
> On 4/30/20 11:58 AM, Matthew Wilcox wrote:
>> On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
>>> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
>>> also check for IOCB_NOWAIT for whether or not we should handle this read
>>> or write in a non-blocking fashion. If we don't, then we will block on
>>> data or space for iocbs that explicitly asked for non-blocking
>>> operation. This messes up callers that explicitly ask for non-blocking
>>> operations.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> Wouldn't this be better?
> 
> Yeah, that's probably a better idea. Care to send a "proper" patch?

I take that back, running into issues going with a whole-sale conversion
like that:

mkdir("/run/dhcpcd", 0755)              = -1 EEXIST (File exists)
openat(AT_FDCWD, "/run/dhcpcd/ens7.pid", O_WRONLY|O_CREAT|O_NONBLOCK|O_CLOEXEC, 0644) = 4
flock(4, LOCK_EX|LOCK_NB)               = 0
getpid()                                = 214
ftruncate(4, 0)                         = 0
lseek(4, 0, SEEK_SET)                   = 0
fstat(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
lseek(4, 0, SEEK_CUR)                   = 0
write(4, "214\n", 4)                    = -1 EINVAL (Invalid argument)

which I don't know where is coming from yet, but it's definitely
breakage by auto setting IOCB_NOWAIT if O_NONBLOCK is set.

I'd prefer to go your route, but I also would like this fixed for pipes
for 5.7. So I'd suggest we go with mine, and then investigate why this
is breaking stuff and go with the all-in approach for 5.8 if feasible.
Jens Axboe April 30, 2020, 7:56 p.m. UTC | #4 
On 4/30/20 1:51 PM, Jens Axboe wrote:
> On 4/30/20 12:47 PM, Jens Axboe wrote:
>> On 4/30/20 11:58 AM, Matthew Wilcox wrote:
>>> On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
>>>> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
>>>> also check for IOCB_NOWAIT for whether or not we should handle this read
>>>> or write in a non-blocking fashion. If we don't, then we will block on
>>>> data or space for iocbs that explicitly asked for non-blocking
>>>> operation. This messes up callers that explicitly ask for non-blocking
>>>> operations.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>
>>> Wouldn't this be better?
>>
>> Yeah, that's probably a better idea. Care to send a "proper" patch?
> 
> I take that back, running into issues going with a whole-sale conversion
> like that:
> 
> mkdir("/run/dhcpcd", 0755)              = -1 EEXIST (File exists)
> openat(AT_FDCWD, "/run/dhcpcd/ens7.pid", O_WRONLY|O_CREAT|O_NONBLOCK|O_CLOEXEC, 0644) = 4
> flock(4, LOCK_EX|LOCK_NB)               = 0
> getpid()                                = 214
> ftruncate(4, 0)                         = 0
> lseek(4, 0, SEEK_SET)                   = 0
> fstat(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
> lseek(4, 0, SEEK_CUR)                   = 0
> write(4, "214\n", 4)                    = -1 EINVAL (Invalid argument)
> 
> which I don't know where is coming from yet, but it's definitely
> breakage by auto setting IOCB_NOWAIT if O_NONBLOCK is set.
> 
> I'd prefer to go your route, but I also would like this fixed for pipes
> for 5.7. So I'd suggest we go with mine, and then investigate why this
> is breaking stuff and go with the all-in approach for 5.8 if feasible.

OK, it's the old classic in generic_write_checks(), is my guess:

        if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT))  
                return -EINVAL;

so we definitely can't just flip the switch on O_NONBLOCK -> IOCB_NOWAIT
in general, at least not for writes.
Al Viro May 1, 2020, 3:58 a.m. UTC | #5 
On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
> also check for IOCB_NOWAIT for whether or not we should handle this read
> or write in a non-blocking fashion. If we don't, then we will block on
> data or space for iocbs that explicitly asked for non-blocking
> operation. This messes up callers that explicitly ask for non-blocking
> operations.

Why does io_uring allow setting IOCB_NOWAIT without FMODE_NOWAIT, anyway?
Jens Axboe May 1, 2020, 4:14 a.m. UTC | #6 
On 4/30/20 9:58 PM, Al Viro wrote:
> On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
>> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
>> also check for IOCB_NOWAIT for whether or not we should handle this read
>> or write in a non-blocking fashion. If we don't, then we will block on
>> data or space for iocbs that explicitly asked for non-blocking
>> operation. This messes up callers that explicitly ask for non-blocking
>> operations.
> 
> Why does io_uring allow setting IOCB_NOWAIT without FMODE_NOWAIT, anyway?

To do per-io non-blocking operations. It's not practical or convenient
to flip the file flag, nor does it work if you have multiple of them
going. If pipes honor the flag for the read/write iter handlers, then
we can handle them a lot more efficiently instead of requiring async
offload.
Jens Axboe May 1, 2020, 4:21 a.m. UTC | #7 
On 4/30/20 10:14 PM, Jens Axboe wrote:
> On 4/30/20 9:58 PM, Al Viro wrote:
>> On Thu, Apr 30, 2020 at 10:24:46AM -0600, Jens Axboe wrote:
>>> Pipe read/write only checks for the file O_NONBLOCK flag, but we should
>>> also check for IOCB_NOWAIT for whether or not we should handle this read
>>> or write in a non-blocking fashion. If we don't, then we will block on
>>> data or space for iocbs that explicitly asked for non-blocking
>>> operation. This messes up callers that explicitly ask for non-blocking
>>> operations.
>>
>> Why does io_uring allow setting IOCB_NOWAIT without FMODE_NOWAIT, anyway?
> 
> To do per-io non-blocking operations. It's not practical or convenient
> to flip the file flag, nor does it work if you have multiple of them
> going. If pipes honor the flag for the read/write iter handlers, then
> we can handle them a lot more efficiently instead of requiring async
> offload.

Sorry, I think I misread you and the answer, while correct by itself, is
not the answer to the question you are asking. You're saying we should
not be using IOCB_NOWAIT if FMODE_NOWAIT isn't set, which is fair. I'll
re-do the patch, we can probably just use FMODE_NOWAIT for the final
check in io_uring instead.

For pipes, we should include the setting of FMODE_NOWAIT for fifo_open()
with the patch, at least.
diff mbox series

Patch

diff --git a/fs/pipe.c b/fs/pipe.c
index 16fb72e9abf7..5711e6bca12d 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -363,7 +363,8 @@  pipe_read(struct kiocb *iocb, struct iov_iter *to)
 			break;
 		if (ret)
 			break;
-		if (filp->f_flags & O_NONBLOCK) {
+		if ((filp->f_flags & O_NONBLOCK) ||
+		    (iocb->ki_flags & IOCB_NOWAIT)) {
 			ret = -EAGAIN;
 			break;
 		}
@@ -566,7 +567,8 @@  pipe_write(struct kiocb *iocb, struct iov_iter *from)
 			continue;
 
 		/* Wait for buffer space to become available. */
-		if (filp->f_flags & O_NONBLOCK) {
+		if ((filp->f_flags & O_NONBLOCK) ||
+		    (iocb->ki_flags & IOCB_NOWAIT)) {
 			if (!ret)
 				ret = -EAGAIN;
 			break;