* [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT @ 2020-04-30 16:24 Jens Axboe 2020-04-30 17:58 ` Matthew Wilcox 2020-05-01 3:58 ` Al Viro 0 siblings, 2 replies; 8+ messages in thread From: Jens Axboe @ 2020-04-30 16:24 UTC (permalink / raw) To: Alexander Viro; +Cc: linux-fsdevel, linux-kernel 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> --- 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; -- Jens Axboe ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-04-30 16:24 [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT Jens Axboe @ 2020-04-30 17:58 ` Matthew Wilcox 2020-04-30 18:47 ` Jens Axboe 2020-05-01 3:58 ` Al Viro 1 sibling, 1 reply; 8+ messages in thread From: Matthew Wilcox @ 2020-04-30 17:58 UTC (permalink / raw) To: Jens Axboe; +Cc: Alexander Viro, linux-fsdevel, linux-kernel 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; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-04-30 17:58 ` Matthew Wilcox @ 2020-04-30 18:47 ` Jens Axboe 2020-04-30 19:51 ` Jens Axboe 0 siblings, 1 reply; 8+ messages in thread From: Jens Axboe @ 2020-04-30 18:47 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Alexander Viro, linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-04-30 18:47 ` Jens Axboe @ 2020-04-30 19:51 ` Jens Axboe 2020-04-30 19:56 ` Jens Axboe 0 siblings, 1 reply; 8+ messages in thread From: Jens Axboe @ 2020-04-30 19:51 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Alexander Viro, linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-04-30 19:51 ` Jens Axboe @ 2020-04-30 19:56 ` Jens Axboe 0 siblings, 0 replies; 8+ messages in thread From: Jens Axboe @ 2020-04-30 19:56 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Alexander Viro, linux-fsdevel, linux-kernel 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. -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-04-30 16:24 [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT Jens Axboe 2020-04-30 17:58 ` Matthew Wilcox @ 2020-05-01 3:58 ` Al Viro 2020-05-01 4:14 ` Jens Axboe 1 sibling, 1 reply; 8+ messages in thread From: Al Viro @ 2020-05-01 3:58 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-fsdevel, linux-kernel 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-05-01 3:58 ` Al Viro @ 2020-05-01 4:14 ` Jens Axboe 2020-05-01 4:21 ` Jens Axboe 0 siblings, 1 reply; 8+ messages in thread From: Jens Axboe @ 2020-05-01 4:14 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT 2020-05-01 4:14 ` Jens Axboe @ 2020-05-01 4:21 ` Jens Axboe 0 siblings, 0 replies; 8+ messages in thread From: Jens Axboe @ 2020-05-01 4:21 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, linux-kernel 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. -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-01 4:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-30 16:24 [PATCH] pipe: read/write_iter() handler should check for IOCB_NOWAIT Jens Axboe 2020-04-30 17:58 ` Matthew Wilcox 2020-04-30 18:47 ` Jens Axboe 2020-04-30 19:51 ` Jens Axboe 2020-04-30 19:56 ` Jens Axboe 2020-05-01 3:58 ` Al Viro 2020-05-01 4:14 ` Jens Axboe 2020-05-01 4:21 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).