On Wed, Mar 12, 2025 at 02:22:05PM +0800, saz97 wrote: The email subject says [PATCH 1/2] but I only see one patch email. The second patch is at the bottom of this email. Please send each patch in a separate email with email threading (this is what git-send-email(1) does by default) in the future.
There is a website with useful git-send-email(1) setup advice here: https://git-send-email.io/. You can test your patch emails by sending them to yourself with `git send-email --to s...@qq.com ...`. It would be nice to actually merge this once code review discussion has completed. It will be necessary to have a commit description for each patch to explains its purpose. Performance results demonstrating the advantage of supporting higher queue depths are also needed. > Signed-off-by: Changzhi Xie <s...@qq.com> > --- > block/export/fuse.c | 167 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 114 insertions(+), 53 deletions(-) > > diff --git a/block/export/fuse.c b/block/export/fuse.c > index 465cc9891d..f47117a00d 100644 > --- a/block/export/fuse.c > +++ b/block/export/fuse.c > @@ -64,6 +64,16 @@ typedef struct FuseExport { > gid_t st_gid; > } FuseExport; > > +typedef struct FuseIORequest { > + fuse_req_t req; > + fuse_ino_t inode; > + size_t size; > + off_t offset; > + struct fuse_file_info *fi; > + FuseExport *exp; > + char *write_buf; > +} FuseIORequest; > + > static GHashTable *exports; > static const struct fuse_lowlevel_ops fuse_ops; > > @@ -570,102 +580,153 @@ static void fuse_open(fuse_req_t req, fuse_ino_t > inode, > fuse_reply_open(req, fi); > } > > -/** > - * Handle client reads from the exported image. > - */ > -static void fuse_read(fuse_req_t req, fuse_ino_t inode, > - size_t size, off_t offset, struct fuse_file_info *fi) > +static void coroutine_fn fuse_read_coroutine(void *opaque) > { > - FuseExport *exp = fuse_req_userdata(req); > + FuseIORequest *io_req = opaque; > + FuseExport *exp = io_req->exp; > int64_t length; > - void *buf; > + void *buffer; > int ret; > > - /* Limited by max_read, should not happen */ > - if (size > FUSE_MAX_BOUNCE_BYTES) { > - fuse_reply_err(req, EINVAL); > - return; > + if (io_req->size > FUSE_MAX_BOUNCE_BYTES) { > + fuse_reply_err(io_req->req, EINVAL); > + goto cleanup; > } > > - /** > - * Clients will expect short reads at EOF, so we have to limit > - * offset+size to the image length. > - */ > length = blk_getlength(exp->common.blk); > if (length < 0) { > - fuse_reply_err(req, -length); > - return; > + fuse_reply_err(io_req->req, -length); > + goto cleanup; > } > > - if (offset + size > length) { > - size = length - offset; > + if (io_req->offset + io_req->size > length) { > + io_req->size = length - io_req->offset; > } > > - buf = qemu_try_blockalign(blk_bs(exp->common.blk), size); > - if (!buf) { > - fuse_reply_err(req, ENOMEM); > - return; > + if (io_req->size == 0) { > + fuse_reply_buf(io_req->req, NULL, 0); > + goto cleanup; > + } > + > + buffer = qemu_try_blockalign(blk_bs(exp->common.blk), io_req->size); > + if (!buffer) { > + fuse_reply_err(io_req->req, ENOMEM); > + goto cleanup; > } > > - ret = blk_pread(exp->common.blk, offset, size, buf, 0); > + ret = blk_co_pread(exp->common.blk, io_req->offset, > + io_req->size, buffer, 0); > if (ret >= 0) { > - fuse_reply_buf(req, buf, size); > + fuse_reply_buf(io_req->req, buffer, io_req->size); > } else { > - fuse_reply_err(req, -ret); > + fuse_reply_err(io_req->req, -ret); > } > > - qemu_vfree(buf); > + qemu_vfree(buffer); > + > +cleanup: > + g_free(io_req); > } > > -/** > - * Handle client writes to the exported image. > - */ > -static void fuse_write(fuse_req_t req, fuse_ino_t inode, const char *buf, > - size_t size, off_t offset, struct fuse_file_info *fi) > +static void coroutine_fn fuse_write_coroutine(void *opaque) > { > - FuseExport *exp = fuse_req_userdata(req); > + FuseIORequest *io_req = opaque; > + FuseExport *exp = io_req->exp; > int64_t length; > int ret; > > - /* Limited by max_write, should not happen */ > - if (size > BDRV_REQUEST_MAX_BYTES) { > - fuse_reply_err(req, EINVAL); > - return; > + if (io_req->size > BDRV_REQUEST_MAX_BYTES) { > + fuse_reply_err(io_req->req, EINVAL); > + goto cleanup; > } > > if (!exp->writable) { > - fuse_reply_err(req, EACCES); > - return; > + fuse_reply_err(io_req->req, EACCES); > + goto cleanup; > } > > - /** > - * Clients will expect short writes at EOF, so we have to limit > - * offset+size to the image length. > - */ > length = blk_getlength(exp->common.blk); > if (length < 0) { > - fuse_reply_err(req, -length); > - return; > + fuse_reply_err(io_req->req, -length); > + goto cleanup; > } > > - if (offset + size > length) { > + if (io_req->offset + io_req->size > length) { > if (exp->growable) { > - ret = fuse_do_truncate(exp, offset + size, true, > PREALLOC_MODE_OFF); > + ret = fuse_do_truncate(exp, io_req->offset + io_req->size, > + true, PREALLOC_MODE_OFF); > if (ret < 0) { > - fuse_reply_err(req, -ret); > - return; > + fuse_reply_err(io_req->req, -ret); > + goto cleanup; > } > } else { > - size = length - offset; > + io_req->size = MAX(0, length - io_req->offset); > + if (io_req->size == 0) { > + fuse_reply_write(io_req->req, 0); > + goto cleanup; > + } > } > } > > - ret = blk_pwrite(exp->common.blk, offset, size, buf, 0); > + ret = blk_co_pwrite(exp->common.blk, io_req->offset, io_req->size, > + io_req->write_buf, 0); > if (ret >= 0) { > - fuse_reply_write(req, size); > + fuse_reply_write(io_req->req, io_req->size); > } else { > - fuse_reply_err(req, -ret); > + fuse_reply_err(io_req->req, -ret); > } > + > +cleanup: > + g_free(io_req->write_buf); > + g_free(io_req); > +} > + > +/** > + * Handle client reads from the exported image. > + */ > +static void fuse_read(fuse_req_t req, fuse_ino_t inode, > + size_t size, off_t offset, struct fuse_file_info *fi) > +{ > + FuseExport *exp = fuse_req_userdata(req); > + FuseIORequest *io_req = g_new(FuseIORequest, 1); > + io_req->req = req; > + io_req->inode = inode; > + io_req->size = size; > + io_req->offset = offset; > + io_req->fi = fi; > + io_req->exp = exp; > + > + Coroutine *co = qemu_coroutine_create(fuse_read_coroutine, io_req); > + qemu_coroutine_enter(co); > +} > + > + > +/** > + * Handle client writes to the exported image. > + */ > +static void fuse_write(fuse_req_t req, fuse_ino_t inode, const char *buf, > + size_t size, off_t offset, struct fuse_file_info *fi) > +{ > + FuseExport *exp = fuse_req_userdata(req); > + FuseIORequest *io_req = g_new(FuseIORequest, 1); > + > + io_req->write_buf = g_try_malloc(size); > + if (!io_req->write_buf) { > + fuse_reply_err(req, ENOMEM); > + g_free(io_req); > + return; > + } > + memcpy(io_req->write_buf, buf, size); > + > + io_req->req = req; > + io_req->inode = inode; > + io_req->size = size; > + io_req->offset = offset; > + io_req->fi = fi; > + io_req->exp = exp; > + > + Coroutine *co = qemu_coroutine_create(fuse_write_coroutine, io_req); > + qemu_coroutine_enter(co); > } > > /** > -- > 2.34.1 > > > From 3d2d317a49eb4cbf401294c6e19c72533eeeefad Mon Sep 17 00:00:00 2001 > From: saz97 <s...@qq.com> > Date: Wed, 12 Mar 2025 13:47:01 +0800 > Subject: [PATCH 2/2] allocate independent fuse_buffor each coroutine and store > fuse_file_info copy instead of pointerin FuseIORequest Reviewers appreciate it when a patch series is a sequence of logical changes without intermediate steps (like "fix typo", etc) modifying earlier code changes. That way reviewers don't spend time reviewing and commenting on code that will be replaced immediately. Please squash this patch into the previous one. This can be done with `git rebase -i` and its "squash" command. > > Signed-off-by: Changzhi Xie <s...@qq.com> > --- > block/export/fuse.c | 45 ++++++++++++++++++++++++--------------------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/block/export/fuse.c b/block/export/fuse.c > index f47117a00d..69ffe4f0ca 100644 > --- a/block/export/fuse.c > +++ b/block/export/fuse.c > @@ -69,7 +69,7 @@ typedef struct FuseIORequest { > fuse_ino_t inode; > size_t size; > off_t offset; > - struct fuse_file_info *fi; > + struct fuse_file_info fi; > FuseExport *exp; > char *write_buf; > } FuseIORequest; > @@ -298,6 +298,10 @@ fail: > static void read_from_fuse_export(void *opaque) > { > FuseExport *exp = opaque; > + struct fuse_buf buf = { > + .mem = g_malloc(FUSE_MAX_BOUNCE_BYTES), > + .size = FUSE_MAX_BOUNCE_BYTES, > + }; libfuse's lib/fuse_lowlevel.c:_fuse_session_receive_buf() allocates the memory as needed. It should be freed using fuse_buf_free(). Is it possible to use just `struct fuse_buf buf = {};` and let libfuse automatically allocate memory? > int ret; > > blk_exp_ref(&exp->common); > @@ -314,6 +318,7 @@ static void read_from_fuse_export(void *opaque) > fuse_session_process_buf(exp->fuse_session, &exp->fuse_buf); > > out: > + g_free(buf.mem); fuse_session_process_buf() can return while the coroutine still exists, so it's not safe to free the buffer here. > if (qatomic_fetch_dec(&exp->in_flight) == 1) { > aio_wait_kick(); /* wake AIO_WAIT_WHILE() */ > } This assumes the request has already completed, but now there's a coroutine that might still exist. Please take a look at read_from_fuse_export() and think about how to put the in_flight decrement and blk_exp_unref() into the coroutine so it only happens when the request finishes. > @@ -689,12 +694,15 @@ static void fuse_read(fuse_req_t req, fuse_ino_t inode, > { > FuseExport *exp = fuse_req_userdata(req); > FuseIORequest *io_req = g_new(FuseIORequest, 1); > - io_req->req = req; > - io_req->inode = inode; > - io_req->size = size; > - io_req->offset = offset; > - io_req->fi = fi; > - io_req->exp = exp; > + > + *io_req = (FuseIORequest) { > + .req = req, > + .inode = inode, > + .size = size, > + .offset = offset, > + .exp = exp, > + .fi = *fi, > + }; I'm not sure if FuseIORequest is necessary. If read_from_fuse_export() creates a coroutine that reads and processes the next request, then it should be possible to implement the request directly in fuse_write()/fuse_write() without an intermediate request struct. That would be simpler, avoid the io_req heap allocation, and require less code. > > Coroutine *co = qemu_coroutine_create(fuse_read_coroutine, io_req); > qemu_coroutine_enter(co); > @@ -710,20 +718,15 @@ static void fuse_write(fuse_req_t req, fuse_ino_t > inode, const char *buf, > FuseExport *exp = fuse_req_userdata(req); > FuseIORequest *io_req = g_new(FuseIORequest, 1); > > - io_req->write_buf = g_try_malloc(size); > - if (!io_req->write_buf) { > - fuse_reply_err(req, ENOMEM); > - g_free(io_req); > - return; > - } > - memcpy(io_req->write_buf, buf, size); > - > - io_req->req = req; > - io_req->inode = inode; > - io_req->size = size; > - io_req->offset = offset; > - io_req->fi = fi; > - io_req->exp = exp; > + *io_req = (FuseIORequest) { > + .req = req, > + .inode = inode, > + .size = size, > + .offset = offset, > + .exp = exp, > + .fi = *fi, > + .write_buf = g_memdup2_qemu(buf, size), > + }; > > Coroutine *co = qemu_coroutine_create(fuse_write_coroutine, io_req); > qemu_coroutine_enter(co); > -- > 2.34.1 >
signature.asc
Description: PGP signature
