* [GIT PULL] io_uring fix for 5.16-rc7 @ 2021-12-23 21:11 Jens Axboe 2021-12-23 23:39 ` Linus Torvalds 2021-12-23 23:46 ` pr-tracker-bot 0 siblings, 2 replies; 6+ messages in thread From: Jens Axboe @ 2021-12-23 21:11 UTC (permalink / raw) To: Linus Torvalds; +Cc: io-uring Hi Linus, Single fix for not clearing kiocb->ki_pos back to 0 for a stream, destined for stable as well. Please pull! The following changes since commit d800c65c2d4eccebb27ffb7808e842d5b533823c: io-wq: drop wqe lock before creating new worker (2021-12-13 09:04:01 -0700) are available in the Git repository at: git://git.kernel.dk/linux-block.git tags/io_uring-5.16-2021-12-23 for you to fetch changes up to 7b9762a5e8837b92a027d58d396a9d27f6440c36: io_uring: zero iocb->ki_pos for stream file types (2021-12-22 20:34:32 -0700) ---------------------------------------------------------------- io_uring-5.16-2021-12-23 ---------------------------------------------------------------- Jens Axboe (1): io_uring: zero iocb->ki_pos for stream file types fs/io_uring.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] io_uring fix for 5.16-rc7 2021-12-23 21:11 [GIT PULL] io_uring fix for 5.16-rc7 Jens Axboe @ 2021-12-23 23:39 ` Linus Torvalds 2021-12-23 23:43 ` Linus Torvalds 2021-12-23 23:49 ` Jens Axboe 2021-12-23 23:46 ` pr-tracker-bot 1 sibling, 2 replies; 6+ messages in thread From: Linus Torvalds @ 2021-12-23 23:39 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring On Thu, Dec 23, 2021 at 1:11 PM Jens Axboe <[email protected]> wrote: > > Single fix for not clearing kiocb->ki_pos back to 0 for a stream, > destined for stable as well. I don't think any of this is right. You can't use f_pos without using fdget_pos() to actually get the lock to it. Which you don't do in io_uring. Linus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] io_uring fix for 5.16-rc7 2021-12-23 23:39 ` Linus Torvalds @ 2021-12-23 23:43 ` Linus Torvalds 2021-12-24 0:55 ` Jens Axboe 2021-12-23 23:49 ` Jens Axboe 1 sibling, 1 reply; 6+ messages in thread From: Linus Torvalds @ 2021-12-23 23:43 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring On Thu, Dec 23, 2021 at 3:39 PM Linus Torvalds <[email protected]> wrote: > > I don't think any of this is right. > > You can't use f_pos without using fdget_pos() to actually get the lock to it. > > Which you don't do in io_uring. I've pulled it because it's still an improvement on what it used to be, but f_pos use in io_uring does seem very broken. Linus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] io_uring fix for 5.16-rc7 2021-12-23 23:43 ` Linus Torvalds @ 2021-12-24 0:55 ` Jens Axboe 0 siblings, 0 replies; 6+ messages in thread From: Jens Axboe @ 2021-12-24 0:55 UTC (permalink / raw) To: Linus Torvalds; +Cc: io-uring On 12/23/21 4:43 PM, Linus Torvalds wrote: > On Thu, Dec 23, 2021 at 3:39 PM Linus Torvalds > <[email protected]> wrote: >> >> I don't think any of this is right. >> >> You can't use f_pos without using fdget_pos() to actually get the lock to it. >> >> Which you don't do in io_uring. > > I've pulled it because it's still an improvement on what it used to > be, but f_pos use in io_uring does seem very broken. Totally untested, but something like this should make it saner. This grabs the lock (if needed) and position at issue time, rather than at prep time. We could potentially handle the unlock in the handler themselves too, but store the state in a flag and make it part of the slower path cleanup instead. That ensures we always end up performing it, even if the request is errored. Might be straight forward enough for 5.16 in fact, but considering it's not a new regression, deferring for 5.17 will be saner imho. diff --git a/fs/io_uring.c b/fs/io_uring.c index fb2a0cb4aaf8..a80f9f8f2cd0 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -112,7 +112,7 @@ #define IO_REQ_CLEAN_FLAGS (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP | \ REQ_F_POLLED | REQ_F_INFLIGHT | REQ_F_CREDS | \ - REQ_F_ASYNC_DATA) + REQ_F_ASYNC_DATA | REQ_F_CUR_POS_LOCK) #define IO_TCTX_REFS_CACHE_NR (1U << 10) @@ -726,6 +726,7 @@ enum { REQ_F_FAIL_BIT = 8, REQ_F_INFLIGHT_BIT, REQ_F_CUR_POS_BIT, + REQ_F_CUR_POS_LOCK_BIT, REQ_F_NOWAIT_BIT, REQ_F_LINK_TIMEOUT_BIT, REQ_F_NEED_CLEANUP_BIT, @@ -765,6 +766,8 @@ enum { REQ_F_INFLIGHT = BIT(REQ_F_INFLIGHT_BIT), /* read/write uses file position */ REQ_F_CUR_POS = BIT(REQ_F_CUR_POS_BIT), + /* request is holding file position lock */ + REQ_F_CUR_POS_LOCK = BIT(REQ_F_CUR_POS_LOCK_BIT), /* must not punt to workers */ REQ_F_NOWAIT = BIT(REQ_F_NOWAIT_BIT), /* has or had linked timeout */ @@ -2892,12 +2895,15 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) kiocb->ki_pos = READ_ONCE(sqe->off); if (kiocb->ki_pos == -1) { - if (!(file->f_mode & FMODE_STREAM)) { + /* + * If we end up using the current file position, just set the + * flag and the actual file position will be read in the op + * handler themselves. + */ + if (!(file->f_mode & FMODE_STREAM)) req->flags |= REQ_F_CUR_POS; - kiocb->ki_pos = file->f_pos; - } else { + else kiocb->ki_pos = 0; - } } kiocb->ki_flags = iocb_flags(file); ret = kiocb_set_rw_flags(kiocb, READ_ONCE(sqe->rw_flags)); @@ -3515,6 +3521,25 @@ static bool need_read_all(struct io_kiocb *req) S_ISBLK(file_inode(req->file)->i_mode); } +static bool io_file_pos_lock(struct io_kiocb *req, bool force_nonblock) +{ + struct file *file = req->file; + + if (!(req->flags & REQ_F_CUR_POS)) + return false; + if (file->f_mode & FMODE_ATOMIC_POS && file_count(file) > 1) { + if (force_nonblock) { + if (!mutex_trylock(&file->f_pos_lock)) + return true; + } else { + mutex_lock(&file->f_pos_lock); + } + req->flags |= REQ_F_CUR_POS_LOCK; + } + req->rw.kiocb.ki_pos = file->f_pos; + return false; +} + static int io_read(struct io_kiocb *req, unsigned int issue_flags) { struct io_rw_state __s, *s = &__s; @@ -3543,7 +3568,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) if (force_nonblock) { /* If the file doesn't support async, just async punt */ - if (unlikely(!io_file_supports_nowait(req))) { + if (unlikely(!io_file_supports_nowait(req) || + io_file_pos_lock(req, true))) { ret = io_setup_async_rw(req, iovec, s, true); return ret ?: -EAGAIN; } @@ -3551,6 +3577,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) } else { /* Ensure we clear previously set non-block flag */ kiocb->ki_flags &= ~IOCB_NOWAIT; + io_file_pos_lock(req, false); } ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), req->result); @@ -3668,7 +3695,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) if (force_nonblock) { /* If the file doesn't support async, just async punt */ - if (unlikely(!io_file_supports_nowait(req))) + if (unlikely(!io_file_supports_nowait(req) || + io_file_pos_lock(req, true))) goto copy_iov; /* file path doesn't support NOWAIT for non-direct_IO */ @@ -3680,6 +3708,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) } else { /* Ensure we clear previously set non-block flag */ kiocb->ki_flags &= ~IOCB_NOWAIT; + io_file_pos_lock(req, false); } ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), req->result); @@ -6584,6 +6613,8 @@ static void io_clean_op(struct io_kiocb *req) kfree(req->kbuf); req->kbuf = NULL; } + if (req->flags & REQ_F_CUR_POS_LOCK) + __f_unlock_pos(req->file); if (req->flags & REQ_F_NEED_CLEANUP) { switch (req->opcode) { -- Jens Axboe ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [GIT PULL] io_uring fix for 5.16-rc7 2021-12-23 23:39 ` Linus Torvalds 2021-12-23 23:43 ` Linus Torvalds @ 2021-12-23 23:49 ` Jens Axboe 1 sibling, 0 replies; 6+ messages in thread From: Jens Axboe @ 2021-12-23 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: io-uring On 12/23/21 4:39 PM, Linus Torvalds wrote: > On Thu, Dec 23, 2021 at 1:11 PM Jens Axboe <[email protected]> wrote: >> >> Single fix for not clearing kiocb->ki_pos back to 0 for a stream, >> destined for stable as well. > > I don't think any of this is right. > > You can't use f_pos without using fdget_pos() to actually get the lock > to it. > > Which you don't do in io_uring. Well, current position doesn't really make much sense to begin with in an async API, but there are various use cases that want to use it for sync IO. We do have the file at that point, but it's most certainly not sychronized across completions (or serialized with other fdget_pos() users). We could hold ->f_pos_lock over the duration of the IO however, that would probably be saner... As completions are always done from the task itself as well, that should work. I'll take a look at that for 5.17. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] io_uring fix for 5.16-rc7 2021-12-23 21:11 [GIT PULL] io_uring fix for 5.16-rc7 Jens Axboe 2021-12-23 23:39 ` Linus Torvalds @ 2021-12-23 23:46 ` pr-tracker-bot 1 sibling, 0 replies; 6+ messages in thread From: pr-tracker-bot @ 2021-12-23 23:46 UTC (permalink / raw) To: Jens Axboe; +Cc: Linus Torvalds, io-uring The pull request you sent on Thu, 23 Dec 2021 14:11:26 -0700: > git://git.kernel.dk/linux-block.git tags/io_uring-5.16-2021-12-23 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/a026fa5404316787c2104bec3f8ff506acf85b98 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-24 0:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-23 21:11 [GIT PULL] io_uring fix for 5.16-rc7 Jens Axboe 2021-12-23 23:39 ` Linus Torvalds 2021-12-23 23:43 ` Linus Torvalds 2021-12-24 0:55 ` Jens Axboe 2021-12-23 23:49 ` Jens Axboe 2021-12-23 23:46 ` pr-tracker-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox