Re: [Qemu-devel] [Qemu-trivial] [PATCH] vmdk: Fix cylinders number during convert

[Fam Zheng]

On Thu, 10/30 15:28, Arthur Gautier wrote:
> On Thu, Oct 30, 2014 at 06:27:20PM +0800, Fam Zheng wrote:
> > On Thu, 10/30 10:09, Arthur Gautier wrote:
> > > On Wed, Oct 29, 2014 at 09:28:52AM +0800, Fam Zheng wrote:
> > > > On Tue, 10/28 16:00, Stefan Hajnoczi wrote:
> > > > > On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> > > > > > Michael Tokarev <address@hidden> writes:
> > > > > > 
> > > > > > > On 10/22/2014 05:25 PM, Arthur Gautier wrote:
> > > > > > >> We can not rely on int cast to get a correct number of 
> > > > > > >> cylinders. The
> > > > > > >> cylinders information was wrong in 49.9999% of cases.
> > > > > > >> 
> > > > > > >> This ensures the cylinders always gets the ceiling value.
> > > > > > >
> > > > > > > Good thing, especially the good probability :), and also a good 
> > > > > > > patch
> > > > > > > which comes with a test.
> > > > > > >
> > > > > > > But I wonder if anything can break this way?  Migration, windows 
> > > > > > > guest
> > > > > > > being unable to find its partitions, something else?
> > > > 
> > > > I'd like to hear an answer to this question too, so we know why it's 
> > > > right and
> > > > worth to have.
> > > > 
> > > Sincerely, I didn't tried myself.
> > > 
> > > > > > >
> > > > > > > And more.  What-if our drive size in cylinders will be larger than
> > > > > > > the size in bytes?  The proposed div_round_up() will increase 
> > > > > > > number
> > > > > > > of cylinders, so size in CHS will be larger than size in bytes.  
> > > > > > > Maybe
> > > > > > > there was a reason why originally the size in cylinders was 
> > > > > > > calculated
> > > > > > > by truncating extra fractional part?  What-if guest will try to 
> > > > > > > access
> > > > > > > the very last CHS which is incomplete?
> > > > 
> > > > I don't remember a reason why truncating (I doubt there is any), OTOH 
> > > > I'm not
> > > > sure what is the right thing to do if the guest tries to write to the 
> > > > last
> > > > incomplete CHS either.
> > > > 
> > > > Fam
> > > 
> > > Maybe we can expand the disk to the size matching cylinders * sectors *
> > > sector_size and issue a warning to the user? This way it will always be
> > > safe.
> > > 
> > 
> > Possible, but I don't know if it's worth it. Could you explain what doesn't
> > work now, in order to show *why* you need this change?
> > 
> > Thanks,
> > Fam
> 
> Please take a look at the test in the patch. I have been able to
> generate an image with a number of cylinders of zero which I believe is
> wrong.
> 

Why do you believe it is wrong? We are also able to create images with length
0.

Do you have a real use case for this? If we "fix" this to round up the
cylinders, there might be other issues for those users who actually use this
field.

Do yo know how VMware handle this?

Fam