Re: [Qemu-devel] [PATCHv2 11/11] iscsi: assert that sectors are aligned to LUN blocksize

[Peter Lieven]

Am 02.07.2013 um 09:44 schrieb Stefan Hajnoczi <address@hidden>:

> On Mon, Jul 01, 2013 at 05:59:02PM +0200, Peter Lieven wrote:
>> 
>> Am 01.07.2013 um 16:35 schrieb Stefan Hajnoczi <address@hidden>:
>> 
>>> On Thu, Jun 27, 2013 at 03:11:35PM +0200, Peter Lieven wrote:
>>>> if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE
>>>> it is possible that sector_num or nb_sectors are not correctly
>>>> alligned.
>>>> 
>>>> to avoid corruption we fail requests which are misaligned.
>>>> 
>>>> Signed-off-by: Peter Lieven <address@hidden>
>>>> ---
>>>> block/iscsi.c |   27 +++++++++++++++++++++++++++
>>>> 1 file changed, 27 insertions(+)
>>>> 
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index 0567b46..bff2e1f 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -298,6 +298,13 @@ static int64_t sector_lun2qemu(int64_t sector, 
>>>> IscsiLun *iscsilun)
>>>>    return sector * iscsilun->block_size / BDRV_SECTOR_SIZE;
>>>> }
>>>> 
>>>> +static int64_t is_request_lun_aligned(int64_t sector_num, int nb_sectors,
>>>> +                                      IscsiLun *iscsilun)
>>>> +{
>>>> +    return ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size ||
>>>> +            (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) ? 0 : 
>>>> 1;
>>>> +}
>>> 
>>> When QEMU does geometry probing it reads 2KB.  So if the LUN has 4KB
>>> block size then QEMU will fail to open it?  This would also affect image
>>> formats on top of iSCSI LUNs.
>> 
>> opening a 4K LUN does not fail with my target. So writing unaligned sectors 
>> will
>> result in corruption. we should at least fail all those unaligned operations 
>> until
>> we have a fix or workaround in place in place.
> 
> When QEMU tries to probe the master boot record or when an image format
> performs an unaligned read (say 512 bytes instead of 4 KB), won't we
> fail the read request?  Therefore opening will fail.

I can speak for iSCSI. Currently it fails when opening a boot device. I think 
there
is no MBR probing done for non-boot devices.

Ronnie added some magic to iscsi_aio_readv to have an unaligned number for 
nb_sectors,
but this seems not to work anymore (I will add a patch to remove it). But as 
soon
as nb_sectors is aligned the sector_num will be rounded down and its not exactly
doing what it should. Writing will succeed but with the wrong offset. UNMAP will
always work.

> 
> I agree that it's better to emit an error than to corrupt the disk.  I'm
> just trying to understand what needs to be done on top of this to make 4
> KB LUNs work.

Reading is quite easy, you just have to have a wrapper that is reading an 
aligned
portion of sectors around the original request and then extracting what was 
requested.
Writing is very complicated. Requests have to be serialized, Read-Modify-Write 
for
unaligned write requests. Paolo had all this prepared already.

I wonder if it would be enough to have the block size of the host/iSCSI device 
propagated
to the guest drivers and read 4K (or the protocol block size bytes) when 
probing the MBR.

Windows 8, Windows 2012 and recent LInux support 4kN. So they should
handle all what its needed internally. What Paolo tried to integrate is sth 
like 512e support
for 4kN drives in qemu. What I have read in fact most of the advanced format 
drives do 512e
already. So the only need left is for iSCSI, but I would be totally fine to say 
if you want to attach
a 4kN device to qemu run an operating system that supports it.

Peter