On Tue, Mar 25, 2025 at 05:06:51PM +0100, Hanna Czenczek wrote:
> Manually read requests from the /dev/fuse FD and process them, without
> using libfuse. This allows us to safely add parallel request processing
> in coroutines later, without having to worry about libfuse internals.
> (Technically, we already have exactly that problem with
> read_from_fuse_export()/read_from_fuse_fd() nesting.)
>
> We will continue to use libfuse for mounting the filesystem; fusermount3
> is a effectively a helper program of libfuse, so it should know best how
> to interact with it. (Doing it manually without libfuse, while doable,
> is a bit of a pain, and it is not clear to me how stable the "protocol"
> actually is.)
>
> @@ -247,6 +268,14 @@ static int fuse_export_create(BlockExport *blk_exp,
>
> g_hash_table_insert(exports, g_strdup(exp->mountpoint), NULL);
>
> + exp->fuse_fd = fuse_session_fd(exp->fuse_session);
> + ret = fcntl(exp->fuse_fd, F_SETFL, O_NONBLOCK);
fctnl(F_SETFL) should be used in a read-modify-write pattern with
F_GETFL (otherwise, you are nuking any other flags that might have
been important).
See also block/file-posix.c:fcntl_setfl. Maybe we should hoist that
into a common helper in util/osdep.c?
> /**
> - * Handle client reads from the exported image.
> + * Handle client reads from the exported image. Allocates *bufptr and reads
> + * data from the block device into that buffer.
Worth calling out tht *bufptr must be freed with qemu_vfree...
> + * Returns the buffer (read) size on success, and -errno on error.
> */
> -static void fuse_read(fuse_req_t req, fuse_ino_t inode,
> - size_t size, off_t offset, struct fuse_file_info *fi)
> +static ssize_t fuse_read(FuseExport *exp, void **bufptr,
> + uint64_t offset, uint32_t size)
...
> {
> buf = qemu_try_blockalign(blk_bs(exp->common.blk), size);
> if (!buf) {
> - fuse_reply_err(req, ENOMEM);
> - return;
> + return -ENOMEM;
> }
>
> ret = blk_pread(exp->common.blk, offset, size, buf, 0);
> - if (ret >= 0) {
> - fuse_reply_buf(req, buf, size);
> - } else {
> - fuse_reply_err(req, -ret);
> + if (ret < 0) {
> + qemu_vfree(buf);
> + return ret;
...since internal cleanup recognizes that plain free() is wrong?
> #ifdef CONFIG_FUSE_LSEEK
> /**
> * Let clients inquire allocation status.
> + * Return the number of bytes written to *out on success, and -errno on
> error.
> */
> -static void fuse_lseek(fuse_req_t req, fuse_ino_t inode, off_t offset,
> - int whence, struct fuse_file_info *fi)
> +static ssize_t fuse_lseek(FuseExport *exp, struct fuse_lseek_out *out,
> + uint64_t offset, uint32_t whence)
> {
> - FuseExport *exp = fuse_req_userdata(req);
> -
> if (whence != SEEK_HOLE && whence != SEEK_DATA) {
> - fuse_reply_err(req, EINVAL);
> - return;
> + return -EINVAL;
Unrelated to this patch, but any reason why we only SEEK_HOLE/DATA
(and not, say, SEEK_SET)? Is it because we aren't really maintaining
the notion of a current offset? I guess that works as long as the
caller is always using pread/pwrite (never bare read/write where
depending on our internal offset would matter).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
