From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47928) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agwbM-0000tr-LI for qemu-devel@nongnu.org; Fri, 18 Mar 2016 11:46:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agwbL-00014f-FI for qemu-devel@nongnu.org; Fri, 18 Mar 2016 11:46:12 -0400 Date: Fri, 18 Mar 2016 15:46:00 +0000 From: "Daniel P. Berrange" Message-ID: <20160318154600.GL17895@redhat.com> References: <1458237102-16204-1-git-send-email-berrange@redhat.com> <1458237102-16204-7-git-send-email-berrange@redhat.com> <20160318120935.GB5515@noname.redhat.com> <20160318144542.GJ17895@redhat.com> <20160318151907.GD5515@noname.redhat.com> <20160318153712.GK17895@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160318153712.GK17895@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 6/7] block: add generic full disk encryption driver Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org On Fri, Mar 18, 2016 at 03:37:12PM +0000, Daniel P. Berrange wrote: > On Fri, Mar 18, 2016 at 04:19:07PM +0100, Kevin Wolf wrote: > > Am 18.03.2016 um 15:45 hat Daniel P. Berrange geschrieben: > > > On Fri, Mar 18, 2016 at 01:09:35PM +0100, Kevin Wolf wrote: > > > > Am 17.03.2016 um 18:51 hat Daniel P. Berrange geschrieben: > > > > > + ret = bdrv_co_readv(bs->file->bs, > > > > > + payload_offset + sector_num, > > > > > + cur_nr_sectors, &hd_qiov); > > > > > + qemu_co_mutex_lock(&crypto->lock); > > > > > + if (ret < 0) { > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + if (qcrypto_block_decrypt(crypto->block, > > > > > + sector_num, > > > > > + cipher_data, cur_nr_sectors * 512, > > > > > + NULL) < 0) { > > > > > + ret = -1; > > > > > > > > Need a real -errno code here. > > > > > > > > > + goto cleanup; > > > > > + } > > > > > > > > ...nor is there one between here and the end of the function. > > > > > > > > So what does this CoMutex protect? If qcrypto_block_decrypt() needs this > > > > for some reason (it doesn't seem to be touching anything that isn't per > > > > request, but maybe I'm missing something), would it be clearer to put > > > > the locking only around that call? > > > > > > This just a result of me blindly copying the locking pattern from > > > qcow2.c qcow2_co_readv() method without really understanding what > > > it was protecting. > > > > qcow2 protects a few fields in BDRVQcow2State and metadata that is used > > and possibly modified by requests. For example, after reading in some > > metadata, another request could make changes that invalidate it, and we > > need to protect against that. > > > > I don't see that the crypto driver relies on any global (i.e. not > > per-request) state either in memory or on disk, except for things that > > are never changed after open, so the lock might not be needed. > > Actually it does have global state - the QCryptoCipher object that's > into the QCryptoBlock object must not be used concurrently by multiple > threads, as each thread will need to initialize different IV data. > > > > If it not possible for two calls to bdrv_co_readv() to run in > > > parallel, then I can drop this mutex. > > > > They can. The obvious yield point where a coroutine switch can happen is > > the bdrv_co_readv() call above (but you already unlock for that one). > > Unless qcrypto_block_decrypt() does some I/O internally, we can't have > > any other yield points. > > Ok, so we do need the mutex then to protect the cipher object state > against concurrent use. Ignore this comment & the one above. Since we're using coroutines the qcrypto_block_decrypt() calls can't ever be running truely concurrently. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|