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

[Kevin Wolf]

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

signature.asc
Description: PGP signature