From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60485) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqPUs-00040R-82 for qemu-devel@nongnu.org; Mon, 17 Nov 2014 11:49:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqPUj-0007TK-2I for qemu-devel@nongnu.org; Mon, 17 Nov 2014 11:49:50 -0500 Received: from mail-wg0-x22f.google.com ([2a00:1450:400c:c00::22f]:35415) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqPUi-0007Sy-P3 for qemu-devel@nongnu.org; Mon, 17 Nov 2014 11:49:41 -0500 Received: by mail-wg0-f47.google.com with SMTP id n12so3186106wgh.34 for ; Mon, 17 Nov 2014 08:49:39 -0800 (PST) Date: Mon, 17 Nov 2014 16:49:36 +0000 From: Stefan Hajnoczi Message-ID: <20141117164936.GL16192@stefanha-thinkpad.redhat.com> References: <1415365933-3727-1-git-send-email-fromani@redhat.com> <1415365933-3727-2-git-send-email-fromani@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kaF1vgn83Aa7CiXN" Content-Disposition: inline In-Reply-To: <1415365933-3727-2-git-send-email-fromani@redhat.com> Subject: Re: [Qemu-devel] [RFC][PATCH v2] block: add write threshold reporting for block devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Francesco Romani Cc: kwolf@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mdroth@linux.vnet.ibm.com --kaF1vgn83Aa7CiXN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 07, 2014 at 02:12:13PM +0100, Francesco Romani wrote: Sorry for the long review delay. Looks pretty good, just one real issue to think about at the bottom. > +static void usage_threshold_disable(BlockDriverState *bs) > +{ It would be safest to make this idempotent: if (!usage_threshold_is_set(bs)) { return; } That way it can be called multiple times. > + notifier_with_return_remove(&bs->wr_usage_threshold_notifier); > + bs->wr_offset_threshold =3D 0; > +} > + > +static int usage_threshold_is_set(const BlockDriverState *bs) > +{ > + return !!(bs->wr_offset_threshold > 0); > +} Please use the bool type instead of an int return value. > + > +static int64_t usage_threshold_exceeded(const BlockDriverState *bs, > + const BdrvTrackedRequest *req) > +{ > + if (usage_threshold_is_set(bs)) { > + int64_t amount =3D req->offset + req->bytes - bs->wr_offset_thre= shold; > + if (amount > 0) { > + return amount; > + } > + } > + return 0; > +} > + > +static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, > + void *opaque) > +{ > + BdrvTrackedRequest *req =3D opaque; > + BlockDriverState *bs =3D req->bs; > + int64_t amount =3D 0; > + > + assert((req->offset & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); > + assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); Does the code still make these assumptions or are the asserts left over =66rom previous versions of the patch? > + > + amount =3D usage_threshold_exceeded(bs, req); > + if (amount > 0) { > + qapi_event_send_block_usage_threshold( > + bdrv_get_device_name(bs), /* FIXME: this does not work */ > + amount, > + bs->wr_offset_threshold, > + &error_abort); > + > + /* autodisable to avoid to flood the monitor */ > + usage_threshold_disable(bs); > + } > + > + return 0; /* should always let other notifiers run */ > +} > + > +static void usage_threshold_register_notifier(BlockDriverState *bs) > +{ > + bs->wr_usage_threshold_notifier.notify =3D before_write_notify; > + notifier_with_return_list_add(&bs->before_write_notifiers, > + &bs->wr_usage_threshold_notifier); > +} > + > +void bdrv_set_usage_threshold(BlockDriverState *bs, int64_t threshold_by= tes) > +{ > + BlockDriverState *target_bs =3D bs; > + if (bs->file) { > + target_bs =3D bs->file; > + } Hmm...I think now I understand why you are trying to use bs->file. This is an attempt to make image formats work with the threshold. Unfortunately the BlockDriverState topology can be more complicated than just 1 level. If we hardcode a strategy to traverse bs->file then it will work in most cases: while (bs->file) { bs =3D bs->file; } But there are cases like VMDK extent files where a BlockDriverState actually has multiple children. One way to solve this is to require that the management tool tells QEMU which exact BlockDriverState node the threshold applies to. Then QEMU doesn't need any hardcoded policy. But I'm not sure how realistic that it at the moment (whether management tools are uses node names for each node yet), so it may be best to hardcode the bs->file traversal that I've suggested. Kevin: Do you agree? --kaF1vgn83Aa7CiXN Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUaicgAAoJEJykq7OBq3PId6EH/1WAHWBG4psgUcM/CMh7hJ6d WNaZ6wZhYkYsDr0PkxTuYWRWa7J0Lewx1hHcl7hzZhyze5GmKRXKQT5xDCcqRM1S Yha44pXRXqUE7cI29n09vd/r1pUDCCi7/LUievlWYdzSVusrkkUl5/Bu2FDE0sgj 2Jprd5aRlu7NI6ebNm9O2WB7B/nOY22bj9Co4lDvjhThFJ0YwgjAdvM6Xdx229S/ 4e+dJ8usfnXyN7nTomcsgkcjeC40UpH4hwD9D6JXIRUjKV5xjIBiSQeU/JM5NF+i QgEwUN78GtcCdNK09R8PejEk8RD61ViqJOWY6m9Mr73T2s5zWplBcgyX6t8c+E4= =596T -----END PGP SIGNATURE----- --kaF1vgn83Aa7CiXN--