From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5LTA-0004PB-ED for qemu-devel@nongnu.org; Tue, 24 May 2016 19:10:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5LT7-0006MO-FF for qemu-devel@nongnu.org; Tue, 24 May 2016 19:10:35 -0400 References: <1464079240-24362-1-git-send-email-pl@kamp.de> From: Eric Blake Message-ID: <5744DF5F.3070600@redhat.com> Date: Tue, 24 May 2016 17:10:23 -0600 MIME-Version: 1.0 In-Reply-To: <1464079240-24362-1-git-send-email-pl@kamp.de> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8Ws00F9h6dGjWbVJqC1QoQjecmUJlt0cG" Subject: Re: [Qemu-devel] [PATCH V3] block/iscsi: allow caching of the allocation map List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-block@nongnu.org Cc: pbonzini@redhat.com, famz@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8Ws00F9h6dGjWbVJqC1QoQjecmUJlt0cG Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Signed-off-by: Peter Lieven > --- > +static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags) > { > - if (iscsilun->allocationmap =3D=3D NULL) { > - return; > + iscsi_allocmap_free(iscsilun); > + > + iscsilun->allocmap_size =3D > + 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 =3D bitmap_try_new(iscsilun->allocmap_size); > + if (!iscsilun->allocmap) { > + return -ENOMEM; > + } > + > + if (open_flags & BDRV_O_NOCACHE) { > + /* in case that cache.direct =3D 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()? > + } > + > + iscsilun->allocmap_valid =3D bitmap_try_new(iscsilun->allocmap_siz= e); > + 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; > } > =20 > -static void iscsi_allocationmap_clear(IscsiLun *iscsilun, int64_t sect= or_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 =3D=3D NULL) { > + > + if (iscsilun->allocmap =3D=3D 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? > cluster_num =3D DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors= ); > nb_clusters =3D (sector_num + nb_sectors) / iscsilun->cluster_sect= ors > - 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); =2E..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 =3D=3D 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)? > + if (valid) { > + if (nb_clusters > 0) { > + bitmap_set(iscsilun->allocmap_valid, cluster_num, nb_clust= ers); > + } > + } else { > + bitmap_clear(iscsilun->allocmap_valid, > + sector_num / iscsilun->cluster_sectors, > + DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sector= s)); Here, the rounding is opposite - you round in for valid (leaving the head and tail in an unknown state because you didn't visit the whole cluster), and round out for clear (you don't bother checking whether your action will change the state of the head and tail, so it's easier to just mark that cluster as needing a refresh). Again, comments might help. And if you are relying on allocmap_valid before consulting allocmap, I don't know if it makes any sense to mark any bits of the allocmap above if those bits will just be masked out by allocmap_valid, so maybe the earlier code could unconditionally round in, instead of rounding out for allocated and in for unallocated. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --8Ws00F9h6dGjWbVJqC1QoQjecmUJlt0cG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXRN9fAAoJEKeha0olJ0NqTcwH/13r4qDmGOt8uI6gJVtz+FB3 lcXUhhKVh0b8dv5Ut6535u2nRXjTNHp7gX5XCHiUqxJ6OcGWK7oLJgYOCscxtk14 if0hFUiGoDGgz85c/ZVSjV6cJU+k2jaq/GS5M07wKCxXHTXmvCJPZynIjvuCC8Ml /2xKh9lZENRiDdGg0A9dboCDbhgGVTpjcEcBstBz8RLamJpJbQ/xtWv7McxGz/J2 skxMQUSP1BuqyWFHPNgbaVybkGb/dbcHryB7AiyBHfohx2gc7whfwtMGfzpfbFFp rzDVmJS0QCGOKrvUHzpTSdTA1qUh227e+tfThhVEpmn3KZOri+o5staMlnvO8qk= =F/1T -----END PGP SIGNATURE----- --8Ws00F9h6dGjWbVJqC1QoQjecmUJlt0cG--