Re: [PATCH v2 02/20] fuse: Allow exporting BDSs via FUSE

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3775 bytes --]

Am 15.10.2020 um 16:46 hat Max Reitz geschrieben:
> On 15.10.20 10:57, Kevin Wolf wrote:
> > Am 22.09.2020 um 12:49 hat Max Reitz geschrieben:
> >> block-export-add type=fuse allows mounting block graph nodes via FUSE on
> >> some existing regular file.  That file should then appears like a raw
> >> disk image, and accesses to it result in accesses to the exported BDS.
> >>
> >> Right now, we only implement the necessary block export functions to set
> >> it up and shut it down.  We do not implement any access functions, so
> >> accessing the mount point only results in errors.  This will be
> >> addressed by a followup patch.
> >>
> >> We keep a hash table of exported mount points, because we want to be
> >> able to detect when users try to use a mount point twice.  This is
> >> because we invoke stat() to check whether the given mount point is a
> >> regular file, but if that file is served by ourselves (because it is
> >> already used as a mount point), then this stat() would have to be served
> >> by ourselves, too, which is impossible to do while we (as the caller)
> >> are waiting for it to settle.  Therefore, keep track of mount point
> >> paths to at least catch the most obvious instances of that problem.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>

> >> +/**
> >> + * Callback to be invoked when the FUSE session FD can be read from.
> >> + * (This is basically the FUSE event loop.)
> >> + */
> >> +static void read_from_fuse_export(void *opaque)
> >> +{
> >> +    FuseExport *exp = opaque;
> >> +    int ret;
> >> +
> >> +    blk_exp_ref(&exp->common);
> >> +
> >> +    ret = fuse_session_receive_buf(exp->fuse_session, &exp->fuse_buf);
> > 
> > The fuse_session_loop() implementation seems to imply that we should
> > retry on EINTR.
> 
> OK.  I see you’re digging into libfuse already. :)

Well, I have to review against something, and the documentation tends to
be terse...

> >> +    if (ret < 0) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    fuse_session_process_buf(exp->fuse_session, &exp->fuse_buf);
> >> +
> >> +out:
> >> +    blk_exp_unref(&exp->common);
> >> +}
> >> +
> >> +static void fuse_export_shutdown(BlockExport *blk_exp)
> >> +{
> >> +    FuseExport *exp = container_of(blk_exp, FuseExport, common);
> >> +
> >> +    if (exp->fuse_session) {
> >> +        fuse_session_exit(exp->fuse_session);
> >> +
> >> +        if (exp->mounted) {
> >> +            fuse_session_unmount(exp->fuse_session);
> >> +            exp->mounted = false;
> >> +        }
> >> +
> >> +        if (exp->fd_handler_set_up) {
> >> +            aio_set_fd_handler(exp->common.ctx,
> >> +                               fuse_session_fd(exp->fuse_session), true,
> >> +                               NULL, NULL, NULL, NULL);
> >> +            exp->fd_handler_set_up = false;
> >> +        }
> >> +
> >> +        fuse_session_destroy(exp->fuse_session);
> >> +        exp->fuse_session = NULL;
> > 
> > What happens if a request is still in flight?
> > 
> > Oh, can't happen because the driver is fully synchronous after this
> > series. Fair enough, making it asynchronous can come on top of it.
> 
> (I had multiple approaches of handling parallel requests, but none made
> a substantial performance difference, which is why I left the driver in
> the most simple form for this first proposal.)

I think the more relevant part is that we'd block the guest or anything
else running in the main loop while doing I/O.

Not a problem if you spawn a new qemu-storage-daemon just for this FUSE
export, but if you want to have multiple exports, or export from the
system emulator, you probably don't want to have synchronous operations.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]