Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Max Reitz <mreitz@redhat.com>]

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

On 25.11.2015 17:18, Kevin Wolf wrote:
> Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
>> On 25.11.2015 16:57, Kevin Wolf wrote:
>>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>>>> Put the code for setting up and removing op blockers into an own
>>>> function, respectively. Then, we can invoke those functions whenever a
>>>> BDS is removed from an virtio-blk BB or inserted into it.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> Do you know of a case where this is observable?
>>
>> Actually, no.
>>
>>>                                                 I don't think you can
>>> change the medium of a virtio-blk device, and blk_set_bs() isn't
>>> converted to a wrapper around blk_remove/insert_bs() yet. If this patch
>>> is necessary to fix something observable, the latter is probably a bug.
>>
>> So I guess that implies "Otherwise, this patch should be dropped"?
> 
> I'm not sure. I guess you had a reason to include these patches other
> than putting the notifiers to use?

I'm not sure, it has been so long. :-)

I can very well imagine having included it because a similar change is
necessary to virtio-scsi, so I included it just because I was already
doing work for virtio. Maybe I imagined that virtio-blk may reasonably
offer tray devices in the future, but I just now read you saying
somewhere else:

> Please write your code so that it makes sense today.

So that doesn't hold up. :-)

Maybe I just saw it unblocking the EJECT op blocker, and so I thought
there might be a reason for that.

> With blk_set_bs() changed, I think it would have an effect: The op
> blockers would move from the old BDS to the new top-level one. This
> sounds desirable to me.

Hm, thanks for saving me. Yes, that seems useful indeed.

[...]

>>> This makes me wonder: What do we even block here any more? If I didn't
>>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
>>> why this needs to be blocked, or if we simply forgot to enable it.
>>
>> Well, even though in practice this wall of code doesn't make much sense,
>> of course it will be safe for potential additions of new op blockers.
>>
>> And of course we actually don't want these blockers at all anymore...
> 
> Yes, but dataplane shouldn't really be special enough any more that we
> want to disable features for it initially. By now it sounds more like an
> easy way to forget unblocking a new feature even though it would work.
> 
> So perhaps we should really just remove the blockers from dataplane.
> Then we don't have to answer the question above...

Well, maybe. I guess this is up to Stefan.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]