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

[Alexey Kuznetsov]

Hello!

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

Indeed. Thanks!

About suggestion,  well, no. Not only this would be not simpler, it
would be just wrong.
All the checks and all the use must be done after fget before fput, otherwise
user space can close the descriptor and even open something
else in another thread.

I thought you would rather suggest rolling the loop at least to reduce
indentation level. It is possible. Thought about this and found this will
be too much trouble for me right now, as actual code which I use for
testing is heavily
patched, it uses ioat dma to copy and has to keep a lot more of context
and I definitely do not want break that patch until final proof this is useless.
Intersting thing though.


On Thu, Jan 18, 2024 at 8:26 PM Kui Liu <kui.liu at acronis.com> wrote:
>
>
>
> > 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.
>
>