On 11/23/2015 09:28 PM, Fam Zheng wrote: > On Mon, 11/23 16:34, John Snow wrote: >> Hmm, what's the idea, here? >> >> This patch does a lot more than just hide hbitmap details from callers >> of block_dirty_bitmap functions. >> >> So we're changing the backing hbitmap to always be one where g=0 and the >> number of physical bits directly is (now) the same as the number of >> 'virtual' bits, pre-patch. Then, to compensate, we handle the shift math >> to convert the bitmap granularity to sector size and vice-versa in the >> Block Dirty Bitmap layer instead of in the hbitmap layer. >> >> What's the benefit? It looks like we just pull all the implementation >> details up from hbitmap and into BdrvDirtyBitmap, which I am not >> immediately convinced of as being a benefit. > > It feels counter intuitive to me with hbitmap handling granularity, it makes > it > more like a HGranularityBitmap rather than HBitmap, and is unnecessarily > complex to work on. >
I guess it's a matter of personal taste on where to try to hide the complexity. Since hbitmap can and already does manage it for us, inertia leaves me satisfied with this option. I don't know if pulling the granularity out of hbitmap and into BdrvDirtyBitmap (so now we have granularity management code in two objects) is a significant gain, but I wouldn't NACK this over that specifically ... I'll let Vladimir et al decide if this does make e.g. his migration/persistence patches easier to write or not. I agree that the new iterator object for BdrvDirtyBitmap is good, though, and wouldn't mind seeing this split up into its two parts: (1) Hiding the hbitmap implementation detail entirely from users of BdrvDirtyBitmap, by adding new BdrvDirtyBitmap iterators (2) Moving the granularity logic up into BdrvDirtyBitmap > Now it's simplified in that only one BdrvDirtyBitmap needs to care about the > granularity, and which I think is a big benefit when we are going to extend > the > dirty bitmap interface, for example to serialize and deserialize for > persistence. > > Fam >
