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

[Max Reitz]

On 20.12.19 12:24, Kevin Wolf wrote:
> Am 20.12.2019 um 11:48 hat Max Reitz geschrieben:
>> On 20.12.19 11:26, Kevin Wolf wrote:
>>> Am 19.12.2019 um 15:38 hat Max Reitz geschrieben:
>>>> fuse-export-add 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 set up the mount point and tear all mount points down
>>>> in bdrv_close_all().  We do not implement any access functions, so
>>>> accessing the mount point only results in errors.  This will be
>>>> addressed by a followup patch.
>>>>
>>>> The set of exported nodes is kept in a hash table so we can later add a
>>>> fuse-export-remove that allows unmounting.
>>>>
>>>> Signed-off-by: Max Reitz <address@hidden>
>>>
>>>> diff --git a/qapi/block.json b/qapi/block.json
>>>> index 145c268bb6..03f8d1b537 100644
>>>> --- a/qapi/block.json
>>>> +++ b/qapi/block.json
>>>> @@ -317,6 +317,29 @@
>>>>  ##
>>>>  { 'command': 'nbd-server-stop' }
>>>>  
>>>> +##
>>>> +# @fuse-export-add:
>>>> +#
>>>> +# Exports a block graph node on some (file) mountpoint as a raw image.
>>>> +#
>>>> +# @node-name: Node to be exported
>>>> +#
>>>> +# @mountpoint: Path on which to export the block device via FUSE.
>>>> +#              This must point to an existing regular file.
>>>> +#
>>>> +# @writable: Whether clients should be able to write to the block
>>>> +#            device via the FUSE export. (default: false)
>>>> +#
>>>> +# Since: 5.0
>>>> +##
>>>> +{ 'command': 'fuse-export-add',
>>>> +  'data': {
>>>> +      'node-name': 'str',
>>>> +      'mountpoint': 'str',
>>>> +      '*writable': 'bool'
>>>> +  },
>>>> +  'if': 'defined(CONFIG_FUSE)' }
>>>
>>> Can this use a BlockExport union from the start like I'm introducing in
>>> the storage daemon series, together with a generic block-export-add?
>>
>> Hm, you mean still adding a FuseExport structure that would be part of
>> BlockExport and then dropping fuse-export-add in favor of a
>> block-export-add that we want anyway?
> 
> Yes.
> 
>>> It also looks like node-name and writable should be part of the common
>>> base of BlockExport.
>>
>> node-name definitely, I’m not so sure about writable.  Or, to be more
>> precise, I think that if we want writable to be in the base, we also
>> want growable to be there: Both are primarily options for the
>> BlockBackend that the exports use.
>>
>> But both of course also need to be supported by the export
>> implementation.  nbd can make its BB growable all it wants, but that
>> doesn’t make it work.
> 
> Right. Pragmatically, I think exports are very like to support writable,
> but probably rather unlikely to support growable. So I do think there
> would be a point for making writable part of the common base, but not
> growable.

True.

But there’s nothing that inherently binds it to FUSE, so I think both
from an implementation’s POV and from a user’s POV, it looks just as
generic as “writable”.  But that’s theory.  I agree that in practice, it
won’t be as generic.

(I realize this doesn’t help much in finding out what we should do.)

>> So if we kept writable and growable in the common base, then the schema
>> would give no information about what exports actually support them.
>>
>> On one hand, I don’t know whether it’s important to have this
>> information in a static form, or whether it’s sufficient to learn at
>> runtime.
>>
>> On the other, I don’t know whether it’s important to have those fields
>> in the base or not.  Would it make a difference on the wire?
> 
> Not for the command itself, so I think we're free to change it later. It
> might make a difference for introspection, though, not sure. Markus?

Yes, I asked because I’m wondering whether it would be more cumbersome
to users if we didn’t keep it in the base structure.

The duplication depends on how we want to design the command.  Should
the export implementations receive a ready-to-use BB?  Or just a
node-name?  In the latter case, we wouldn’t get rid of duplicated code
by having writable/growable in the base.  For the former, it could, but
then again, just taking the WRITE permission and making the BB growable
isn’t that bad to duplicate.

Something to consider is that of course the current NBD code wants a
node-name and not a BB.  So if we decided to generally give export
implementations a BB, then the initial implementation of
qmp_block_export_add() would look a bit freaky: It would first branch
off to qmp_nbd_server_add(), and then open the BB for the “common” case,
but this common case only exists for FUSE (right now).

OTOH, right now we’re free to decide whether we open the BB in
qmp_block_export_add() or fuse.c, and so we might as well just do it in
the former.  If we later find out that this was a stupid idea, we can
always move it into fuse.c.


Now I don’t quite know where I’m trying to get with this.

I suppose it means that we should start with qmp_block_export_add()
creating the BB and handing it over to the export implementation.

Then it makes sense to me that the BB should be ready to go, i.e. have
all the necessary flags and permissions set.  If so, writable and
growable should be part of the base, so the WRITE permission can be
taken and allow_write_beyond_eof can be set.

But there’s a catch: The common code actually cannot pass on a
ready-to-go BB, because it depends on the export type what kinds of
permissions can be shared.  FUSE exports are fine with sharing the
RESIZE permission, but NBD can’t do that.  And, well, if we require the
export implementation to adjust the permissions anyway, we might as well
require it to take WRITE if necessary.  And then the argument for
avoiding duplication is gone.

> Having it in the base might allow us to remove some duplication in the
> code. Probably not much, though, so not too important.
> 
>>> Unfortunately this would mean that I can't use the
>>> same BlockExportNbd for the existing nbd-server-add command any more. I
>>> guess I could somehow get a shared base type for both, though.
>>
>> Hm.  This sounds like you want to make it your problem.  Can I take that
>> to mean that you want to implement block-export-add and I can wait with
>> v2 until that’s done? :-)
> 
> The NBD integration, yes. I already added the BlockExport type to my
> patches, too, but I expect you would beat me to it. I'm not currently
> planning to write a block-export-add because it doesn't add anything new
> for the storage daemon, so FuseExport and the command this is your part.
> The type currently only exists for --export.

That’s too bad. ;-)

I was mostly asking because I imagine it would actually make more sense
to add block-export-add in a seperate (prerequisite) series.

Max

signature.asc
Description: OpenPGP digital signature