[Devel] [PATCH VZ9 3/3] fs/fuse: enhanced splice support

[Kui Liu]

> On 18 Jan 2024, at 5:08 PM, Alexey Kuznetsov <kuznet at virtuozzo.com> wrote:
> 
>> Is it difficult to separate the verification of inputs, such as fds, size of src buffs against dest buffs etc, from the process of copying data? Similar to what’s been done in ‘copy_out_args()’.
> 
> I am not sure I understand what you mean here. Show, plz.
> 
> Do you really think copy_out_args is even "readable"? :-)
> From my pov it is awful. Actually, I hold 4d patch in series
> which replaces copy_out_args with normal human intellibible
> function which copies data 10% faster. And I do not think
> it was by accident, ugly code is usually a bad code.

OK, what I meant was can we break the big for loop in two smaller ones.  In the first one, just verify whether inputs are valid, something like 

+	for (i = 0; i < nsplices; i++) {
+		void *src, *dst;
+		struct fd f = fdget(fdarr[i]);
+
+		if (f.file) {
+			unsigned int head, tail, mask;
+
+			if (unlikely(f.file->f_op != fuse_g_splice_ops)) {
+				if (fuse_g_splice_ops == NULL) {
+					struct file *probe_files[2];
+
+					if (create_pipe_files(probe_files, 0)) {
+						err = -EBADF;
+						goto out;
+					}
+					fuse_g_splice_ops = probe_files[0]->f_op;
+					fput(probe_files[0]);
+					fput(probe_files[1]);
+				}
+				if (unlikely(f.file->f_op != fuse_g_splice_ops)) {
+					err = -EBADF;
+					goto out;
+				}
+			}
+
+			pipe = f.file->private_data;
+
+			if (pipe == NULL) {
+				err = -EBADF;
+				goto out;
+			}
+		} else {
+			err = -EBADF;
+			goto out;
+		}
		
		/* count total size of inputs */

		fdput(f);
+	}
	/* check whether size of inputs exceeds available buff in req->ap here */


after all checks pass, do data copying in the second loop.  Something like:

	for (i = 0; i < nsplices; i++) {
+		void *src, *dst;
+		struct fd f = fdget(fdarr[i]);

+		pipe = f.file->private_data;
+		pipe_lock(pipe);
+
+		head = pipe->head;
+		mask = pipe->ring_size - 1;
+
+		for (tail = pipe->tail; tail != head; tail++) {
+			struct page *ipage = pipe->bufs[tail & mask].page;
+			int ioff = pipe->bufs[tail & mask].offset;
+			int ilen = pipe->bufs[tail & mask].len;

+			src = kmap_atomic(ipage);
+			while (ilen > 0) {
+				int copy = ilen;

+				if (doff >= dend) {
+					didx++;
+					dpage = ap->pages[didx];
+					doff = ap->descs[didx].offset;
+					dend = doff + ap->descs[didx].length;
+				}
+
+				if (copy > dend - doff)
+					copy = dend - doff;
+					dst = kmap_atomic(dpage);
+					memcpy(dst + doff, src + ioff, copy);
+					kunmap_atomic(dst);
+
+					doff += copy;
+					ioff += copy;
+					ilen -= copy;
+				}
			}
+			kunmap_atomic(src);
+			put_page(ipage);
+			pipe->bufs[tail & mask].ops = NULL;
+			pipe->bufs[tail & mask].page = NULL;
+			pipe->tail = tail + 1;
+		}
+		pipe_unlock(pipe);
+		fdput(f);
	}
 
This way we can fail before starting to copy data if there’s something wrong in inputs, however it does fdget/ fdput twice, I’m not sure how expensive fdget/fdput can be. 
If it’s expensive, maybe we can cache fdget() from the first loop and do fdput() after second loop. 

BTW I just found out there’s no fdput() in the error path. 

Lastly I was definitely not suggesting to do something similar in fuse_copy_args(), which I agree is not that readable.