> On 18 Jan 2024, at 5:08 PM, Alexey Kuznetsov <kuz...@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.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
