Re: [PATCH 006/104] virtiofsd: Trim down imported files

[Xiao Yang]

On 2020/1/21 18:51, Dr. David Alan Gilbert wrote:
> * Xiao Yang (address@hidden) wrote:
> > On 2019/12/13 0:37, Dr. David Alan Gilbert (git) wrote:
> > > -       res = fuse_buf_copy(&pipe_buf, buf,
> > > -                           FUSE_BUF_FORCE_SPLICE | FUSE_BUF_SPLICE_NONBLOCK);
> > > -       if (res<   0) {
> > > -               if (res == -EAGAIN || res == -EINVAL) {
> > > -                       /*
> > > -                        * Should only get EAGAIN on kernels with
> > > -                        * broken SPLICE_F_NONBLOCK support (<=
> > > -                        * 2.6.35) where this error or a short read is
> > > -                        * returned even if the pipe itself is not
> > > -                        * full
> > > -                        *
> > > -                        * EINVAL might mean that splice can't handle
> > > -                        * this combination of input and output.
> > > -                        */
> > > -                       if (res == -EAGAIN)
> > > -                               se->broken_splice_nonblock = 1;
> > > -
> > > -                       pthread_setspecific(se->pipe_key, NULL);
> > > -                       fuse_ll_pipe_free(llp);
> > > -                       goto fallback;
> > > -               }
> > > -               res = -res;
> > > -               goto clear_pipe;
> > > -       }
> > > -
> > > -       if (res != 0&&   res<   len) {
> > > -               struct fuse_bufvec mem_buf = FUSE_BUFVEC_INIT(len);
> > > -               void *mbuf;
> > > -               size_t now_len = res;
> > > -               /*
> > > -                * For regular files a short count is either
> > > -                *  1) due to EOF, or
> > > -                *  2) because of broken SPLICE_F_NONBLOCK (see above)
> > > -                *
> > > -                * For other inputs it's possible that we overflowed
> > > -                * the pipe because of small buffer fragments.
> > > -                */
> > > -
> > > -               res = posix_memalign(&mbuf, pagesize, len);
> > > -               if (res != 0)
> > > -                       goto clear_pipe;
> > > -
> > > -               mem_buf.buf[0].mem = mbuf;
> > > -               mem_buf.off = now_len;
> > > -               res = fuse_buf_copy(&mem_buf, buf, 0);
> > > -               if (res>   0) {
> > > -                       char *tmpbuf;
> > > -                       size_t extra_len = res;
> > > -                       /*
> > > -                        * Trickiest case: got more data.  Need to get
> > > -                        * back the data from the pipe and then fall
> > > -                        * back to regular write.
> > > -                        */
> > > -                       tmpbuf = malloc(headerlen);
> > > -                       if (tmpbuf == NULL) {
> > > -                               free(mbuf);
> > > -                               res = ENOMEM;
> > > -                               goto clear_pipe;
> > > -                       }
> > > -                       res = read_back(llp->pipe[0], tmpbuf, headerlen);
> > > -                       free(tmpbuf);
> > > -                       if (res != 0) {
> > > -                               free(mbuf);
> > > -                               goto clear_pipe;
> > > -                       }
> > > -                       res = read_back(llp->pipe[0], mbuf, now_len);
> > > -                       if (res != 0) {
> > > -                               free(mbuf);
> > > -                               goto clear_pipe;
> > > -                       }
> > > -                       len = now_len + extra_len;
> > > -                       iov[iov_count].iov_base = mbuf;
> > > -                       iov[iov_count].iov_len = len;
> > > -                       iov_count++;
> > > -                       res = fuse_send_msg(se, ch, iov, iov_count);
> > > -                       free(mbuf);
> > > -                       return res;
> > > -               }
> > > -               free(mbuf);
> > > -               res = now_len;
> > > -       }
> > > -       len = res;
> > > -       out->len = headerlen + len;
> > > -
> > > -       if (se->debug) {
> > > -               fuse_log(FUSE_LOG_DEBUG,
> > > -                       "   unique: %llu, success, outsize: %i (splice)\n",
> > > -                       (unsigned long long) out->unique, out->len);
> > > -       }
> > > -
> > > -       splice_flags = 0;
> > > -       if ((flags&   FUSE_BUF_SPLICE_MOVE)&&
> > > -           (se->conn.want&   FUSE_CAP_SPLICE_MOVE))
> > > -               splice_flags |= SPLICE_F_MOVE;
> > > -
> > > -       res = splice(llp->pipe[0], NULL, ch ? ch->fd : se->fd,
> > > -                    NULL, out->len, splice_flags);
> > Hi,
> >
> > 1) In buffer.c, fuse_buf_splice() uses splice(2) to copy/move data in some
> > cases if the syscall is supported.
> > 2) One pipe needs to be passed to splice(2) so splice(2) without one pipe
> > fails and then go back to use other ways(e.g. use fuse_buf_fd_to_fd()).
> > 3) fuse_buf_copy() calls fuse_buf_splice() indirectly and this patch has
> > removed all pipes used by fuse_buf_copy().
> >
> > Is it necessary to leave the code related to splice(2)?  Is it going to be
> > used in future?
> > We have to use splice(2) by the correct CONFIG_SPLICE macro If necessary.
> Yes, I think we never set HAVE_SPLICE; so that code can go.
> I'll change this patch to remove that as well.
Hi Dave,

Agreed.
Reviewed-by: Xiao Yang <address@hidden>

Best Regards,
Xiao Yang
> Dave
>
> > Best Regards,
> > Xiao Yang
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>
>
>
> .