Skip to contentSkip to conversation

io_uring lseek

RFC In review Hao Xu 5 participants 19 messages Jul 26, 2023 – Jul 27, 2023

Subsystems (6)
lore
Share & tools
lore

Hao Xu

author
#0 lore raw

cover io_uring lseek

From: Hao Xu <[email protected]>

This series adds lseek for io_uring, the motivation to import this syscall is in previous io_uring getdents patchset, we lack a way to rewind the file cursor when it goes to the end of file. Another reason is lseek is a common syscall, it's good for coding consistency when users use io_uring as their main loop.

Patch 1 is code clean for iomap
Patch 2 adds IOMAP_NOWAIT logic for iomap lseek
Patch 3 adds a nowait parameter to for IOMAP_NOWAIT control Patch 4 adds llseek_nowait() for file_operations so that specific
filesystem can implement it for nowait lseek
Patch 5 adds llseek_nowait() implementation for xfs Patch 6 adds a new vfs wrapper for io_uring use
Patch 7 is the main io_uring lseek implementation

Note, this series depends on the previous io_uring getdents series.

This is marked RFC since there is (at least) an issue to be discussed:
The work in this series is mainly to reslove a problem that the current llseek() in struct file_operations doesn't have a place to deliver nowait info, and adding an argument to it results in update for llseek implementation of all filesystems (35 functions), so here I introduce a new llseek_nowait() as a workaround.

For performance, it has about 20%~30% improvement on iops. The test program is just like the one for io_uring getdents, here is the link to it: https://github.com/HowHsu/liburing/blob/llseek/test/lseek.c
- Each test runs about 30000 async requests/sync syscalls
- Each test runs 100 times and get the average value.
- offset is randomly generated value
- the file is a 1M all zero file

[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_SET, offset: 334772
Average of  sync :  0.012300650000000002
Average of  iouring :  0.008528009999999999
30.67%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_CUR, offset: 389292
Average of  sync :  0.012736129999999995
Average of  iouring :  0.00928725
27.08%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_END, offset: 281141
Average of  sync :  0.01221595
Average of  iouring :  0.008442890000000003
30.89%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_DATA, offset: 931103
Average of  sync :  0.015496230000000005
Average of  iouring :  0.012341509999999998
20.36%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_HOLE, offset: 430194
Average of  sync :  0.01555663000000001
Average of  iouring :  0.012064940000000003
22.45%

Hao Xu (7):
iomap: merge iomap_seek_hole() and iomap_seek_data()
xfs: add nowait support for xfs_seek_iomap_begin() add nowait parameter for iomap_seek()
add llseek_nowait() for struct file_operations add llseek_nowait support for xfs
add vfs_lseek_nowait()
add lseek for io_uring

 fs/ext4/file.c                |  9 ++---
 fs/gfs2/inode.c               |  4 +--
 fs/iomap/seek.c               | 42 ++++++-----------------
 fs/read_write.c               | 18 ++++++++++
 fs/xfs/xfs_file.c             | 34 ++++++++++++++++---
 fs/xfs/xfs_iomap.c            |  4 ++-
 include/linux/fs.h            |  4 +++
 include/linux/iomap.h         |  6 ++--
 include/uapi/linux/io_uring.h |  1 +
 io_uring/fs.c                 | 63 +++++++++++++++++++++++++++++++++++
 io_uring/fs.h                 |  3 ++
 io_uring/opdef.c              |  8 +++++
 12 files changed, 145 insertions(+), 51 deletions(-)

base-commit: 4a4b046082eca8ae90b654d772fccc30e9f23f4d

Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 1/7 iomap: merge iomap_seek_hole() and iomap_seek_data()

From: Hao Xu <[email protected]>

The two functions share almost same code, merge them together. No functional change in this patch.

---
 fs/ext4/file.c        |  9 ++-------
 fs/gfs2/inode.c       |  4 ++--
 fs/iomap/seek.c       | 40 ++++++++--------------------------------
 fs/xfs/xfs_file.c     |  5 ++---
 include/linux/iomap.h |  6 ++----
 5 files changed, 16 insertions(+), 48 deletions(-)
fs/ext4/file.c +2 -7
index c457c8517f0f..3d59993bce56 100644
@@ -936,15 +936,10 @@ loff_t ext4_llseek(struct file *file, loff_t offset, int whence)
 		return generic_file_llseek_size(file, offset, whence,
 						maxbytes, i_size_read(inode));
 	case SEEK_HOLE:
-		inode_lock_shared(inode);
-		offset = iomap_seek_hole(inode, offset,
-					 &ext4_iomap_report_ops);
-		inode_unlock_shared(inode);
-		break;
 	case SEEK_DATA:
 		inode_lock_shared(inode);
-		offset = iomap_seek_data(inode, offset,
-					 &ext4_iomap_report_ops);
+		offset = iomap_seek(inode, offset, &ext4_iomap_report_ops,
+				    whence == SEEK_HOLE);
 		inode_unlock_shared(inode);
 		break;
 	}
fs/gfs2/inode.c +2 -2
index 17c994a0c0d0..628f9d014491 100644
@@ -2111,7 +2111,7 @@ loff_t gfs2_seek_data(struct file *file, loff_t offset)
 	inode_lock_shared(inode);
 	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
 	if (!ret)
-		ret = iomap_seek_data(inode, offset, &gfs2_iomap_ops);
+		ret = iomap_seek(inode, offset, &gfs2_iomap_ops, false);
 	gfs2_glock_dq_uninit(&gh);
 	inode_unlock_shared(inode);
 
@@ -2130,7 +2130,7 @@ loff_t gfs2_seek_hole(struct file *file, loff_t offset)
 	inode_lock_shared(inode);
 	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
 	if (!ret)
-		ret = iomap_seek_hole(inode, offset, &gfs2_iomap_ops);
+		ret = iomap_seek(inode, offset, &gfs2_iomap_ops, true);
 	gfs2_glock_dq_uninit(&gh);
 	inode_unlock_shared(inode);
 
fs/iomap/seek.c +8 -32
index a845c012b50c..5e8641c5f0da 100644
@@ -30,32 +30,6 @@ static loff_t iomap_seek_hole_iter(const struct iomap_iter *iter,
 	}
 }
 
-loff_t
-iomap_seek_hole(struct inode *inode, loff_t pos, const struct iomap_ops *ops)
-{
-	loff_t size = i_size_read(inode);
-	struct iomap_iter iter = {
-		.inode	= inode,
-		.pos	= pos,
-		.flags	= IOMAP_REPORT,
-	};
-	int ret;
-
-	/* Nothing to be found before or beyond the end of the file. */
-	if (pos < 0 || pos >= size)
-		return -ENXIO;
-
-	iter.len = size - pos;
-	while ((ret = iomap_iter(&iter, ops)) > 0)
-		iter.processed = iomap_seek_hole_iter(&iter, &pos);
-	if (ret < 0)
-		return ret;
-	if (iter.len) /* found hole before EOF */
-		return pos;
-	return size;
-}
-EXPORT_SYMBOL_GPL(iomap_seek_hole);
-
 static loff_t iomap_seek_data_iter(const struct iomap_iter *iter,
 		loff_t *hole_pos)
 {
@@ -77,7 +51,8 @@ static loff_t iomap_seek_data_iter(const struct iomap_iter *iter,
 }
 
 loff_t
-iomap_seek_data(struct inode *inode, loff_t pos, const struct iomap_ops *ops)
+iomap_seek(struct inode *inode, loff_t pos, const struct iomap_ops *ops,
+	   bool hole)
 {
 	loff_t size = i_size_read(inode);
 	struct iomap_iter iter = {
@@ -93,12 +68,13 @@ iomap_seek_data(struct inode *inode, loff_t pos, const struct iomap_ops *ops)
 
 	iter.len = size - pos;
 	while ((ret = iomap_iter(&iter, ops)) > 0)
-		iter.processed = iomap_seek_data_iter(&iter, &pos);
+		iter.processed = hole ? iomap_seek_hole_iter(&iter, &pos) :
+				 iomap_seek_data_iter(&iter, &pos);
 	if (ret < 0)
 		return ret;
-	if (iter.len) /* found data before EOF */
+	if (iter.len) /* found hole/data before EOF */
 		return pos;
-	/* We've reached the end of the file without finding data */
-	return -ENXIO;
+	/* We've reached the end of the file without finding hole/data */
+	return hole ? size : -ENXIO;
 }
-EXPORT_SYMBOL_GPL(iomap_seek_data);
+EXPORT_SYMBOL_GPL(iomap_seek);
fs/xfs/xfs_file.c +2 -3
index 4f502219ae4f..d7d37f8fb6bc 100644
@@ -1271,10 +1271,9 @@ xfs_file_llseek(
 	default:
 		return generic_file_llseek(file, offset, whence);
 	case SEEK_HOLE:
-		offset = iomap_seek_hole(inode, offset, &xfs_seek_iomap_ops);
-		break;
 	case SEEK_DATA:
-		offset = iomap_seek_data(inode, offset, &xfs_seek_iomap_ops);
+		offset = iomap_seek(inode, offset, &xfs_seek_iomap_ops,
+				    whence == SEEK_HOLE);
 		break;
 	}
 
include/linux/iomap.h +2 -4
index e2b836c2e119..22d5f9b19a22 100644
@@ -274,10 +274,8 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
 			const struct iomap_ops *ops);
 int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		u64 start, u64 len, const struct iomap_ops *ops);
-loff_t iomap_seek_hole(struct inode *inode, loff_t offset,
-		const struct iomap_ops *ops);
-loff_t iomap_seek_data(struct inode *inode, loff_t offset,
-		const struct iomap_ops *ops);
+loff_t iomap_seek(struct inode *inode, loff_t offset,
+		  const struct iomap_ops *ops, bool hole);
 sector_t iomap_bmap(struct address_space *mapping, sector_t bno,
 		const struct iomap_ops *ops);
 
Signature · 1 line
2.25.1
#2 lore raw

Re: [PATCH 1/7] iomap: merge iomap_seek_hole() and iomap_seek_data()

Quoted 4 lines · Hao Xu

From: Hao Xu <[email protected]>

The two functions share almost same code, merge them together. No functional change in this patch.

No, please don't. seek data and seek hole have subtly different semantics and return values, and we've explicitly kept them separate because it's much easier to maintain them as separate functions with separate semantics than combine them into a single function with lots of non-obvious conditional behaviour.

The fact that the new iomap_seek() API requires a boolean "SEEK_DATA or SEEK_HOLE" field to indicate that the caller wants makes it clear that this isn't actually an improvement over explicit iomap_seek_data() and iomap_seek_hole() API calls.

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#3 lore raw

Re: [PATCH 1/7] iomap: merge iomap_seek_hole() and iomap_seek_data()

Quoted 12 lines · Dave Chinner
Quoted 4 lines · Hao Xu

From: Hao Xu <[email protected]>

The two functions share almost same code, merge them together. No functional change in this patch.

No, please don't. seek data and seek hole have subtly different semantics and return values, and we've explicitly kept them separate because it's much easier to maintain them as separate functions with separate semantics than combine them into a single function with lots of non-obvious conditional behaviour.

Quoted 6 lines

The fact that the new iomap_seek() API requires a boolean "SEEK_DATA or SEEK_HOLE" field to indicate that the caller wants makes it clear that this isn't actually an improvement over explicit iomap_seek_data() and iomap_seek_hole() API calls.

-Dave.

I'll revert it back in next version. Thanks.

author
#4 lore raw

PATCH 2/7 xfs: add nowait support for xfs_seek_iomap_begin()

In reply to Hao Xu

From: Hao Xu <[email protected]>

To support nowait llseek(), IOMAP_NOWAIT semantics should be respected. In xfs, xfs_seek_iomap_begin() is the only place which may be blocked by ilock and extent loading. Let's turn it into trylock logic just like what we've done in xfs_readdir().

---
 fs/xfs/xfs_iomap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/xfs/xfs_iomap.c +3 -1
index 18c8f168b153..bbd7c6b27701 100644
@@ -1294,7 +1294,9 @@ xfs_seek_iomap_begin(
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 
-	lockmode = xfs_ilock_data_map_shared(ip);
+	lockmode = xfs_ilock_data_map_shared_generic(ip, flags & IOMAP_NOWAIT);
+	if (!lockmode)
+		return -EAGAIN;
 	error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
 	if (error)
 		goto out_unlock;
Signature · 1 line
2.25.1
#5 lore raw

Re: [PATCH 2/7] xfs: add nowait support for xfs_seek_iomap_begin()

Quoted 17 lines · Hao Xu

From: Hao Xu <[email protected]>

To support nowait llseek(), IOMAP_NOWAIT semantics should be respected. In xfs, xfs_seek_iomap_begin() is the only place which may be blocked by ilock and extent loading. Let's turn it into trylock logic just like what we've done in xfs_readdir().

---
 fs/xfs/xfs_iomap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/xfs/xfs_iomap.c +0 -0
index 18c8f168b153..bbd7c6b27701 100644

fs/xfs/xfs_iomap.c xfs_seek_iomap_begin( · line 1297

Quoted 5 lines
fs/xfs/xfs_iomap.c +1 -1
@@ -1294,7 +1294,9 @@ xfs_seek_iomap_begin(
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 
-	lockmode = xfs_ilock_data_map_shared(ip);
+	lockmode = xfs_ilock_data_map_shared_generic(ip, flags & IOMAP_NOWAIT);

What does this magic XFS function I can't find anywhere in this patch set do?

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]
#6 lore raw

Re: [PATCH 2/7] xfs: add nowait support for xfs_seek_iomap_begin()

Quoted 21 lines · Dave Chinner
Quoted 20 lines · Hao Xu

From: Hao Xu <[email protected]>

To support nowait llseek(), IOMAP_NOWAIT semantics should be respected. In xfs, xfs_seek_iomap_begin() is the only place which may be blocked by ilock and extent loading. Let's turn it into trylock logic just like what we've done in xfs_readdir().

---
 fs/xfs/xfs_iomap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/xfs/xfs_iomap.c +0 -0
index 18c8f168b153..bbd7c6b27701 100644
@@ -1294,7 +1294,9 @@ xfs_seek_iomap_begin(
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 
Quoted 5 lines

fs/xfs/xfs_iomap.c xfs_seek_iomap_begin( · line 1297

Quoted 2 lines
fs/xfs/xfs_iomap.c +1 -1
-	lockmode = xfs_ilock_data_map_shared(ip);
+	lockmode = xfs_ilock_data_map_shared_generic(ip, flags & IOMAP_NOWAIT);

What does this magic XFS function I can't find anywhere in this patch set do?

It's in (iirc) the io_uring getdents patchset that wasn't cc'd to linux-xfs and that I haven't looked at yet.

--D

Quoted 4 lines

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#7 lore raw

Re: [PATCH 2/7] xfs: add nowait support for xfs_seek_iomap_begin()

Quoted 22 lines · Darrick J. Wong
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Hao Xu

From: Hao Xu <[email protected]>

To support nowait llseek(), IOMAP_NOWAIT semantics should be respected. In xfs, xfs_seek_iomap_begin() is the only place which may be blocked by ilock and extent loading. Let's turn it into trylock logic just like what we've done in xfs_readdir().

---
  fs/xfs/xfs_iomap.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
fs/xfs/xfs_iomap.c +0 -0
index 18c8f168b153..bbd7c6b27701 100644
@@ -1294,7 +1294,9 @@ xfs_seek_iomap_begin(
  	if (xfs_is_shutdown(mp))
  		return -EIO;
 
Quoted 5 lines
Quoted 5 lines

fs/xfs/xfs_iomap.c xfs_seek_iomap_begin( · line 1297

Quoted 2 lines
fs/xfs/xfs_iomap.c +1 -1
-	lockmode = xfs_ilock_data_map_shared(ip);
+	lockmode = xfs_ilock_data_map_shared_generic(ip, flags & IOMAP_NOWAIT);

What does this magic XFS function I can't find anywhere in this patch set do?

Sorry, forgot to say, It was xfs_ilock_for_readdir() in io_uring getdents patchset, I changed the name since it is now used for lseek as well.

Quoted 4 lines

It's in (iirc) the io_uring getdents patchset that wasn't cc'd to linux-xfs and that I haven't looked at yet.

Hi Darrick, I forwarded the xfs related patch in that series. Forgot to cc xfs list at the beginning. I'll make xfs list be Cc-ed when sending next version. Sorry for inconvenience.

Regards,
Hao

Quoted 6 lines

--D

Quoted 4 lines

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#8 lore raw

PATCH 3/7 add nowait parameter for iomap_seek()

In reply to Hao Xu

From: Hao Xu <[email protected]>

Add a nowait parameter for iomap_seek(), later IOMAP_NOWAIT is set according to this parameter's value.

---
 fs/ext4/file.c        | 2 +-
 fs/gfs2/inode.c       | 4 ++--
 fs/iomap/seek.c       | 4 +++-
 fs/xfs/xfs_file.c     | 2 +-
 include/linux/iomap.h | 2 +-
 5 files changed, 8 insertions(+), 6 deletions(-)
fs/ext4/file.c +1 -1
index 3d59993bce56..c6c38c34148b 100644
@@ -939,7 +939,7 @@ loff_t ext4_llseek(struct file *file, loff_t offset, int whence)
 	case SEEK_DATA:
 		inode_lock_shared(inode);
 		offset = iomap_seek(inode, offset, &ext4_iomap_report_ops,
-				    whence == SEEK_HOLE);
+				    whence == SEEK_HOLE, false);
 		inode_unlock_shared(inode);
 		break;
 	}
fs/gfs2/inode.c +2 -2
index 628f9d014491..5d6e7471cb07 100644
@@ -2111,7 +2111,7 @@ loff_t gfs2_seek_data(struct file *file, loff_t offset)
 	inode_lock_shared(inode);
 	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
 	if (!ret)
-		ret = iomap_seek(inode, offset, &gfs2_iomap_ops, false);
+		ret = iomap_seek(inode, offset, &gfs2_iomap_ops, false, false);
 	gfs2_glock_dq_uninit(&gh);
 	inode_unlock_shared(inode);
 
@@ -2130,7 +2130,7 @@ loff_t gfs2_seek_hole(struct file *file, loff_t offset)
 	inode_lock_shared(inode);
 	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
 	if (!ret)
-		ret = iomap_seek(inode, offset, &gfs2_iomap_ops, true);
+		ret = iomap_seek(inode, offset, &gfs2_iomap_ops, true, false);
 	gfs2_glock_dq_uninit(&gh);
 	inode_unlock_shared(inode);
 
fs/iomap/seek.c +3 -1
index 5e8641c5f0da..319ea19fa90d 100644
@@ -52,7 +52,7 @@ static loff_t iomap_seek_data_iter(const struct iomap_iter *iter,
 
 loff_t
 iomap_seek(struct inode *inode, loff_t pos, const struct iomap_ops *ops,
-	   bool hole)
+	   bool hole, bool nowait)
 {
 	loff_t size = i_size_read(inode);
 	struct iomap_iter iter = {
@@ -66,6 +66,8 @@ iomap_seek(struct inode *inode, loff_t pos, const struct iomap_ops *ops,
 	if (pos < 0 || pos >= size)
 		return -ENXIO;
 
+	if (nowait)
+		iter.flags |= IOMAP_NOWAIT;
 	iter.len = size - pos;
 	while ((ret = iomap_iter(&iter, ops)) > 0)
 		iter.processed = hole ? iomap_seek_hole_iter(&iter, &pos) :
fs/xfs/xfs_file.c +1 -1
index d7d37f8fb6bc..73adc0aee2ff 100644
@@ -1273,7 +1273,7 @@ xfs_file_llseek(
 	case SEEK_HOLE:
 	case SEEK_DATA:
 		offset = iomap_seek(inode, offset, &xfs_seek_iomap_ops,
-				    whence == SEEK_HOLE);
+				    whence == SEEK_HOLE, nowait);
 		break;
 	}
 
include/linux/iomap.h +1 -1
index 22d5f9b19a22..f99769d4fc42 100644
@@ -275,7 +275,7 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
 int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		u64 start, u64 len, const struct iomap_ops *ops);
 loff_t iomap_seek(struct inode *inode, loff_t offset,
-		  const struct iomap_ops *ops, bool hole);
+		  const struct iomap_ops *ops, bool hole, bool nowait);
 sector_t iomap_bmap(struct address_space *mapping, sector_t bno,
 		const struct iomap_ops *ops);
 
Signature · 1 line
2.25.1
#9 lore raw

Re: [PATCH 3/7] add nowait parameter for iomap_seek()

Quoted 4 lines · Hao Xu

From: Hao Xu <[email protected]>

Add a nowait parameter for iomap_seek(), later IOMAP_NOWAIT is set according to this parameter's value.

This shows the problem with adding booleans as flags to functions. Now there are -two- anonymous boolean flags to iomap_seek(), and reading the code becomes really hard to know what is actually being asked of iomap_seek().

This is another reason for not combining iomap_seek_data/hole()...

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#10 lore raw

PATCH 4/7 add llseek_nowait() for struct file_operations

In reply to Hao Xu

From: Hao Xu <[email protected]>

Add a new function member llseek_nowait() in struct file_operations for nowait llseek. It act just like llseek() but has an extra boolean parameter called nowait to indicate if it's a nowait try, avoid IO and locks if so.

---
 include/linux/fs.h | 1 +
 1 file changed, 1 insertion(+)
include/linux/fs.h +1 -0
index f3e315e8efdd..d37290da2d7e 100644
@@ -1823,6 +1823,7 @@ struct file_operations {
 	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
 	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
 				unsigned int poll_flags);
+	loff_t (*llseek_nowait)(struct file *, loff_t, int, bool);
 } __randomize_layout;
 
 struct inode_operations {
Signature · 1 line
2.25.1
#11 lore raw

Re: [PATCH 4/7] add llseek_nowait() for struct file_operations

include/linux/fs.h struct file_operations { · line 1826

Quoted 1 line · Hao Xu
include/linux/fs.h +1 -0
+	loff_t (*llseek_nowait)(struct file *, loff_t, int, bool);

You don't have to name the struct file, but an unnamed int and an unnamed bool is just not acceptable.

author
#12 lore raw

PATCH 5/7 add llseek_nowait support for xfs

In reply to Hao Xu

From: Hao Xu <[email protected]>

Add llseek_nowait() operation for xfs, it acts just like llseek(). The thing different is it delivers nowait parameter to iomap layer.

---
 fs/xfs/xfs_file.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)
fs/xfs/xfs_file.c +27 -2
index 73adc0aee2ff..cba82264221d 100644
@@ -1257,10 +1257,11 @@ xfs_file_readdir(
 }
 
 STATIC loff_t
-xfs_file_llseek(
+__xfs_file_llseek(
 	struct file	*file,
 	loff_t		offset,
-	int		whence)
+	int		whence,
+	bool		nowait)
 {
 	struct inode		*inode = file->f_mapping->host;
 
@@ -1282,6 +1283,28 @@ xfs_file_llseek(
 	return vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
 }
 
+STATIC loff_t
+xfs_file_llseek(
+	struct file	*file,
+	loff_t		offset,
+	int		whence)
+{
+	return __xfs_file_llseek(file, offset, whence, false);
+}
+
+STATIC loff_t
+xfs_file_llseek_nowait(
+	struct file	*file,
+	loff_t		offset,
+	int		whence,
+	bool		nowait)
+{
+	if (file->f_op == &xfs_file_operations)
+		return __xfs_file_llseek(file, offset, whence, nowait);
+	else
+		return generic_file_llseek(file, offset, whence);
+}
+
 #ifdef CONFIG_FS_DAX
 static inline vm_fault_t
 xfs_dax_fault(
@@ -1442,6 +1465,7 @@ xfs_file_mmap(
 
 const struct file_operations xfs_file_operations = {
 	.llseek		= xfs_file_llseek,
+	.llseek_nowait	= xfs_file_llseek_nowait,
 	.read_iter	= xfs_file_read_iter,
 	.write_iter	= xfs_file_write_iter,
 	.splice_read	= xfs_file_splice_read,
@@ -1467,6 +1491,7 @@ const struct file_operations xfs_dir_file_operations = {
 	.read		= generic_read_dir,
 	.iterate_shared	= xfs_file_readdir,
 	.llseek		= generic_file_llseek,
+	.llseek_nowait	= xfs_file_llseek_nowait,
 	.unlocked_ioctl	= xfs_file_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= xfs_file_compat_ioctl,
Signature · 1 line
2.25.1
#13 lore raw

Re: [PATCH 5/7] add llseek_nowait support for xfs

Quoted 66 lines · Hao Xu

From: Hao Xu <[email protected]>

Add llseek_nowait() operation for xfs, it acts just like llseek(). The thing different is it delivers nowait parameter to iomap layer.

---
 fs/xfs/xfs_file.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)
fs/xfs/xfs_file.c +26 -2
index 73adc0aee2ff..cba82264221d 100644
@@ -1257,10 +1257,11 @@ xfs_file_readdir(
 }
 
 STATIC loff_t
-xfs_file_llseek(
+__xfs_file_llseek(
 	struct file	*file,
 	loff_t		offset,
-	int		whence)
+	int		whence,
+	bool		nowait)
 {
 	struct inode		*inode = file->f_mapping->host;
 
@@ -1282,6 +1283,28 @@ xfs_file_llseek(
 	return vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
 }
 
+STATIC loff_t
+xfs_file_llseek(
+	struct file	*file,
+	loff_t		offset,
+	int		whence)
+{
+	return __xfs_file_llseek(file, offset, whence, false);
+}
+
+STATIC loff_t
+xfs_file_llseek_nowait(
+	struct file	*file,
+	loff_t		offset,
+	int		whence,
+	bool		nowait)
+{
+	if (file->f_op == &xfs_file_operations)
+		return __xfs_file_llseek(file, offset, whence, nowait);
+	else
+		return generic_file_llseek(file, offset, whence);
+}
+
 #ifdef CONFIG_FS_DAX
 static inline vm_fault_t
 xfs_dax_fault(
@@ -1442,6 +1465,7 @@ xfs_file_mmap(
 
 const struct file_operations xfs_file_operations = {
 	.llseek		= xfs_file_llseek,
+	.llseek_nowait	= xfs_file_llseek_nowait,
 	.read_iter	= xfs_file_read_iter,
 	.write_iter	= xfs_file_write_iter,
 	.splice_read	= xfs_file_splice_read,

fs/xfs/xfs_file.c const struct file_operations xfs_dir_file_operations = { · line 1497

Quoted 7 lines
fs/xfs/xfs_file.c +1 -0
@@ -1467,6 +1491,7 @@ const struct file_operations xfs_dir_file_operations = {
 	.read		= generic_read_dir,
 	.iterate_shared	= xfs_file_readdir,
 	.llseek		= generic_file_llseek,
+	.llseek_nowait	= xfs_file_llseek_nowait,
 	.unlocked_ioctl	= xfs_file_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= xfs_file_compat_ioctl,

This is pretty nasty. It would be far better just to change the .llseek method than to inflict this on every filesystem for the forseeable future.

Not that I'm a fan of passing "nowait" booleans all through the file operations methods - that way lies madness. We use a control structure for the IO path operations (kiocb) to hold per-call context information, perhaps we need something similar for these other methods that people are wanting to hook up to io_uring (e.g. readdir) so taht we don't have to play whack-a-mole with every new io_uring method that people want and then end up with a different nowait solution for every method.

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#14 lore raw

Re: [PATCH 5/7] add llseek_nowait support for xfs

Quoted 88 lines · Dave Chinner
Quoted 66 lines · Hao Xu

From: Hao Xu <[email protected]>

Add llseek_nowait() operation for xfs, it acts just like llseek(). The thing different is it delivers nowait parameter to iomap layer.

---
  fs/xfs/xfs_file.c | 29 +++++++++++++++++++++++++++--
  1 file changed, 27 insertions(+), 2 deletions(-)
fs/xfs/xfs_file.c +26 -2
index 73adc0aee2ff..cba82264221d 100644
@@ -1257,10 +1257,11 @@ xfs_file_readdir(
  }
 
  STATIC loff_t
-xfs_file_llseek(
+__xfs_file_llseek(
  	struct file	*file,
  	loff_t		offset,
-	int		whence)
+	int		whence,
+	bool		nowait)
  {
  	struct inode		*inode = file->f_mapping->host;
 
@@ -1282,6 +1283,28 @@ xfs_file_llseek(
  	return vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
  }
 
+STATIC loff_t
+xfs_file_llseek(
+	struct file	*file,
+	loff_t		offset,
+	int		whence)
+{
+	return __xfs_file_llseek(file, offset, whence, false);
+}
+
+STATIC loff_t
+xfs_file_llseek_nowait(
+	struct file	*file,
+	loff_t		offset,
+	int		whence,
+	bool		nowait)
+{
+	if (file->f_op == &xfs_file_operations)
+		return __xfs_file_llseek(file, offset, whence, nowait);
+	else
+		return generic_file_llseek(file, offset, whence);
+}
+
  #ifdef CONFIG_FS_DAX
  static inline vm_fault_t
  xfs_dax_fault(
@@ -1442,6 +1465,7 @@ xfs_file_mmap(
 
  const struct file_operations xfs_file_operations = {
  	.llseek		= xfs_file_llseek,
+	.llseek_nowait	= xfs_file_llseek_nowait,
  	.read_iter	= xfs_file_read_iter,
  	.write_iter	= xfs_file_write_iter,
  	.splice_read	= xfs_file_splice_read,

fs/xfs/xfs_file.c const struct file_operations xfs_dir_file_operations = { · line 1497

Quoted 7 lines
fs/xfs/xfs_file.c +1 -0
@@ -1467,6 +1491,7 @@ const struct file_operations xfs_dir_file_operations = {
  	.read		= generic_read_dir,
  	.iterate_shared	= xfs_file_readdir,
  	.llseek		= generic_file_llseek,
+	.llseek_nowait	= xfs_file_llseek_nowait,
  	.unlocked_ioctl	= xfs_file_ioctl,
  #ifdef CONFIG_COMPAT
  	.compat_ioctl	= xfs_file_compat_ioctl,

This is pretty nasty. It would be far better just to change the .llseek method than to inflict this on every filesystem for the forseeable future.

Not that I'm a fan of passing "nowait" booleans all through the file operations methods - that way lies madness. We use a control structure for the IO path operations (kiocb) to hold per-call context information, perhaps we need something similar for these other methods that people are wanting to hook up to io_uring (e.g. readdir) so taht we don't have to play whack-a-mole with every new io_uring method that people want and then end up with a different nowait solution for every method.

Quoted 1 line

-Dave.

whack-a-mole is exactly what I thought when I tried to find a place to hold that...I'll think about it in next version, a generic structure for io_uring is good I believe. I'll update this patchset after the getdents stuff are merged.

Thanks,
Hao

author
#15 lore raw

PATCH 6/7 add vfs_lseek_nowait()

In reply to Hao Xu

From: Hao Xu <[email protected]>

Add a new vfs wrapper for io_uring lseek usage. The reason is the current vfs_lseek() calls llseek() but what we need is llseek_nowait().

---
 fs/read_write.c    | 18 ++++++++++++++++++
 include/linux/fs.h |  3 +++
 2 files changed, 21 insertions(+)
fs/read_write.c +18 -0
index b07de77ef126..b4c3bcf706e2 100644
@@ -290,6 +290,24 @@ loff_t vfs_llseek(struct file *file, loff_t offset, int whence)
 }
 EXPORT_SYMBOL(vfs_llseek);
 
+loff_t vfs_lseek_nowait(struct file *file, off_t offset,
+			 int whence, bool nowait)
+{
+	if (!(file->f_mode & FMODE_LSEEK))
+		return -ESPIPE;
+	/*
+	 * This function is only used by io_uring, thus
+	 * returning -ENOTSUPP is not proper since doing
+	 * nonblock lseek as the first try is asked internally
+	 * by io_uring not by users. Return -ENOTSUPP to users
+	 * is not sane.
+	 */
+	if (!file->f_op->llseek_nowait)
+		return -EAGAIN;
+	return file->f_op->llseek_nowait(file, offset, whence, nowait);
+}
+EXPORT_SYMBOL(vfs_lseek_nowait);
+
 static off_t ksys_lseek(unsigned int fd, off_t offset, unsigned int whence)
 {
 	off_t retval;
include/linux/fs.h +3 -0
index d37290da2d7e..cb804d1f1650 100644
@@ -2654,6 +2654,9 @@ extern loff_t default_llseek(struct file *file, loff_t offset, int whence);
 
 extern loff_t vfs_llseek(struct file *file, loff_t offset, int whence);
 
+extern loff_t vfs_lseek_nowait(struct file *file, off_t offset,
+			       int whence, bool nowait);
+
 extern int inode_init_always(struct super_block *, struct inode *);
 extern void inode_init_once(struct inode *);
 extern void address_space_init_once(struct address_space *mapping);
Signature · 1 line
2.25.1
author
#16 lore raw

PATCH 7/7 add lseek for io_uring

In reply to Hao Xu

From: Hao Xu <[email protected]>

This is related with previous io_uring getdents patchset, we need a way to rewind the cursor of file when it comes to the end of a file by getdents. Introduce lseek to io_uring for this, besides, it's also a common syscall users call. So it's good for coding consistency when users use io_uring as their main loop.

---
 include/uapi/linux/io_uring.h |  1 +
 io_uring/fs.c                 | 63 +++++++++++++++++++++++++++++++++++
 io_uring/fs.h                 |  3 ++
 io_uring/opdef.c              |  8 +++++
 4 files changed, 75 insertions(+)
include/uapi/linux/io_uring.h +1 -0
index c3efe241e310..d445876d4afc 100644
@@ -236,6 +236,7 @@ enum io_uring_op {
 	IORING_OP_SEND_ZC,
 	IORING_OP_SENDMSG_ZC,
 	IORING_OP_GETDENTS,
+	IORING_OP_LSEEK,
 
 	/* this goes last, obviously */
 	IORING_OP_LAST,
io_uring/fs.c +63 -0

72 lines not on this page · raw message

io_uring/fs.h +3 -0
index f83a6f3a678d..32a8441c5142 100644
@@ -21,3 +21,6 @@ void io_link_cleanup(struct io_kiocb *req);
 
 int io_getdents_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
 int io_getdents(struct io_kiocb *req, unsigned int issue_flags);
+
+int io_lseek_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
+int io_lseek(struct io_kiocb *req, unsigned int issue_flags);
io_uring/opdef.c +8 -0
index 1bae6b2a8d0b..eb1f7ee4f079 100644
@@ -433,6 +433,11 @@ const struct io_issue_def io_issue_defs[] = {
 		.prep			= io_getdents_prep,
 		.issue			= io_getdents,
 	},
+	[IORING_OP_LSEEK] = {
+		.needs_file		= 1,
+		.prep			= io_lseek_prep,
+		.issue			= io_lseek,
+	},
 };
 
 
@@ -656,6 +661,9 @@ const struct io_cold_def io_cold_defs[] = {
 	[IORING_OP_GETDENTS] = {
 		.name			= "GETDENTS",
 	},
+	[IORING_OP_LSEEK] = {
+		.name			= "LSEEK",
+	},
 };
 
 const char *io_uring_get_opcode(u8 opcode)
Signature · 1 line
2.25.1
#17 lore raw
In reply to Hao Xu
Quoted 7 lines · Hao Xu

From: Hao Xu <[email protected]>

This series adds lseek for io_uring, the motivation to import this syscall is in previous io_uring getdents patchset, we lack a way to rewind the file cursor when it goes to the end of file. Another reason is lseek is a common syscall, it's good for coding consistency when users use io_uring as their main loop.

While I understand this it is a time consuming review to make sure things work correctly. So before we get this thing going we better get getdents correct first.

Quoted 10 lines

Patch 1 is code clean for iomap
Patch 2 adds IOMAP_NOWAIT logic for iomap lseek
Patch 3 adds a nowait parameter to for IOMAP_NOWAIT control Patch 4 adds llseek_nowait() for file_operations so that specific
filesystem can implement it for nowait lseek
Patch 5 adds llseek_nowait() implementation for xfs Patch 6 adds a new vfs wrapper for io_uring use
Patch 7 is the main io_uring lseek implementation

Quoted 8 lines

Note, this series depends on the previous io_uring getdents series.

This is marked RFC since there is (at least) an issue to be discussed:
The work in this series is mainly to reslove a problem that the current llseek() in struct file_operations doesn't have a place to deliver nowait info, and adding an argument to it results in update for llseek implementation of all filesystems (35 functions), so here I introduce a new llseek_nowait() as a workaround.

My intuition would be to update all filesystems. Adding new inode operations always starts as a temporary thing and then we live with two different methods for the next years or possibly forever.

But it'd be good to hear what others think.

author
#18 lore raw
In reply to Hao Xu
Quoted 89 lines · Hao Xu

From: Hao Xu <[email protected]>

This series adds lseek for io_uring, the motivation to import this syscall is in previous io_uring getdents patchset, we lack a way to rewind the file cursor when it goes to the end of file. Another reason is lseek is a common syscall, it's good for coding consistency when users use io_uring as their main loop.

Patch 1 is code clean for iomap
Patch 2 adds IOMAP_NOWAIT logic for iomap lseek
Patch 3 adds a nowait parameter to for IOMAP_NOWAIT control Patch 4 adds llseek_nowait() for file_operations so that specific
filesystem can implement it for nowait lseek
Patch 5 adds llseek_nowait() implementation for xfs Patch 6 adds a new vfs wrapper for io_uring use
Patch 7 is the main io_uring lseek implementation

Note, this series depends on the previous io_uring getdents series.

This is marked RFC since there is (at least) an issue to be discussed:
The work in this series is mainly to reslove a problem that the current llseek() in struct file_operations doesn't have a place to deliver nowait info, and adding an argument to it results in update for llseek implementation of all filesystems (35 functions), so here I introduce a new llseek_nowait() as a workaround.

For performance, it has about 20%~30% improvement on iops. The test program is just like the one for io_uring getdents, here is the link to it: https://github.com/HowHsu/liburing/blob/llseek/test/lseek.c
- Each test runs about 30000 async requests/sync syscalls
- Each test runs 100 times and get the average value.
- offset is randomly generated value
- the file is a 1M all zero file

[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_SET, offset: 334772
Average of  sync :  0.012300650000000002
Average of  iouring :  0.008528009999999999
30.67%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_CUR, offset: 389292
Average of  sync :  0.012736129999999995
Average of  iouring :  0.00928725
27.08%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_END, offset: 281141
Average of  sync :  0.01221595
Average of  iouring :  0.008442890000000003
30.89%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_DATA, offset: 931103
Average of  sync :  0.015496230000000005
Average of  iouring :  0.012341509999999998
20.36%
[howeyxu@~]$ python3 run_lseek.py
test args:  seek mode:SEEK_HOLE, offset: 430194
Average of  sync :  0.01555663000000001
Average of  iouring :  0.012064940000000003
22.45%

Hao Xu (7):
iomap: merge iomap_seek_hole() and iomap_seek_data()
xfs: add nowait support for xfs_seek_iomap_begin() add nowait parameter for iomap_seek()
add llseek_nowait() for struct file_operations add llseek_nowait support for xfs
add vfs_lseek_nowait()
add lseek for io_uring

  fs/ext4/file.c                |  9 ++---
  fs/gfs2/inode.c               |  4 +--
  fs/iomap/seek.c               | 42 ++++++-----------------
  fs/read_write.c               | 18 ++++++++++
  fs/xfs/xfs_file.c             | 34 ++++++++++++++++---
  fs/xfs/xfs_iomap.c            |  4 ++-
  include/linux/fs.h            |  4 +++
  include/linux/iomap.h         |  6 ++--
  include/uapi/linux/io_uring.h |  1 +
  io_uring/fs.c                 | 63 +++++++++++++++++++++++++++++++++++
  io_uring/fs.h                 |  3 ++
  io_uring/opdef.c              |  8 +++++
  12 files changed, 145 insertions(+), 51 deletions(-)
Quoted 1 line

base-commit: 4a4b046082eca8ae90b654d772fccc30e9f23f4d

Hi folks,
I'll leave this patchset here before the io_uring getdents is merged, then come back and update this one.

Thanks,
Hao