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
