21.12.2019 1:04, Eric Blake wrote: > On 12/19/19 10:14 AM, Nir Soffer wrote: > >>>>> 1. Using disk name as a bitmap name is a bad behavior, as they are >>>>> completely >>>>> different concepts. Especially keeping in mind that user already knows >>>>> disk name anyway >>>>> and no reason to write this export name inside metadata context of this >>>>> export. >>>> >>>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix. >>>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. >>> >>> Why do you think so? Did you read NBD specification? >> >> Yes - the name of the bitmap does not have any meaning. >> But for nbd_server_add we allow only single bitmap for export. > > Just because qemu is currently limited to only exposing one bitmap at the > moment does not mean that a future version can't expose multiple bitmaps. It > may very well be that we have reason to expose both "qemu:dirty-bitmap:timeA" > and "qemu:dirty-bitmap:timeB" on the same export, for exposing two bitmaps at > once. To get to that point, we'd have to refactor the QAPI command to allow > attaching more than one bitmap at the time of creating the NBD export, but > it's not impossible. > >> >>> Metadata context is always owned by some export. >> >> Of course. >> >>> Do you mean that there will bemetadata contexts >>> >>> qemu:dirty-bitmap:export-A >>> qemu:dirty-bitmap:export-B >>> >>> both defined for export-A? >> >> It does not make sense, but it is valid. > > If an image has multiple bitmaps, exposing all of those as separate contexts > at the same time for a single export can indeed make sense. > >> >>>>> 2. It's not directly documented. You assume that NAME == @name. I >>>>> understand that >>>>> it may be assumed.. But it's not documented. >>>> >>>> But NAME is likely to be understood as the name argument, and unlikely to >>>> be the >>>> bitmap name. >>> >>> Yes likely. But it's still bad specification, which should be fixed. >> >> If we cannot change the current behavior since it will break current users, >> I agree fixing the spec to describe the current behavior is a good idea. > > We need the doc fix.
Do you like the patch starting this thread? It's a fix we are talking about.. > Whether we also want an additional fix adding an optional parameter allowing > user control over the export name is also under debate (the fact that the old > x-nbd-server-add-bitmap supported it shows that it may be useful, but it is > not minimal, and as I pointed out at the time of removing x-, libvirt can > always control what name is exposed by creating a temporary bitmap and > merging from other bitmaps into the temporary). > > We also have to think about a future of parallel backup jobs: libvirt can > create a single temporary bitmap to expose whatever name it wants under one > job, but if libvirt wants to expose the SAME user-visible name to two > parallel jobs, it cannot create two bitmaps with the same name, so having a > way to set the user-visible name of an arbitrary bitmap when producing the > NBD export makes sense on that front. > > >>>> >>>>> 3. It's never worked like you write. So if we change the behavior, we'll >>>>> break >>>>> existing users. >>>> >>>> Do we have existing users? isn't this new feature in 4.2? >>> >>> No, it's since 4.0 > > As long as altering the exported name is controlled by a new optional > parameter, it does not hurt older 4.0 clients that do not use the new > parameter. > -- Best regards, Vladimir
