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.
> > > > +BlockDriver bdrv_crypto_luks = {
> > > > + .format_name = "luks",
> > > > + .instance_size = sizeof(BlockCrypto),
> > > > + .bdrv_probe = block_crypto_probe_luks,
> > > > + .bdrv_open = block_crypto_open_luks,
> > > > + .bdrv_close = block_crypto_close,
> > > > + .bdrv_create = block_crypto_create_luks,
> > > > + .create_opts = &block_crypto_create_opts_luks,
> > > > +
> > > > + .bdrv_co_readv = block_crypto_co_readv,
> > > > + .bdrv_co_writev = block_crypto_co_writev,
> > > > + .bdrv_getlength = block_crypto_getlength,
> > > > +};
> > >
> > > Rather minimalistic, but we can always add the missing functions later.
> >
> > Do you have any recommendations on which are top priority / important
> > callbacks to add support for so I can prioritize future effort.
>
> Hm... I was thinking of has_zero_init/discard/get_block_status, but I'm
> not sure how interesting that really is with encryption.
>
> In theory, we could discard with undefined contents as the result, if we
> don't mind that we would be exposing that information (on the other
> hand, encrypted qcow2 images will expose it, too). And you have to
> enable unmap manually anyway.
>
> Efficient zero write is out of question, I'm afraid.
>
> The other thing that would be nice are several functions that provide
> information about the image, like refresh_limits, bdrv_info, etc.
Ok, I'll have a look at these.
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 :|
