Re: [Qemu-devel] [PATCHv2 RESEND] block/iscsi: speed up read for unallocated sectors

[Peter Lieven]

Am 28.04.2014 15:22, schrieb Paolo Bonzini:
> Il 28/04/2014 13:11, Peter Lieven ha scritto:
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index b490e98..9f5b4a0 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -30,6 +30,8 @@
>>  #include "qemu-common.h"
>>  #include "qemu/config-file.h"
>>  #include "qemu/error-report.h"
>> +#include "qemu/bitops.h"
>> +#include "qemu/bitmap.h"
>>  #include "block/block_int.h"
>>  #include "trace.h"
>>  #include "block/scsi.h"
>> @@ -59,6 +61,8 @@ typedef struct IscsiLun {
>>      struct scsi_inquiry_logical_block_provisioning lbp;
>>      struct scsi_inquiry_block_limits bl;
>>      unsigned char *zeroblock;
>> +    unsigned long *allocationmap;
>> +    int cluster_sectors;
>>  } IscsiLun;
>>
>>  typedef struct IscsiTask {
>> @@ -91,6 +95,7 @@ typedef struct IscsiAIOCB {
>>  #define NOP_INTERVAL 5000
>>  #define MAX_NOP_FAILURES 3
>>  #define ISCSI_CMD_RETRIES 5
>> +#define ISCSI_CHECKALLOC_THRES 63

My intention to introduce this threshold was to have a trade off between how 
much does it hurt to read unallocated sectors with READ16 and ask for the 
allocation status, find out that
sectors are allocated and READ16 then anyway. I was not intending to make this 
correlated to the cluster_size. I just wanted to avoid asking the lba_status 
for e.g. a 4K read and
in this case just read.

>
> One thing that crossed my mind.  You have a threshold of 64 sectors, which is 
> a nice power of two and usually smaller than the minimum "interesting" 
> opt_unmap_gran (64K).  So perhaps one of the following is true:
>
> a) the threshold should be 128
As stated above the THRESHOLD was introduced to justify the additional 
GET_LBA_STATUS callout.

>
> b) the allocationmap should be allocated even for out-of-range 
> opt_unmap_gran, using a granularity of 64 sectors in that case.

Would the increase of the resolution bring any benefit? If we increase the 
resolution I think all sectors falling into the same
physical cluster would end up with the same status.
>
> c) an opt_unmap_gran of 32K should be used by QEMU as a cluster size

Do we have evidence of used cluster sizes on real hardware? I only know our 
Equallogic storages which have 30720 sectors.

In general I don't mind to use any non-zero value here. Worst case, we would 
end up have an allocation map with
1/4096 bytes in size of the target size. Maybe lower-limit should be 8 sectors 
(4K cluster size) which would mean
1/32768 of the target size. For a 100GB target this would mean an allocation 
map of 3.2MB.

>
> d) the threshold should be dynamic and equal to iscsilun->cluster_sectors

I don't think this is useful. For big cluster_sizes we will end up to never use 
the allocation cache.

>
> e) c+d are both true
>
> If you respin for any of the above changes, please change uses of 
> ISCSI_CHECK_ALLOC_THRES to compare with >=.  It would simplify the logic a 
> bit, and the non-power-of-two makes people scratch their heads. Otherwise 
> please post a patch to be applied on top.  For now, I'm holding this patch.

Peter