Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Dylan Yudaken <dylany@fb.com>
To:     Jens Axboe <axboe@kernel.dk>,
        Pavel Begunkov <asml.silence@gmail.com>,
        <io-uring@vger.kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <kernel-team@fb.com>,
        Dylan Yudaken <dylany@fb.com>
Subject: [PATCH v2 4/4] io_uring: pre-increment f_pos on rw
Date:   Mon, 21 Feb 2022 06:16:49 -0800
Message-ID: <20220221141649.624233-5-dylany@fb.com>
In-Reply-To: <20220221141649.624233-1-dylany@fb.com>
References: <20220221141649.624233-1-dylany@fb.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain
Precedence: bulk

In read/write ops, preincrement f_pos when no offset is specified, and
then attempt fix up the position after IO completes if it completed less
than expected. This fixes the problem where multiple queued up IO will al=
l
obtain the same f_pos, and so perform the same read/write.

This is still not as consistent as sync r/w, as it is able to advance the
file offset past the end of the file. It seems it would be quite a
performance hit to work around this limitation - such as by keeping track
of concurrent operations - and the downside does not seem to be too
problematic.

The attempt to fix up the f_pos after will at least mean that in situatio=
ns
where a single operation is run, then the position will be consistent.

Co-developed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 81 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 13 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index abd8c739988e..a951d0754899 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3066,21 +3066,71 @@ static inline void io_rw_done(struct kiocb *kiocb=
, ssize_t ret)
 	}
 }
=20
-static inline loff_t*
-io_kiocb_update_pos(struct io_kiocb *req, struct kiocb *kiocb)
+static inline bool
+io_kiocb_update_pos(struct io_kiocb *req, struct kiocb *kiocb,
+		loff_t **ppos, u64 expected, bool force_nonblock)
 {
 	bool is_stream =3D req->file->f_mode & FMODE_STREAM;
 	if (kiocb->ki_pos =3D=3D -1) {
 		if (!is_stream) {
-			req->flags |=3D REQ_F_CUR_POS;
+			*ppos =3D &kiocb->ki_pos;
+			WARN_ON(req->flags & REQ_F_CUR_POS);
+			if (req->file->f_mode & FMODE_ATOMIC_POS) {
+				if (force_nonblock) {
+					if (!mutex_trylock(&req->file->f_pos_lock))
+						return true;
+				} else {
+					mutex_lock(&req->file->f_pos_lock);
+				}
+			}
 			kiocb->ki_pos =3D req->file->f_pos;
-			return &kiocb->ki_pos;
+			req->flags |=3D REQ_F_CUR_POS;
+			req->file->f_pos +=3D expected;
+			if (req->file->f_mode & FMODE_ATOMIC_POS)
+				mutex_unlock(&req->file->f_pos_lock);
+			return false;
 		} else {
 			kiocb->ki_pos =3D 0;
-			return NULL;
+			*ppos =3D NULL;
+			return false;
 		}
 	}
-	return is_stream ? NULL : &kiocb->ki_pos;
+	*ppos =3D is_stream ? NULL : &kiocb->ki_pos;
+	return false;
+}
+
+static inline void
+io_kiocb_done_pos(struct io_kiocb *req, struct kiocb *kiocb, u64 actual)
+{
+	u64 expected;
+
+	if (likely(!(req->flags & REQ_F_CUR_POS)))
+		return;
+
+	expected =3D req->rw.len;
+	if (actual >=3D expected)
+		return;
+
+	/*
+	 * It's not definitely safe to lock here, and the assumption is,
+	 * that if we cannot lock the position that it will be changing,
+	 * and if it will be changing - then we can't update it anyway
+	 */
+	if (req->file->f_mode & FMODE_ATOMIC_POS
+		&& !mutex_trylock(&req->file->f_pos_lock))
+		return;
+
+	/*
+	 * now we want to move the pointer, but only if everything is consisten=
t
+	 * with how we left it originally
+	 */
+	if (req->file->f_pos =3D=3D kiocb->ki_pos + (expected - actual))
+		req->file->f_pos =3D kiocb->ki_pos;
+
+	/* else something else messed with f_pos and we can't do anything */
+
+	if (req->file->f_mode & FMODE_ATOMIC_POS)
+		mutex_unlock(&req->file->f_pos_lock);
 }
=20
 static void kiocb_done(struct io_kiocb *req, ssize_t ret,
@@ -3096,8 +3146,7 @@ static void kiocb_done(struct io_kiocb *req, ssize_=
t ret,
 			ret +=3D io->bytes_done;
 	}
=20
-	if (req->flags & REQ_F_CUR_POS)
-		req->file->f_pos =3D req->rw.kiocb.ki_pos;
+	io_kiocb_done_pos(req, &req->rw.kiocb, ret >=3D 0 ? ret : 0);
 	if (ret >=3D 0 && (req->rw.kiocb.ki_complete =3D=3D io_complete_rw))
 		__io_complete_rw(req, ret, issue_flags);
 	else
@@ -3662,21 +3711,23 @@ static int io_read(struct io_kiocb *req, unsigned=
 int issue_flags)
=20
 	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_kiocb_update_pos(req, kiocb, &ppos,
+						req->rw.len, true))) {
 			ret =3D io_setup_async_rw(req, iovec, s, true);
 			return ret ?: -EAGAIN;
 		}
 		kiocb->ki_flags |=3D IOCB_NOWAIT;
 	} else {
+		io_kiocb_update_pos(req, kiocb, &ppos, req->rw.len, false);
 		/* Ensure we clear previously set non-block flag */
 		kiocb->ki_flags &=3D ~IOCB_NOWAIT;
 	}
=20
-	ppos =3D io_kiocb_update_pos(req, kiocb);
-
 	ret =3D rw_verify_area(READ, req->file, ppos, req->result);
 	if (unlikely(ret)) {
 		kfree(iovec);
+		io_kiocb_done_pos(req, kiocb, 0);
 		return ret;
 	}
=20
@@ -3798,14 +3849,17 @@ static int io_write(struct io_kiocb *req, unsigne=
d int issue_flags)
 		    (req->flags & REQ_F_ISREG))
 			goto copy_iov;
=20
+		/* if we cannot lock the file position then punt */
+		if (unlikely(io_kiocb_update_pos(req, kiocb, &ppos, req->rw.len, true)=
))
+			goto copy_iov;
+
 		kiocb->ki_flags |=3D IOCB_NOWAIT;
 	} else {
+		io_kiocb_update_pos(req, kiocb, &ppos, req->rw.len, false);
 		/* Ensure we clear previously set non-block flag */
 		kiocb->ki_flags &=3D ~IOCB_NOWAIT;
 	}
=20
-	ppos =3D io_kiocb_update_pos(req, kiocb);
-
 	ret =3D rw_verify_area(WRITE, req->file, ppos, req->result);
 	if (unlikely(ret))
 		goto out_free;
@@ -3858,6 +3912,7 @@ static int io_write(struct io_kiocb *req, unsigned =
int issue_flags)
 		return ret ?: -EAGAIN;
 	}
 out_free:
+	io_kiocb_done_pos(req, kiocb, 0);
 	/* it's reportedly faster than delegating the null check to kfree() */
 	if (iovec)
 		kfree(iovec);
--=20
2.30.2