Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add

From: Eric Blake <eblake@redhat.com>
To: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>,
	qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org
Subject: Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add
Date: Wed, 19 Aug 2020 15:05:58 -0500	[thread overview]
Message-ID: <cf3f6122-7cc0-0323-4aa5-9df3482a2f35@redhat.com> (raw)
In-Reply-To: <faa916e7-3c39-84f4-183f-808153abe12d@redhat.com>

On 8/17/20 7:56 AM, Max Reitz wrote:
> On 13.08.20 18:29, Kevin Wolf wrote:
>> qemu-nbd allows use of writethrough cache modes, which mean that write
>> requests made through NBD will cause a flush before they complete.
>> Expose the same functionality in block-export-add.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---

>> +++ b/qapi/block-export.json
>> @@ -167,10 +167,15 @@
>>   # Describes a block export, i.e. how single node should be exported on an
>>   # external interface.
>>   #
>> +# @writethrough: If true, caches are flushed after every write request to the
>> +#                export before completion is signalled. (since: 5.2;
>> +#                default: false)
>> +#
>>   # Since: 4.2
>>   ##
>>   { 'union': 'BlockExportOptions',
>> -  'base': { 'type': 'BlockExportType' },
>> +  'base': { 'type': 'BlockExportType',
>> +            '*writethrough': 'bool' },
>>     'discriminator': 'type',
>>     'data': {
>>         'nbd': 'BlockExportOptionsNbd'
> 
> Hm.  I find it weird to have @writethrough in the base but @device in
> the specialized class.
> 
> I think everything that will be common to all block exports should be in
> the base, and that probably includes a node-name.  I’m aware that will
> make things more tedious in the code, but perhaps it would be a nicer
> interface in the end.  Or is the real problem that that would create
> problems in the storage daemon’s command line interface, because then
> the specialized (legacy) NBD interface would no longer be compatible
> with the new generalized block export interface?
> 
> Anyway, @writable might be a similar story.  A @read-only may make sense
> in general, I think.

And maybe even auto-read-only.  As for @writable vs. @read-only, that's 
a choice of spelling, but we don't want both; there's also the question 
of which default is saner (having to opt-in to allowing writes is more 
verbose than defaulting to allowing writes when possible, but arguably 
saner as it is less risk of unintended writes when you forgot to specify 
the option; @auto-read-only can help in choosing nicer defaults).

> 
> Basically, I think that the export code should be separate from the code
> setting up the BlockBackend that should be exported, so all options
> regarding that BB should be common; and those options are @node-name,
> @writethrough, and @read-only.  (And perhaps other things like
> @resizable, too, even though that isn’t something to consider for NBD.)

NBD isn't resizable yet, but extending the protocol to let it become so 
is on my TODO list.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org