6:22 a.m.
пн, 10 лют. 2020 о 10:53 Richard W.M. Jones <rjones(a)redhat.com> пише:
On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets wrote:
> From: Nikolay Ivanets <stenavin(a)gmail.com>
>
> I faced with situation where libguestfs cannot recognize partitions on a
> disk image which was partitioned on a system with "4K native" sector
> size support.
>
> In order to fix the issue we need to allow users to specify desired
> physical and/or logical block size per drive basis.
>
> It is definitely not a complete patch but rather a way to request for a
> comments. Nevertheless it is already working patch. I've added an
> optional parameters to add_drive API method which allow specifying
> physical and logical block size for a drive separetely.
>
> There are no documentation and tests yet. Input parameters are not
> validated for correctness.
The patch is generally fine, but ...
> Here are my questions:
> - Am I move in the right direction adding support for physical/logical
> block size?
I'm fairly sure we only need one of these. I'm just not
sure which one we need. I think we need to ask an expert
or look at the qemu / kernel code to find out exactly what
each setting really does.
Here are what they do stand for:
physical_block_size: The physical block size the disk will report to
the guest OS. For Linux this would be the value returned by the
BLKPBSZGET ioctl and describes the disk's hardware sector size which
can be relevant for the alignment of disk data. We don't have an API
to get this one.
logical_block_size: The logical block size the disk will report to the
guest OS. For Linux this would be the value returned by the BLKSSZGET
ioctl and describes the smallest units for disk I/O. We have
blockdev-getsz API to get this value.
How do they use. If your HDD has physical block size = 4096 you might
want make I/O request equals to (or multiple) this value. Otherwise
you might hit performance penalty. I think, the same is valid for
virtual disk image which is located on physical storage with 4K
physical sector size.
> - I'm not sure I've made a good choise for parameter
names: 'pblocksize'
> and 'lblocksize' respectively. I've tried to avoid long names like
> 'physicalblocksize' while keeping readability and semantic.
If we only have one, we can use "blocksize". But it does
require us to answer the previous one.
Here are possible combinations of [pl]blocksize in real world:
physical | logical
-----------------------
4096 | 4096
4096 | 512
512 | 512
Having both values equals works except for case #2 which will not
impact on functionality, only performance might be hit.
If we want to simplify our API - we might expose the only parameter
called 'blocksize' and set physical_block_size and logical_block_size
to the same value.
On the other hand, reading this values from libvirt XML we will take
logical_block_size and attach disk to libguestfs with
physical_block_size == logical_block_size.
> - Do we want to add the same optional parameters to
'add_drive_scratch'
> API method? I think it would be nice but it is up to you.
It should also be added to add_domain and add_libvirt_dom (note all
the APIs which have 'discard' and 'copyonread' already).
Yeah, already did that.
It could be added to add_drive_scratch, I guess. However it
doesn't
seem very useful for scratch drives (why create a scratch drive with
4K sectors which will be thrown away in the end?)
For testing purpose and API consistency, or again if you have 4K
backing storage.
> - What about 'add_dom', 'add_libvirt_dom' API
methods? Should they also
> handle 'blokio' tag in domain XML and act respectively?
Yes.
Yeah, already did that.
> - Anything else I didn't spot yet?
> - Do we want guestfish to accept physical/logical block size per drive
> from command line?
If you can be bothered, but best to put it in a second patch.
OK.
> - What about other virt tools like virt-df, virt-cat and so on?
If you change the command line handling, then it should apply to other
tools mostly automatically. Have a look at how the --format option
works.
Yeah, I spot that already.
--
Mykola Ivanets