public inbox for [email protected]
 help / color / mirror / Atom feed
* [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 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

* 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 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

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