Re: io_uring_prep_openat_direct() and link/drain

[Jens Axboe <axboe@kernel.dk>]

On 4/1/22 10:21 AM, Jens Axboe wrote:
> On 4/1/22 10:02 AM, Miklos Szeredi wrote:
>> On Fri, 1 Apr 2022 at 17:36, Jens Axboe <axboe@kernel.dk> wrote:
>>
>>> I take it you're continually reusing those slots?
>>
>> Yes.
>>
>>>  If you have a test
>>> case that'd be ideal. Agree that it sounds like we just need an
>>> appropriate breather to allow fput/task_work to run. Or it could be the
>>> deferral free of the fixed slot.
>>
>> Adding a breather could make the worst case latency be large.  I think
>> doing the fput synchronously would be better in general.
> 
> fput() isn't sync, it'll just offload to task_work. There are some
> dependencies there that would need to be checked. But we'll find a way
> to deal with it.
> 
>> I test this on an VM with 8G of memory and run the following:
>>
>> ./forkbomb 14 &
>> # wait till 16k processes are forked
>> for i in `seq 1 100`; do ./procreads u; done
>>
>> You can compare performance with plain reads (./procreads p), the
>> other tests don't work on public kernels.
> 
> OK, I'll check up on this, but probably won't have time to do so before
> early next week.

Can you try with this patch? It's not complete yet, there's actually a
bunch of things we can do to improve the direct descriptor case. But
this one is easy enough to pull off, and I think it'll fix your OOM
case. Not a proposed patch, but it'll prove the theory.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 0e199040f151..d52cd9c98d6d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -231,7 +231,7 @@ struct io_rsrc_put {
 	u64 tag;
 	union {
 		void *rsrc;
-		struct file *file;
+		unsigned long file_ptr;
 		struct io_mapped_ubuf *buf;
 	};
 };
@@ -1601,7 +1601,12 @@ static bool req_need_defer(struct io_kiocb *req, u32 seq)
 
 #define FFS_NOWAIT		0x1UL
 #define FFS_ISREG		0x2UL
-#define FFS_MASK		~(FFS_NOWAIT|FFS_ISREG)
+#if defined(CONFIG_64BIT)
+#define FFS_DIRECT		0x4UL
+#else
+#define FFS_DIRECT		0x0UL
+#endif
+#define FFS_MASK		~(FFS_NOWAIT|FFS_ISREG|FFS_DIRECT)
 
 static inline bool io_req_ffs_set(struct io_kiocb *req)
 {
@@ -7443,12 +7448,19 @@ static inline struct file *io_file_from_index(struct io_ring_ctx *ctx,
 	return (struct file *) (slot->file_ptr & FFS_MASK);
 }
 
-static void io_fixed_file_set(struct io_fixed_file *file_slot, struct file *file)
+static bool io_fixed_file_set(struct io_fixed_file *file_slot, struct file *file,
+			      bool direct_descriptor)
 {
 	unsigned long file_ptr = (unsigned long) file;
+	bool ret = false;
 
 	file_ptr |= io_file_get_flags(file);
+	if (direct_descriptor) {
+		file_ptr |= FFS_DIRECT;
+		ret = true;
+	}
 	file_slot->file_ptr = file_ptr;
+	return ret;
 }
 
 static inline struct file *io_file_get_fixed(struct io_ring_ctx *ctx,
@@ -8917,7 +8929,7 @@ static int io_sqe_files_scm(struct io_ring_ctx *ctx)
 
 static void io_rsrc_file_put(struct io_ring_ctx *ctx, struct io_rsrc_put *prsrc)
 {
-	struct file *file = prsrc->file;
+	struct file *file = (struct file *) (prsrc->file_ptr & FFS_MASK);
 #if defined(CONFIG_UNIX)
 	struct sock *sock = ctx->ring_sock->sk;
 	struct sk_buff_head list, *head = &sock->sk_receive_queue;
@@ -9083,7 +9095,8 @@ static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
 			fput(file);
 			goto out_fput;
 		}
-		io_fixed_file_set(io_fixed_file_slot(&ctx->file_table, i), file);
+		io_fixed_file_set(io_fixed_file_slot(&ctx->file_table, i), file,
+					false);
 	}
 
 	ret = io_sqe_files_scm(ctx);
@@ -9166,6 +9179,20 @@ static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
 	return 0;
 }
 
+static int io_queue_file_removal(struct io_rsrc_data *data, unsigned idx,
+				 struct io_rsrc_node *node,
+				 unsigned long file_ptr)
+{
+	struct file *file = (struct file *) (file_ptr & FFS_MASK);
+
+	if (file_ptr & FFS_DIRECT) {
+		fput(file);
+		return 0;
+	}
+
+	return io_queue_rsrc_removal(data, idx, node, file);
+}
+
 static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 				 unsigned int issue_flags, u32 slot_index)
 {
@@ -9189,15 +9216,13 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 	file_slot = io_fixed_file_slot(&ctx->file_table, slot_index);
 
 	if (file_slot->file_ptr) {
-		struct file *old_file;
-
 		ret = io_rsrc_node_switch_start(ctx);
 		if (ret)
 			goto err;
 
-		old_file = (struct file *)(file_slot->file_ptr & FFS_MASK);
-		ret = io_queue_rsrc_removal(ctx->file_data, slot_index,
-					    ctx->rsrc_node, old_file);
+		ret = io_queue_file_removal(ctx->file_data, slot_index,
+					    ctx->rsrc_node,
+					    file_slot->file_ptr);
 		if (ret)
 			goto err;
 		file_slot->file_ptr = 0;
@@ -9205,13 +9230,13 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 	}
 
 	*io_get_tag_slot(ctx->file_data, slot_index) = 0;
-	io_fixed_file_set(file_slot, file);
-	ret = io_sqe_file_register(ctx, file, slot_index);
-	if (ret) {
-		file_slot->file_ptr = 0;
-		goto err;
+	if (!io_fixed_file_set(file_slot, file, true)) {
+		ret = io_sqe_file_register(ctx, file, slot_index);
+		if (ret) {
+			file_slot->file_ptr = 0;
+			goto err;
+		}
 	}
-
 	ret = 0;
 err:
 	if (needs_switch)
@@ -9228,7 +9253,6 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_ring_ctx *ctx = req->ctx;
 	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 	struct io_fixed_file *file_slot;
-	struct file *file;
 	int ret, i;
 
 	io_ring_submit_lock(ctx, needs_lock);
@@ -9248,8 +9272,8 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
 	if (!file_slot->file_ptr)
 		goto out;
 
-	file = (struct file *)(file_slot->file_ptr & FFS_MASK);
-	ret = io_queue_rsrc_removal(ctx->file_data, offset, ctx->rsrc_node, file);
+	ret = io_queue_file_removal(ctx->file_data, offset,
+				    ctx->rsrc_node, file_slot->file_ptr);
 	if (ret)
 		goto out;
 
@@ -9298,9 +9322,9 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 		file_slot = io_fixed_file_slot(&ctx->file_table, i);
 
 		if (file_slot->file_ptr) {
-			file = (struct file *)(file_slot->file_ptr & FFS_MASK);
-			err = io_queue_rsrc_removal(data, up->offset + done,
-						    ctx->rsrc_node, file);
+			err = io_queue_file_removal(data, up->offset + done,
+						    ctx->rsrc_node,
+						    file_slot->file_ptr);
 			if (err)
 				break;
 			file_slot->file_ptr = 0;
@@ -9326,7 +9350,7 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 				break;
 			}
 			*io_get_tag_slot(data, up->offset + done) = tag;
-			io_fixed_file_set(file_slot, file);
+			io_fixed_file_set(file_slot, file, false);
 			err = io_sqe_file_register(ctx, file, i);
 			if (err) {
 				file_slot->file_ptr = 0;

-- 
Jens Axboe