Am 30.05.2016 um 08:33 schrieb Peter Lieven:
> Am 25.05.2016 um 01:10 schrieb Eric Blake:
>> On 05/24/2016 02:40 AM, Peter Lieven wrote:
>>> until now the allocation map was used only as a hint if a cluster
>>> is allocated or not. If a block was not allocated (or Qemu had
>>> no info about the allocation status) a get_block_status call was
>>> issued to check the allocation status and possibly avoid
>>> a subsequent read of unallocated sectors. If a block known to be
>>> allocated the get_block_status call was omitted. In the other case
>>> a get_block_status call was issued before every read to avoid
>>> the necessity for a consistent allocation map. To avoid the
>>> potential overhead of calling get_block_status for each and
>>> every read request this took only place for the bigger requests.
>>>
>>> This patch enhances this mechanism to cache the allocation
>>> status and avoid calling get_block_status for blocks where
>>> the allocation status has been queried before. This allows
>>> for bypassing the read request even for smaller requests and
>>> additionally omits calling get_block_status for known to be
>>> unallocated blocks.
>>>
>>> Signed-off-by: Peter Lieven <p...@kamp.de>
>>> ---
>>> +static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
>>> {
>>> - if (iscsilun->allocationmap == NULL) {
>>> - return;
>>> + iscsi_allocmap_free(iscsilun);
>>> +
>>> + iscsilun->allocmap_size =
>>> + DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks, iscsilun),
>>> + iscsilun->cluster_sectors);
>>> +
>> Computes: ceiling( (num_blocks * block_size / 512) / (cluster_size /
>> 512) ); aka number of clusters. But we don't independently track the
>> cluster size, so I don't see any simpler way of writing it, even if we
>> could be more efficient by not having to first scale through qemu's
>> sector size.
>>
>>> + iscsilun->allocmap = bitmap_try_new(iscsilun->allocmap_size);
>>> + if (!iscsilun->allocmap) {
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + if (open_flags & BDRV_O_NOCACHE) {
>>> + /* in case that cache.direct = on all allocmap entries are
>>> + * treated as invalid to force a relookup of the block
>>> + * status on every read request */
>>> + return 0;
>> Can we cache that we are opened BDRV_O_NOCACHE, so that we don't even
>> have to bother allocating allocmap when we know we are never changing
>> its bits? In other words, can you swap this to be before the
>> bitmap_try_new()?
>
> The idea of the allocmap in cache.direct = on mode is that we can
> still speed up block jobs by skipping large unallocated areas. In this case
> the allocmap has only a hint character. If we don't know the status
> we issue a get_block_status request and verify the status. If its unallocated
> we return zeroes. If we new through an earlier get block status request
> that the area is allocated we can skip the useless get_block_status request.
> This is the old behaviour without this patch.
>
>>
>>> + }
>>> +
>>> + iscsilun->allocmap_valid = bitmap_try_new(iscsilun->allocmap_size);
>>> + if (!iscsilun->allocmap_valid) {
>>> + /* if we are under memory pressure free the allocmap as well */
>>> + iscsi_allocmap_free(iscsilun);
>>> + return -ENOMEM;
>>> }
>>> - bitmap_set(iscsilun->allocationmap,
>>> - sector_num / iscsilun->cluster_sectors,
>>> - DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
>>> +
>>> + return 0;
>>> }
>>> -static void iscsi_allocationmap_clear(IscsiLun *iscsilun, int64_t
>>> sector_num,
>>> - int nb_sectors)
>>> +static void
>>> +iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
>>> + int nb_sectors, bool allocated, bool valid)
>>> {
>>> int64_t cluster_num, nb_clusters;
>>> - if (iscsilun->allocationmap == NULL) {
>>> +
>>> + if (iscsilun->allocmap == NULL) {
>>> return;
>>> }
>> Here, you are short-circuiting when there is no allocmap, but shouldn't
>> you also short-circuit if you are BDRV_O_NOCACHE?
>>
>> Should you assert(!(allocated && !valid)) [or by deMorgan's,
>> assert(!allocated || valid)], to make sure we are only tracking 3 states
>> rather than 4?
>
> sure, we thats a good enhancement altough allocated and invalid doesn't
> hurt.
>
>>
>>> cluster_num = DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors);
>>> nb_clusters = (sector_num + nb_sectors) / iscsilun->cluster_sectors
>>> - cluster_num;
>>> - if (nb_clusters > 0) {
>>> - bitmap_clear(iscsilun->allocationmap, cluster_num, nb_clusters);
>>> + if (allocated) {
>>> + bitmap_set(iscsilun->allocmap,
>>> + sector_num / iscsilun->cluster_sectors,
>>> + DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
>> This says that if we have a sub-cluster request, we can round out to
>> cluster alignment (if our covered part of the cluster is allocated,
>> presumably the rest is allocated as well)...
>>
>>> + } else if (nb_clusters > 0) {
>>> + bitmap_clear(iscsilun->allocmap, cluster_num, nb_clusters);
>> ...while this says if we are marking something clear, we have to round
>> in (while we can trim the aligned middle, we should not mark any
>> unaligned head or tail as trimmed, in case they remain allocated due to
>> the unvisited sectors). Still, it may be worth comments for why your
>> rounding between the two calls is different.
>
> Okay
>
>>
>>> + }
>>> +
>>> + if (iscsilun->allocmap_valid == NULL) {
>>> + return;
>>> + }
>> When do we ever have allocmap set but allocmap_valid clear? Isn't it
>> easier to assume that both maps are present (we are tracking status) or
>> absent (we are BDRV_O_NOCACHE)?
>
> Thats the current behaviour. See above.
Ping.
Peter
