[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V3] block/iscsi: allow caching of the allocation

From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCH V3] block/iscsi: allow caching of the allocation map
Date: Mon, 30 May 2016 08:33:04 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2

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 <address@hidden>
+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

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) {
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

      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.


+    }
+    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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]