Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Fri, Sep 16, 2011 at 05:53:20PM +0200, Christoph Hellwig wrote: > On Wed, Sep 14, 2011 at 04:50:25PM +0100, Stefan Hajnoczi wrote: > > I think in this case it will not make the code nicer. Since the > > external iSCSI library is based on callbacks it would be necessary to > > write the coroutines<->callbacks adapter functions. So for example, > > the READ10 command would need a function that can be called in > > coroutine context and yields while libiscsi does the I/O. When the > > callback is invoked it will re-enter the coroutine. > > > > The area where coroutines are useful in the block layer is for image > > formats. We already have common coroutines<->callback adapter > > functions in block.c so it's possible to write sequential code for > > image formats. They only need access to block layer functions which > > have already been adapted. But as soon as you interact with a > > callback-based API from the coroutine, then you need to write an > > adapter yourself. > > So you plan on keeping the aio interface around forever? Qemu with two > different I/O pathes was already more than painful enough, I don't > think keeping three, and two of them beeing fairly complex is a good > idea. The synchronous interfaces can be converted to the coroutine interfaces. The block layer needs a public aio interface because device emulation is asynchronous/callback-based. That doesn't mean that BlockDriver needs aio functions since block.c could transparently set up coroutines. So in theory BlockDriver could have only coroutine interfaces. Doing the aio to coroutine conversion is pretty mechanical, that's why I'm not afraid of doing it with this iSCSI code later. Stefan
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/21/2011 11:37 AM, Ronnie Sahlberg wrote: This driver interfaces with the multiplatform posix library for iscsi initiator/client access to iscsi devices hosted at git://github.com/sahlberg/libiscsi.git Do you plan to make a stable release, so that it can be packaged e.g. in Fedora? Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Stefan,
Thanks for your review.
I feel that iscsi would be beneficial for several usecases.
I have implemented all changes you pointed out, except two, and resent
an updated patch to the list.
Please see below.
regards
ronnie sahlberg
On Mon, Sep 12, 2011 at 7:14 PM, Stefan Hajnoczi
wrote:
> On Sat, Sep 10, 2011 at 02:23:30PM +1000, Ronnie Sahlberg wrote:
>
> Looking good. I think this is worth merging because it does offer
> benefits over host iSCSI.
>
>> +static void
>> +iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void
>> *command_data,
>> + void *private_data)
>> +{
>> +}
>> +
>> +static void
>> +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>> +{
>> + IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
>> + IscsiLun *iscsilun = acb->iscsilun;
>> +
>> + acb->status = -ECANCELED;
>> + acb->common.cb(acb->common.opaque, acb->status);
>> + acb->canceled = 1;
>> +
>> + iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
>> + iscsi_abort_task_cb, NULL);
>> +}
>
> The asynchronous abort task call looks odd. If a caller allocates a
> buffer and issues a read request, then we need to make sure that the
> request is really aborted by the time .bdrv_aio_cancel() returns.
>
> If I understand the code correctly, iscsi_aio_cancel() returns
> immediately but the read request will still be in progress. That means
> the caller could now free their data buffer and the read request will
> overwrite that unallocated memory.
You are right.
I have added a new function to libiscsi that will also release the
task structure from libiscsi as well.
So we should no longer have a window where we might have a cancelled
task in flight writing the data to an already released buffer once the
cancelled data buffers start arriving on the socket.
>
>> +static void
>> +iscsi_aio_write10_cb(struct iscsi_context *iscsi, int status,
>> + void *command_data, void *opaque)
>> +{
>> + IscsiAIOCB *acb = opaque;
>> +
>> + trace_iscsi_aio_write10_cb(iscsi, status, acb, acb->canceled);
>> +
>> + if (acb->buf != NULL) {
>> + free(acb->buf);
>> + }
>
> Please just use g_free(acb->buf). g_free(NULL) is defined as a nop so
> the check isn't necessary. Also, this code uses free(3) when it shoulde.
> use g_free(3).
>
Done.
>> +
>> + if (acb->canceled != 0) {
>> + qemu_aio_release(acb);
>> + scsi_free_scsi_task(acb->task);
>> + acb->task = NULL;
>> + return;
>> + }
>> +
>> + acb->status = 0;
>> + if (status < 0) {
>> + error_report("Failed to write10 data to iSCSI lun. %s",
>> + iscsi_get_error(iscsi));
>> + acb->status = -EIO;
>> + }
>> +
>> + iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> + scsi_free_scsi_task(acb->task);
>> + acb->task = NULL;
>> +}
>> +
>> +static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
>> +{
>> + return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
>> +}
>> +
>> +static BlockDriverAIOCB *
>> +iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> + QEMUIOVector *qiov, int nb_sectors,
>> + BlockDriverCompletionFunc *cb,
>> + void *opaque)
>> +{
>> + IscsiLun *iscsilun = bs->opaque;
>> + struct iscsi_context *iscsi = iscsilun->iscsi;
>> + IscsiAIOCB *acb;
>> + size_t size;
>> + int fua = 0;
>> +
>> + /* set FUA on writes when cache mode is write through */
>> + if (!(bs->open_flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))) {
>> + fua = 1;
>> + }
>
> FUA needs to reflect the guest semantics - does this disk have an
> enabled write cache? When bs->open_flags has BDRV_O_CACHE_WB, then the
> guest knows it needs to send flushes because there is a write cache:
>
> if (!(bs->open_flags & BDRV_O_CACHE_WB)) {
> fua = 1;
> }
>
> BDRV_O_NOCACHE is just for local files and sets the O_DIRECT hint. It
> doesn't affect the cache semantics that the guest sees.
>
Done. When I first started building the patch a while ago, both flags
were needed. I missed when the second flag became obsolete upstream.
>> +/*
>> + * We support iscsi url's on the form
>> + * iscsi://[%@][:]//
>> + */
>> +static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>> +{
>> + IscsiLun *iscsilun = bs->opaque;
>> + struct iscsi_context *iscsi = NULL;
>> + struct iscsi_url *iscsi_url = NULL;
>> + struct IscsiTask task;
>> + int ret;
>> +
>> + if ((BDRV_SECTOR_SIZE % 512) != 0) {
>> + error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
>> + "BDRV_SECTOR_SIZE(%lld) is not a multiple "
>> + "of 512", BDRV_SECTOR_SIZE);
>> + return -EINVAL;
>> + }
>
> Another way of saying this is:
> QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE % 512 != 0);
>
> The advantage is that the build fails instead of waiting until iscsi is
> used at runtime until the failure is detec
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Wed, Sep 21, 2011 at 7:45 PM, Paolo Bonzini wrote: > On 09/21/2011 11:37 AM, Ronnie Sahlberg wrote: >> >> This driver interfaces with the multiplatform posix library for iscsi >> initiator/client access to iscsi devices hosted at >> git://github.com/sahlberg/libiscsi.git > > Do you plan to make a stable release, so that it can be packaged e.g. in > Fedora? Yes. As soon as my primary consumer (==QEMU) has integrated the patch I will make a stable release for libiscsi to match. Until then I keep it "unstable" so I can implement what changes that the QEMU iscsi patch may require. regards ronnie sahlberg
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
I tested this patch with the following command: x86_64-softmmu/qemu-system-x86_64 --enable-kvm rhel54_1.img -m 1024 -net tap,ifname=tap0,script=no -net nic,model=virtio -sdl -drive file=iscsi://127.0.0.1/iqn.2011-09.com.example:server.target1/ And I found that the whole qemu process would get freezed, not reachable via ping and no response on desktop if there's I/O targeted to the iscsi drive and the iscsi target was forcefully stopped. After checking the backtrace with gdb, I found the I/O thread got stuck on the mutex qemu_global_mutex , which was hold by the vcpu thread. It should be released before re-entering guest. But the vcpu thread was waiting for the completion of iscsi aio request endlessly, and therefore couldn't get chance to release the mutex. So the whole qemu process became unresponsive. But this problem doesn't exist with the combination of virtio and iscsi. Only the I/O process got hung on guest in this case. It's more acceptable. I am not sure how to fix this problem. gdb backtrace: (gdb) info threads 2 Thread 0x7fa0fdd4c700 (LWP 5086) 0x003a868de383 in select () from /lib64/libc.so.6 * 1 Thread 0x7fa0fdd4d740 (LWP 5085) 0x003a8700dfe4 in __lll_lock_wait () from /lib64/libpthread.so.0 (gdb) bt #0 0x003a8700dfe4 in __lll_lock_wait () from /lib64/libpthread.so.0 #1 0x003a87009318 in _L_lock_854 () from /lib64/libpthread.so.0 #2 0x003a870091e7 in pthread_mutex_lock () from /lib64/libpthread.so.0 #3 0x004c9819 in qemu_mutex_lock (mutex=) at qemu-thread-posix.c:54 #4 0x004a46c6 in main_loop_wait (nonblocking=out>) at /home/mark/Work/source/qemu/vl.c:1545 #5 0x004a60d6 in main_loop (argc=, argv=, envp=) at /home/mark/Work/source/qemu/vl.c:1579 #6 main (argc=, argv=, envp=) at /home/mark/Work/source/qemu/vl.c:3574 (gdb) t 2 [Switching to thread 2 (Thread 0x7fa0fdd4c700 (LWP 5086))]#0 0x003a868de383 in select () from /lib64/libc.so.6 (gdb) bt #0 0x003a868de383 in select () from /lib64/libc.so.6 #1 0x004096aa in qemu_aio_wait () at aio.c:193 #2 0x00409815 in qemu_aio_flush () at aio.c:113 #3 0x004761ea in bmdma_cmd_writeb (bm=0x1db2230, val=8) at /home/mark/Work/source/qemu/hw/ide/pci.c:311 #4 0x00555900 in access_with_adjusted_size (addr=0, value=0x7fa0fdd4bdb8, size=1, access_size_min=, access_size_max=, access= 0x555820 , opaque=0x1db2370) at /home/mark/Work/source/qemu/memory.c:284 #5 0x00555ae1 in memory_region_iorange_write (iorange=optimized out>, offset=, width=out>, data=8) at /home/mark/Work/source/qemu/memory.c:425 #6 0x0054eda1 in kvm_handle_io (env=0x192e080) at /home/mark/Work/source/qemu/kvm-all.c:834 #7 kvm_cpu_exec (env=0x192e080) at /home/mark/Work/source/qemu/kvm-all.c:976 #8 0x0052cc1a in qemu_kvm_cpu_thread_fn (arg=0x192e080) at /home/mark/Work/source/qemu/cpus.c:656 #9 0x003a870077e1 in start_thread () from /lib64/libpthread.so.0 #10 0x003a868e577d in clone () from /lib64/libc.so.6
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/23/2011 11:15 AM, Mark Wu wrote: I tested this patch with the following command: x86_64-softmmu/qemu-system-x86_64 --enable-kvm rhel54_1.img -m 1024 -net tap,ifname=tap0,script=no -net nic,model=virtio -sdl -drive file=iscsi://127.0.0.1/iqn.2011-09.com.example:server.target1/ And I found that the whole qemu process would get freezed, not reachable via ping and no response on desktop if there's I/O targeted to the iscsi drive and the iscsi target was forcefully stopped. After checking the backtrace with gdb, I found the I/O thread got stuck on the mutex qemu_global_mutex , which was hold by the vcpu thread. It should be released before re-entering guest. But the vcpu thread was waiting for the completion of iscsi aio request endlessly, and therefore couldn't get chance to release the mutex. So the whole qemu process became unresponsive. But this problem doesn't exist with the combination of virtio and iscsi. Only the I/O process got hung on guest in this case. It's more acceptable. I am not sure how to fix this problem. You don't. :( It's a problem in IDE emulation and anything else that uses qemu_aio_flush; luckily it's only called in a few places. It would be the same with NFS instead of iSCSI, for example. Otherwise, you could implement some kind of timeout+reconnect logic in the iSCSI driver or in libiscsi. Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
List, What remains before this patch can be accepted? Previous patch received good feedback and severa people indicated that they would find the feature useful for several use cases. regards ronnie sahlberg On Wed, Sep 21, 2011 at 7:52 PM, ronnie sahlberg wrote: > On Wed, Sep 21, 2011 at 7:45 PM, Paolo Bonzini wrote: >> On 09/21/2011 11:37 AM, Ronnie Sahlberg wrote: >>> >>> This driver interfaces with the multiplatform posix library for iscsi >>> initiator/client access to iscsi devices hosted at >>> git://github.com/sahlberg/libiscsi.git >> >> Do you plan to make a stable release, so that it can be packaged e.g. in >> Fedora? > > Yes. > > As soon as my primary consumer (==QEMU) has integrated the patch I > will make a stable release for libiscsi to match. > Until then I keep it "unstable" so I can implement what changes that > the QEMU iscsi patch may require. > > regards > ronnie sahlberg >
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/27/2011 10:08 PM, ronnie sahlberg wrote: List, What remains before this patch can be accepted? Previous patch received good feedback and severa people indicated that they would find the feature useful for several use cases. Kevin is on vacation this week. :) Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Wed, Sep 21, 2011 at 07:37:55PM +1000, Ronnie Sahlberg wrote: > This provides built-in support for iSCSI to QEMU. > This has the advantage that the iSCSI devices need not be made visible to the > host, which is useful if you have very many virtual machines and very many > iscsi devices. > It also has the benefit that non-root users of QEMU can access iSCSI devices > across the network without requiring root privilege on the host. > > This driver interfaces with the multiplatform posix library for iscsi > initiator/client access to iscsi devices hosted at > git://github.com/sahlberg/libiscsi.git > > The patch adds the driver to interface with the iscsi library. > It also updated the configure script to > * by default, probe is libiscsi is available and if so, build > qemu against libiscsi. > * --enable-libiscsi > Force a build against libiscsi. If libiscsi is not available > the build will fail. > * --disable-libiscsi > Do not link against libiscsi, even if it is available. > > When linked with libiscsi, qemu gains support to access iscsi resources such > as disks and cdrom directly, without having to make the devices visible to > the host. > > You can specify devices using a iscsi url of the form : > iscsi://[[:@]][:/ > When using authentication, the password can optionally be set with > LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list > > Signed-off-by: Ronnie Sahlberg > --- > Makefile.objs |1 + > block/iscsi.c | 596 > + > configure | 31 +++ > trace-events |7 + > 4 files changed, 635 insertions(+), 0 deletions(-) > create mode 100644 block/iscsi.c Reviewed-by: Stefan Hajnoczi
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
ping? On Thu, Sep 29, 2011 at 4:54 PM, Stefan Hajnoczi wrote: > On Wed, Sep 21, 2011 at 07:37:55PM +1000, Ronnie Sahlberg wrote: >> This provides built-in support for iSCSI to QEMU. >> This has the advantage that the iSCSI devices need not be made visible to >> the host, which is useful if you have very many virtual machines and very >> many iscsi devices. >> It also has the benefit that non-root users of QEMU can access iSCSI devices >> across the network without requiring root privilege on the host. >> >> This driver interfaces with the multiplatform posix library for iscsi >> initiator/client access to iscsi devices hosted at >> git://github.com/sahlberg/libiscsi.git >> >> The patch adds the driver to interface with the iscsi library. >> It also updated the configure script to >> * by default, probe is libiscsi is available and if so, build >> qemu against libiscsi. >> * --enable-libiscsi >> Force a build against libiscsi. If libiscsi is not available >> the build will fail. >> * --disable-libiscsi >> Do not link against libiscsi, even if it is available. >> >> When linked with libiscsi, qemu gains support to access iscsi resources such >> as disks and cdrom directly, without having to make the devices visible to >> the host. >> >> You can specify devices using a iscsi url of the form : >> iscsi://[[:@]][:/ >> When using authentication, the password can optionally be set with >> LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list >> >> Signed-off-by: Ronnie Sahlberg >> --- >> Makefile.objs | 1 + >> block/iscsi.c | 596 >> + >> configure | 31 +++ >> trace-events | 7 + >> 4 files changed, 635 insertions(+), 0 deletions(-) >> create mode 100644 block/iscsi.c > > Reviewed-by: Stefan Hajnoczi >
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Previous version of the patch received very positive feedback and several expressed seeing positive value of a built-in initiator. I updated patch from feedback 3 weeks ago and Stefan kindly reviewed it. Is there some other problem with the patch I am not aware of that I should address? I have been trying to push this patch in different versions since December last year. There is obviously a problem here I am not aware of. Please advice what the problem is and I will try to rectify it. Please advice on how I can move forward. I feel a bit at roads end here. Please help. regards ronnie sahlberg On Mon, Oct 10, 2011 at 7:46 AM, ronnie sahlberg wrote: > ping? > > > On Thu, Sep 29, 2011 at 4:54 PM, Stefan Hajnoczi > wrote: >> On Wed, Sep 21, 2011 at 07:37:55PM +1000, Ronnie Sahlberg wrote: >>> This provides built-in support for iSCSI to QEMU. >>> This has the advantage that the iSCSI devices need not be made visible to >>> the host, which is useful if you have very many virtual machines and very >>> many iscsi devices. >>> It also has the benefit that non-root users of QEMU can access iSCSI >>> devices across the network without requiring root privilege on the host. >>> >>> This driver interfaces with the multiplatform posix library for iscsi >>> initiator/client access to iscsi devices hosted at >>> git://github.com/sahlberg/libiscsi.git >>> >>> The patch adds the driver to interface with the iscsi library. >>> It also updated the configure script to >>> * by default, probe is libiscsi is available and if so, build >>> qemu against libiscsi. >>> * --enable-libiscsi >>> Force a build against libiscsi. If libiscsi is not available >>> the build will fail. >>> * --disable-libiscsi >>> Do not link against libiscsi, even if it is available. >>> >>> When linked with libiscsi, qemu gains support to access iscsi resources >>> such as disks and cdrom directly, without having to make the devices >>> visible to the host. >>> >>> You can specify devices using a iscsi url of the form : >>> iscsi://[[:@]][:/ >>> When using authentication, the password can optionally be set with >>> LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list >>> >>> Signed-off-by: Ronnie Sahlberg >>> --- >>> Makefile.objs | 1 + >>> block/iscsi.c | 596 >>> + >>> configure | 31 +++ >>> trace-events | 7 + >>> 4 files changed, 635 insertions(+), 0 deletions(-) >>> create mode 100644 block/iscsi.c >> >> Reviewed-by: Stefan Hajnoczi >> >
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 10/13/2011 11:46 AM, ronnie sahlberg wrote: Previous version of the patch received very positive feedback and several expressed seeing positive value of a built-in initiator. I updated patch from feedback 3 weeks ago and Stefan kindly reviewed it. Is there some other problem with the patch I am not aware of that I should address? I have been trying to push this patch in different versions since December last year. There is obviously a problem here I am not aware of. Please advice what the problem is and I will try to rectify it. The problem is just that everyone is busy. :) The patch looks good to me too. Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Oct 13, 2011 at 10:46 AM, ronnie sahlberg wrote: > Previous version of the patch received very positive feedback and > several expressed seeing positive value of a built-in initiator. > I updated patch from feedback 3 weeks ago and Stefan kindly reviewed it. I'm happy. Stefan
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Oct 13, 2011 at 08:46:54PM +1100, ronnie sahlberg wrote: > Previous version of the patch received very positive feedback and > several expressed seeing positive value of a built-in initiator. > I updated patch from feedback 3 weeks ago and Stefan kindly reviewed it. > > > Is there some other problem with the patch I am not aware of that I > should address? > > I have been trying to push this patch in different versions since > December last year. > There is obviously a problem here I am not aware of. > Please advice what the problem is and I will try to rectify it. > > > Please advice on how I can move forward. I feel a bit at roads end > here. Please help. I can't comment much on the code, but I'm supportive of QEMU gaining this feature, because it addresses a number of use cases not satisfied by using iSCSI via the host OS's block layer. > >>> You can specify devices using a iscsi url of the form : > >>> iscsi://[[:@]][:/ > >>> When using authentication, the password can optionally be set with > >>> LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process > >>> list I'm not a fan of sending passwords via command line args, or environment variables. Env variables *can* be exposed via the process list, albeit not to unprivileged users. More critically, env variables will end up in logfiles like /var/log/libvirt/qemu/$GUESTNAME.log, and in data reported to distro bug trackers, via tools like sosreport which capture /proc/$PID/environ and aforementioned logfiles. We have a similar requirement for specifying passwords with the Ceph/RBD driver, and also for the curl/HTTP block drivers. We have a monitor command for providing decryption passwords for QCow2 disks. We could either reuse that for connection passwords, or perhaps slightly better would be to have a separate command for connection passwords. 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 :|
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Am 13.10.2011 11:46, schrieb ronnie sahlberg: > Previous version of the patch received very positive feedback and > several expressed seeing positive value of a built-in initiator. > I updated patch from feedback 3 weeks ago and Stefan kindly reviewed it. > > > Is there some other problem with the patch I am not aware of that I > should address? No problems that I know of, other than me being on vacation in the last two weeks. I'm planning to have another quick look before I merge it, but Stefan's ack suggests that it's ready to be merged and your part is done. Kevin
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Oct 13, 2011 at 11:01:49AM +0100, Daniel P. Berrange wrote: > On Thu, Oct 13, 2011 at 08:46:54PM +1100, ronnie sahlberg wrote: > > Previous version of the patch received very positive feedback and > > several expressed seeing positive value of a built-in initiator. > > I updated patch from feedback 3 weeks ago and Stefan kindly reviewed it. > > > > > > Is there some other problem with the patch I am not aware of that I > > should address? > > > > I have been trying to push this patch in different versions since > > December last year. > > There is obviously a problem here I am not aware of. > > Please advice what the problem is and I will try to rectify it. > > > > > > Please advice on how I can move forward. I feel a bit at roads end > > here. Please help. > > I can't comment much on the code, but I'm supportive of QEMU gaining > this feature, because it addresses a number of use cases not satisfied > by using iSCSI via the host OS's block layer. > > > > >>> You can specify devices using a iscsi url of the form : > > >>> iscsi://[[:@]][:/ > > >>> When using authentication, the password can optionally be set with > > >>> LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process > > >>> list > > I'm not a fan of sending passwords via command line args, or > environment variables. Env variables *can* be exposed via > the process list, albeit not to unprivileged users. More > critically, env variables will end up in logfiles like > /var/log/libvirt/qemu/$GUESTNAME.log, and in data reported > to distro bug trackers, via tools like sosreport which > capture /proc/$PID/environ and aforementioned logfiles. > > We have a similar requirement for specifying passwords with > the Ceph/RBD driver, and also for the curl/HTTP block drivers. > We have a monitor command for providing decryption passwords for > QCow2 disks. We could either reuse that for connection passwords, > or perhaps slightly better would be to have a separate command > for connection passwords. NB, I didn't mean to suggest that this issue should block merging of this iSCSI driver. The problem with passwords already exists for Ceph & Curl drivers, and I believe the Ceph developers are already working on a patch for QEMU which should be able to apply to all these network block devs 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 :|
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Am 21.09.2011 11:37, schrieb Ronnie Sahlberg:
> This provides built-in support for iSCSI to QEMU.
> This has the advantage that the iSCSI devices need not be made visible to the
> host, which is useful if you have very many virtual machines and very many
> iscsi devices.
> It also has the benefit that non-root users of QEMU can access iSCSI devices
> across the network without requiring root privilege on the host.
>
> This driver interfaces with the multiplatform posix library for iscsi
> initiator/client access to iscsi devices hosted at
> git://github.com/sahlberg/libiscsi.git
>
> The patch adds the driver to interface with the iscsi library.
> It also updated the configure script to
> * by default, probe is libiscsi is available and if so, build
> qemu against libiscsi.
> * --enable-libiscsi
> Force a build against libiscsi. If libiscsi is not available
> the build will fail.
> * --disable-libiscsi
> Do not link against libiscsi, even if it is available.
>
> When linked with libiscsi, qemu gains support to access iscsi resources such
> as disks and cdrom directly, without having to make the devices visible to
> the host.
>
> You can specify devices using a iscsi url of the form :
> iscsi://[[:@]][:/
> When using authentication, the password can optionally be set with
> LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list
>
> Signed-off-by: Ronnie Sahlberg
I have a few comments, but none of them are really critical and in
general the patch looks good to me.
So please let me know if you intend to send another version with the
comments addressed or if I should commit this version and you'll send
fixes/cleanups on top. I don't really mind at this point.
> ---
> Makefile.objs |1 +
> block/iscsi.c | 596
> +
> configure | 31 +++
> trace-events |7 +
> 4 files changed, 635 insertions(+), 0 deletions(-)
> create mode 100644 block/iscsi.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a529a11..8c8e420 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -36,6 +36,7 @@ block-nested-y += qed-check.o
> block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> block-nested-$(CONFIG_WIN32) += raw-win32.o
> block-nested-$(CONFIG_POSIX) += raw-posix.o
> +block-nested-$(CONFIG_LIBISCSI) += iscsi.o
> block-nested-$(CONFIG_CURL) += curl.o
> block-nested-$(CONFIG_RBD) += rbd.o
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> new file mode 100644
> index 000..6517576
> --- /dev/null
> +++ b/block/iscsi.c
> @@ -0,0 +1,596 @@
> +/*
> + * QEMU Block driver for iSCSI images
> + *
> + * Copyright (c) 2010-2011 Ronnie Sahlberg
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "config-host.h"
> +
> +#include
> +#include "sysemu.h"
I doubt that you really need sysemu.h. If you did, I think you couldn't
successfully build qemu-img and qemu-io.
> +#include "qemu-common.h"
> +#include "qemu-error.h"
> +#include "block_int.h"
> +#include "trace.h"
> +
> +#include
> +#include
> +
> +
> +typedef struct IscsiLun {
> +struct iscsi_context *iscsi;
> +int lun;
> +int block_size;
> +unsigned long num_blocks;
> +} IscsiLun;
> +
> +typedef struct IscsiAIOCB {
> +BlockDriverAIOCB common;
> +QEMUIOVector *qiov;
> +QEMUBH *bh;
> +IscsiLun *iscsilun;
> +struct scsi_task *task;
> +uint8_t *buf;
> +int canceled;
> +int status;
> +size_t read_size;
> +size_t read_offset;
> +} IscsiAIOCB;
> +
> +struct IscsiTask {
> +IscsiLun *iscsilun;
> +int status;
> +int complete;
> +};
> +
> +static void
> +iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void
> *command_data,
> +void *private_data)
> +{
> +}
> +
> +static void
> +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +IscsiAIOCB *acb =
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Kevin,
Thanks for the review,
I have attached a modified patch that addresses all your comments.
Please review and/or apply.
> I doubt that you really need sysemu.h. If you did, I think you couldn't
> successfully build qemu-img and qemu-io.
You are right. I removed it.
> Is acb->status == -ECANCELED redundant with acb->canceled == 1?
Fixed.
> qemu_aio_get() never returns NULL, so this check is unnecessary.
Done.
> I think you should also set bs->total_sectors.
Done.
> iscsi_destroy_url() is only ever called here. Do we leak it in the normal
> path?
It didnt leak but the the label was confusing. I have changed it from
"failed" to "finished" since this part
is executed for both normal and failures.
Thanks
Ronnie Sahlberg
From f5fc5880a3071188dbfec903bc1ea1247a1e375c Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg
Date: Tue, 25 Oct 2011 19:03:18 +1100
Subject: [PATCH] This provides built-in support for iSCSI to QEMU.
This has the advantage that the iSCSI devices need not be made visible to the host, which is useful if you have very many virtual machines and very many iscsi devices.
It also has the benefit that non-root users of QEMU can access iSCSI devices across the network without requiring root privilege on the host.
This driver interfaces with the multiplatform posix library for iscsi initiator/client access to iscsi devices hosted at
git://github.com/sahlberg/libiscsi.git
The patch adds the driver to interface with the iscsi library.
It also updated the configure script to
* by default, probe is libiscsi is available and if so, build
qemu against libiscsi.
* --enable-libiscsi
Force a build against libiscsi. If libiscsi is not available
the build will fail.
* --disable-libiscsi
Do not link against libiscsi, even if it is available.
When linked with libiscsi, qemu gains support to access iscsi resources such as disks and cdrom directly, without having to make the devices visible to the host.
You can specify devices using a iscsi url of the form :
iscsi://[[:@]][:/
When using authentication, the password can optionally be set with
LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list
Signed-off-by: Ronnie Sahlberg
---
Makefile.objs |1 +
block/iscsi.c | 589 +
configure | 31 +++
trace-events |6 +
4 files changed, 627 insertions(+), 0 deletions(-)
create mode 100644 block/iscsi.c
diff --git a/Makefile.objs b/Makefile.objs
index 01587c8..d18417c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -36,6 +36,7 @@ block-nested-y += qed-check.o
block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
block-nested-$(CONFIG_WIN32) += raw-win32.o
block-nested-$(CONFIG_POSIX) += raw-posix.o
+block-nested-$(CONFIG_LIBISCSI) += iscsi.o
block-nested-$(CONFIG_CURL) += curl.o
block-nested-$(CONFIG_RBD) += rbd.o
diff --git a/block/iscsi.c b/block/iscsi.c
new file mode 100644
index 000..b105ab7
--- /dev/null
+++ b/block/iscsi.c
@@ -0,0 +1,589 @@
+/*
+ * QEMU Block driver for iSCSI images
+ *
+ * Copyright (c) 2010-2011 Ronnie Sahlberg
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "config-host.h"
+
+#include
+#include "qemu-common.h"
+#include "qemu-error.h"
+#include "block_int.h"
+#include "trace.h"
+
+#include
+#include
+
+
+typedef struct IscsiLun {
+struct iscsi_context *iscsi;
+int lun;
+int block_size;
+unsigned long num_blocks;
+} IscsiLun;
+
+typedef struct IscsiAIOCB {
+BlockDriverAIOCB common;
+QEMUIOVector *qiov;
+QEMUBH *bh;
+IscsiLun *iscsilun;
+struct scsi_task *task;
+uint8_t *buf;
+int status;
+int canceled;
+size_t read_size;
+size_t read_offset;
+} IscsiAIOCB;
+
+struct IscsiTask {
+IscsiLun *iscsilun;
+BlockDriverState *bs;
+int status;
+int complete;
+};
+
+static void
+iscsi_abort_task_cb(struct iscsi_context *iscsi, int status
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Am 25.10.2011 10:04, schrieb ronnie sahlberg: >> iscsi_destroy_url() is only ever called here. Do we leak it in the normal >> path? > > It didnt leak but the the label was confusing. I have changed it from > "failed" to "finished" since this part > is executed for both normal and failures. But there's a return 0; immediately before the label? Kevin
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Hi,
On Tue, Oct 25, 2011 at 7:17 PM, Kevin Wolf wrote:
> Am 25.10.2011 10:04, schrieb ronnie sahlberg:
>>> iscsi_destroy_url() is only ever called here. Do we leak it in the normal
>>> path?
>>
>> It didnt leak but the the label was confusing. I have changed it from
>> "failed" to "finished" since this part
>> is executed for both normal and failures.
>
> But there's a return 0; immediately before the label?
Right. Its been a long day :-)
Please find attached a fixed version.
regards
ronnie sahlberg
>
> Kevin
>
From 02c729b92d8fec01a04669a6dca815db2a5dc35b Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg
Date: Tue, 25 Oct 2011 19:24:24 +1100
Subject: [PATCH] This provides built-in support for iSCSI to QEMU.
This has the advantage that the iSCSI devices need not be made visible to the host, which is useful if you have very many virtual machines and very many iscsi devices.
It also has the benefit that non-root users of QEMU can access iSCSI devices across the network without requiring root privilege on the host.
This driver interfaces with the multiplatform posix library for iscsi initiator/client access to iscsi devices hosted at
git://github.com/sahlberg/libiscsi.git
The patch adds the driver to interface with the iscsi library.
It also updated the configure script to
* by default, probe is libiscsi is available and if so, build
qemu against libiscsi.
* --enable-libiscsi
Force a build against libiscsi. If libiscsi is not available
the build will fail.
* --disable-libiscsi
Do not link against libiscsi, even if it is available.
When linked with libiscsi, qemu gains support to access iscsi resources such as disks and cdrom directly, without having to make the devices visible to the host.
You can specify devices using a iscsi url of the form :
iscsi://[[:@]][:/
When using authentication, the password can optionally be set with
LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list
Signed-off-by: Ronnie Sahlberg
---
Makefile.objs |1 +
block/iscsi.c | 592 +
configure | 31 +++
trace-events |6 +
4 files changed, 630 insertions(+), 0 deletions(-)
create mode 100644 block/iscsi.c
diff --git a/Makefile.objs b/Makefile.objs
index 01587c8..d18417c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -36,6 +36,7 @@ block-nested-y += qed-check.o
block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
block-nested-$(CONFIG_WIN32) += raw-win32.o
block-nested-$(CONFIG_POSIX) += raw-posix.o
+block-nested-$(CONFIG_LIBISCSI) += iscsi.o
block-nested-$(CONFIG_CURL) += curl.o
block-nested-$(CONFIG_RBD) += rbd.o
diff --git a/block/iscsi.c b/block/iscsi.c
new file mode 100644
index 000..010a9f6
--- /dev/null
+++ b/block/iscsi.c
@@ -0,0 +1,592 @@
+/*
+ * QEMU Block driver for iSCSI images
+ *
+ * Copyright (c) 2010-2011 Ronnie Sahlberg
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "config-host.h"
+
+#include
+#include "qemu-common.h"
+#include "qemu-error.h"
+#include "block_int.h"
+#include "trace.h"
+
+#include
+#include
+
+
+typedef struct IscsiLun {
+struct iscsi_context *iscsi;
+int lun;
+int block_size;
+unsigned long num_blocks;
+} IscsiLun;
+
+typedef struct IscsiAIOCB {
+BlockDriverAIOCB common;
+QEMUIOVector *qiov;
+QEMUBH *bh;
+IscsiLun *iscsilun;
+struct scsi_task *task;
+uint8_t *buf;
+int status;
+int canceled;
+size_t read_size;
+size_t read_offset;
+} IscsiAIOCB;
+
+struct IscsiTask {
+IscsiLun *iscsilun;
+BlockDriverState *bs;
+int status;
+int complete;
+};
+
+static void
+iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
+void *private_data)
+{
+}
+
+static void
+iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
+IscsiLun *iscsilun = acb->iscsilun;
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Am 25.10.2011 10:23, schrieb ronnie sahlberg: > Hi, > > > On Tue, Oct 25, 2011 at 7:17 PM, Kevin Wolf wrote: >> Am 25.10.2011 10:04, schrieb ronnie sahlberg: iscsi_destroy_url() is only ever called here. Do we leak it in the normal path? >>> >>> It didnt leak but the the label was confusing. I have changed it from >>> "failed" to "finished" since this part >>> is executed for both normal and failures. >> >> But there's a return 0; immediately before the label? > > Right. Its been a long day :-) > > Please find attached a fixed version. Thanks, applied to the block branch. Kevin
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Wed, Sep 21, 2011 at 5:37 PM, Ronnie Sahlberg
wrote:
> This provides built-in support for iSCSI to QEMU.
> This has the advantage that the iSCSI devices need not be made visible to the
> host, which is useful if you have very many virtual machines and very many
> iscsi devices.
> It also has the benefit that non-root users of QEMU can access iSCSI devices
> across the network without requiring root privilege on the host.
>
> This driver interfaces with the multiplatform posix library for iscsi
> initiator/client access to iscsi devices hosted at
> git://github.com/sahlberg/libiscsi.git
>
> The patch adds the driver to interface with the iscsi library.
> It also updated the configure script to
> * by default, probe is libiscsi is available and if so, build
> qemu against libiscsi.
> * --enable-libiscsi
> Force a build against libiscsi. If libiscsi is not available
> the build will fail.
> * --disable-libiscsi
> Do not link against libiscsi, even if it is available.
>
> When linked with libiscsi, qemu gains support to access iscsi resources such
> as disks and cdrom directly, without having to make the devices visible to
> the host.
>
> You can specify devices using a iscsi url of the form :
> iscsi://[[:@]][:/
> When using authentication, the password can optionally be set with
> LIBISCSI_CHAP_PASSWORD="password" to avoid it showing up in the process list
>
> Signed-off-by: Ronnie Sahlberg
> ---
> Makefile.objs | 1 +
> block/iscsi.c | 596
> +
> configure | 31 +++
> trace-events | 7 +
> 4 files changed, 635 insertions(+), 0 deletions(-)
> create mode 100644 block/iscsi.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a529a11..8c8e420 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -36,6 +36,7 @@ block-nested-y += qed-check.o
> block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> block-nested-$(CONFIG_WIN32) += raw-win32.o
> block-nested-$(CONFIG_POSIX) += raw-posix.o
> +block-nested-$(CONFIG_LIBISCSI) += iscsi.o
> block-nested-$(CONFIG_CURL) += curl.o
> block-nested-$(CONFIG_RBD) += rbd.o
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> new file mode 100644
> index 000..6517576
> --- /dev/null
> +++ b/block/iscsi.c
> @@ -0,0 +1,596 @@
> +/*
> + * QEMU Block driver for iSCSI images
> + *
> + * Copyright (c) 2010-2011 Ronnie Sahlberg
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "config-host.h"
> +
> +#include
> +#include "sysemu.h"
> +#include "qemu-common.h"
> +#include "qemu-error.h"
> +#include "block_int.h"
> +#include "trace.h"
> +
> +#include
> +#include
> +
> +
> +typedef struct IscsiLun {
> + struct iscsi_context *iscsi;
> + int lun;
> + int block_size;
> + unsigned long num_blocks;
> +} IscsiLun;
> +
> +typedef struct IscsiAIOCB {
> + BlockDriverAIOCB common;
> + QEMUIOVector *qiov;
> + QEMUBH *bh;
> + IscsiLun *iscsilun;
> + struct scsi_task *task;
> + uint8_t *buf;
> + int canceled;
> + int status;
> + size_t read_size;
> + size_t read_offset;
> +} IscsiAIOCB;
> +
> +struct IscsiTask {
> + IscsiLun *iscsilun;
> + int status;
> + int complete;
> +};
> +
> +static void
> +iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void
> *command_data,
> + void *private_data)
> +{
> +}
> +
> +static void
> +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> + IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
> + IscsiLun *iscsilun = acb->iscsilun;
> +
> + acb->status = -ECANCELED;
> + acb->common.cb(acb->common.opaque, acb->status);
> + acb->canceled = 1;
> +
> + /* send a task mgmt call to the target to cancel the task on the target
> */
> + iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> + iscsi_abort_task_cb, NULL);
> +
> +
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Sat, Sep 10, 2011 at 02:23:30PM +1000, Ronnie Sahlberg wrote:
Looking good. I think this is worth merging because it does offer
benefits over host iSCSI.
> +static void
> +iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void
> *command_data,
> +void *private_data)
> +{
> +}
> +
> +static void
> +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
> +IscsiLun *iscsilun = acb->iscsilun;
> +
> +acb->status = -ECANCELED;
> +acb->common.cb(acb->common.opaque, acb->status);
> +acb->canceled = 1;
> +
> +iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> + iscsi_abort_task_cb, NULL);
> +}
The asynchronous abort task call looks odd. If a caller allocates a
buffer and issues a read request, then we need to make sure that the
request is really aborted by the time .bdrv_aio_cancel() returns.
If I understand the code correctly, iscsi_aio_cancel() returns
immediately but the read request will still be in progress. That means
the caller could now free their data buffer and the read request will
overwrite that unallocated memory.
> +static void
> +iscsi_aio_write10_cb(struct iscsi_context *iscsi, int status,
> + void *command_data, void *opaque)
> +{
> +IscsiAIOCB *acb = opaque;
> +
> +trace_iscsi_aio_write10_cb(iscsi, status, acb, acb->canceled);
> +
> +if (acb->buf != NULL) {
> +free(acb->buf);
> +}
Please just use g_free(acb->buf). g_free(NULL) is defined as a nop so
the check isn't necessary. Also, this code uses free(3) when it should
use g_free(3).
> +
> +if (acb->canceled != 0) {
> +qemu_aio_release(acb);
> +scsi_free_scsi_task(acb->task);
> +acb->task = NULL;
> +return;
> +}
> +
> +acb->status = 0;
> +if (status < 0) {
> +error_report("Failed to write10 data to iSCSI lun. %s",
> + iscsi_get_error(iscsi));
> +acb->status = -EIO;
> +}
> +
> +iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
> +scsi_free_scsi_task(acb->task);
> +acb->task = NULL;
> +}
> +
> +static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
> +{
> +return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
> +}
> +
> +static BlockDriverAIOCB *
> +iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
> + QEMUIOVector *qiov, int nb_sectors,
> + BlockDriverCompletionFunc *cb,
> + void *opaque)
> +{
> +IscsiLun *iscsilun = bs->opaque;
> +struct iscsi_context *iscsi = iscsilun->iscsi;
> +IscsiAIOCB *acb;
> +size_t size;
> +int fua = 0;
> +
> +/* set FUA on writes when cache mode is write through */
> +if (!(bs->open_flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))) {
> +fua = 1;
> +}
FUA needs to reflect the guest semantics - does this disk have an
enabled write cache? When bs->open_flags has BDRV_O_CACHE_WB, then the
guest knows it needs to send flushes because there is a write cache:
if (!(bs->open_flags & BDRV_O_CACHE_WB)) {
fua = 1;
}
BDRV_O_NOCACHE is just for local files and sets the O_DIRECT hint. It
doesn't affect the cache semantics that the guest sees.
> +/*
> + * We support iscsi url's on the form
> + * iscsi://[%@][:]//
> + */
> +static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
> +{
> +IscsiLun *iscsilun = bs->opaque;
> +struct iscsi_context *iscsi = NULL;
> +struct iscsi_url *iscsi_url = NULL;
> +struct IscsiTask task;
> +int ret;
> +
> +if ((BDRV_SECTOR_SIZE % 512) != 0) {
> +error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
> + "BDRV_SECTOR_SIZE(%lld) is not a multiple "
> + "of 512", BDRV_SECTOR_SIZE);
> +return -EINVAL;
> +}
Another way of saying this is:
QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE % 512 != 0);
The advantage is that the build fails instead of waiting until iscsi is
used at runtime until the failure is detected.
What will happen if BDRV_SECTOR_SIZE is not a multiple of 512?
> +
> +memset(iscsilun, 0, sizeof(IscsiLun));
> +
> +/* Should really append the KVM name after the ':' here */
> +iscsi = iscsi_create_context("iqn.2008-11.org.linux-kvm:");
> +if (iscsi == NULL) {
> +error_report("iSCSI: Failed to create iSCSI context.");
> +ret = -ENOMEM;
> +goto failed;
> +}
> +
> +iscsi_url = iscsi_parse_full_url(iscsi, filename);
> +if (iscsi_url == NULL) {
> +error_report("Failed to parse URL : %s %s", filename,
> + iscsi_get_error(iscsi));
> +ret = -ENOMEM;
-EINVAL?
> +static BlockDriver bdrv_iscsi = {
> +.format_name = "iscsi",
> +.protocol_name = "iscsi",
> +
> +.instance_size = sizeof(IscsiLun),
> +.bdrv_file_open = iscsi_open,
> +.bdrv_close = iscsi_close,
> +
> +.bdrv_getlength
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Mon, Sep 12, 2011 at 10:14:08AM +0100, Stefan Hajnoczi wrote:
> > +static void
> > +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
> > +{
> > +IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
> > +IscsiLun *iscsilun = acb->iscsilun;
> > +
> > +acb->status = -ECANCELED;
> > +acb->common.cb(acb->common.opaque, acb->status);
> > +acb->canceled = 1;
> > +
> > +iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> > + iscsi_abort_task_cb, NULL);
> > +}
>
> The asynchronous abort task call looks odd. If a caller allocates a
> buffer and issues a read request, then we need to make sure that the
> request is really aborted by the time .bdrv_aio_cancel() returns.
Shouldn't new drivers use coroutines instead of aio instead?
(just poking around, I don't fully understand the new scheme myself yet
either)
> BDRV_O_NOCACHE is just for local files and sets the O_DIRECT hint. It
> doesn't affect the cache semantics that the guest sees.
Now that I fixed it it doesn't. But that's been a fairly recent change,
which isn't always easy to pick up people having external patches.
>
> > +/*
> > + * We support iscsi url's on the form
> > + * iscsi://[%@][:]//
> > + */
Is having username + password on the command line really a that good idea?
Also what about the more complicated iSCSI authentification schemes?
> What will happen if BDRV_SECTOR_SIZE is not a multiple of 512?
hell will break lose for all of qemu.
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Wed, Sep 14, 2011 at 3:36 PM, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 10:14:08AM +0100, Stefan Hajnoczi wrote:
>> > +static void
>> > +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>> > +{
>> > + IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
>> > + IscsiLun *iscsilun = acb->iscsilun;
>> > +
>> > + acb->status = -ECANCELED;
>> > + acb->common.cb(acb->common.opaque, acb->status);
>> > + acb->canceled = 1;
>> > +
>> > + iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
>> > + iscsi_abort_task_cb, NULL);
>> > +}
>>
>> The asynchronous abort task call looks odd. If a caller allocates a
>> buffer and issues a read request, then we need to make sure that the
>> request is really aborted by the time .bdrv_aio_cancel() returns.
>
> Shouldn't new drivers use coroutines instead of aio instead?
>
> (just poking around, I don't fully understand the new scheme myself yet
> either)
I think in this case it will not make the code nicer. Since the
external iSCSI library is based on callbacks it would be necessary to
write the coroutines<->callbacks adapter functions. So for example,
the READ10 command would need a function that can be called in
coroutine context and yields while libiscsi does the I/O. When the
callback is invoked it will re-enter the coroutine.
The area where coroutines are useful in the block layer is for image
formats. We already have common coroutines<->callback adapter
functions in block.c so it's possible to write sequential code for
image formats. They only need access to block layer functions which
have already been adapted. But as soon as you interact with a
callback-based API from the coroutine, then you need to write an
adapter yourself.
Stefan
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Sep 15, 2011 at 12:36 AM, Christoph Hellwig wrote: ... >> > +/* >> > + * We support iscsi url's on the form >> > + * iscsi://[%@][:]// >> > + */ > > Is having username + password on the command line really a that good idea? > Also what about the more complicated iSCSI authentification schemes? In general it is a very bad idea. For local use on a private box it is convenient to be able to use "%@" syntax. For use on a shared box, libiscsi supports an alternative method too by setting the username and/or password via environment variables : LIBISCSI_CHAP_USERNAME=... LIBISCSI_CHAP_PASSWORD=... I will document this better in the next patch. Libiscsi only support CHAP at this stage. Which other authentication schemes do you have in mind? Perhaps I can add them. regards ronnie sahlberg
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Stefan,
Thanks for your review.
I will do the changes you recommend and send an updated patch over the weekend.
Could you advice the best path for handling the .bdrv_flush and the
blocksize questions?
I think it is reasonable to just not support iscsi at all for
blocksize that is not multiple of 512 bytes
since a read-modify-write cycle across a network would probably be
prohibitively expensive.
.bdrv_flush() I can easily add a synchronous implementation of this
unless your patch is expected to be merged
in the near future.
regards
ronnie sahlberg
On Mon, Sep 12, 2011 at 7:14 PM, Stefan Hajnoczi
wrote:
> On Sat, Sep 10, 2011 at 02:23:30PM +1000, Ronnie Sahlberg wrote:
>
> Looking good. I think this is worth merging because it does offer
> benefits over host iSCSI.
>
>> +static void
>> +iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void
>> *command_data,
>> + void *private_data)
>> +{
>> +}
>> +
>> +static void
>> +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>> +{
>> + IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
>> + IscsiLun *iscsilun = acb->iscsilun;
>> +
>> + acb->status = -ECANCELED;
>> + acb->common.cb(acb->common.opaque, acb->status);
>> + acb->canceled = 1;
>> +
>> + iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
>> + iscsi_abort_task_cb, NULL);
>> +}
>
> The asynchronous abort task call looks odd. If a caller allocates a
> buffer and issues a read request, then we need to make sure that the
> request is really aborted by the time .bdrv_aio_cancel() returns.
>
> If I understand the code correctly, iscsi_aio_cancel() returns
> immediately but the read request will still be in progress. That means
> the caller could now free their data buffer and the read request will
> overwrite that unallocated memory.
>
Right.
I will need to remove the read pdu from the local initiator side too
so that if the read is in flight and we receive data for it, that this
data is discarded
and not written into the possibly no longer valid buffers.
I will do this change in the next version of the patch.
>> +static void
>> +iscsi_aio_write10_cb(struct iscsi_context *iscsi, int status,
>> + void *command_data, void *opaque)
>> +{
>> + IscsiAIOCB *acb = opaque;
>> +
>> + trace_iscsi_aio_write10_cb(iscsi, status, acb, acb->canceled);
>> +
>> + if (acb->buf != NULL) {
>> + free(acb->buf);
>> + }
>
> Please just use g_free(acb->buf). g_free(NULL) is defined as a nop so
> the check isn't necessary. Also, this code uses free(3) when it should
> use g_free(3).
Will do.
>
>> +
>> + if (acb->canceled != 0) {
>> + qemu_aio_release(acb);
>> + scsi_free_scsi_task(acb->task);
>> + acb->task = NULL;
>> + return;
>> + }
>> +
>> + acb->status = 0;
>> + if (status < 0) {
>> + error_report("Failed to write10 data to iSCSI lun. %s",
>> + iscsi_get_error(iscsi));
>> + acb->status = -EIO;
>> + }
>> +
>> + iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> + scsi_free_scsi_task(acb->task);
>> + acb->task = NULL;
>> +}
>> +
>> +static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
>> +{
>> + return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
>> +}
>> +
>> +static BlockDriverAIOCB *
>> +iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> + QEMUIOVector *qiov, int nb_sectors,
>> + BlockDriverCompletionFunc *cb,
>> + void *opaque)
>> +{
>> + IscsiLun *iscsilun = bs->opaque;
>> + struct iscsi_context *iscsi = iscsilun->iscsi;
>> + IscsiAIOCB *acb;
>> + size_t size;
>> + int fua = 0;
>> +
>> + /* set FUA on writes when cache mode is write through */
>> + if (!(bs->open_flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))) {
>> + fua = 1;
>> + }
>
> FUA needs to reflect the guest semantics - does this disk have an
> enabled write cache? When bs->open_flags has BDRV_O_CACHE_WB, then the
> guest knows it needs to send flushes because there is a write cache:
>
> if (!(bs->open_flags & BDRV_O_CACHE_WB)) {
> fua = 1;
> }
>
> BDRV_O_NOCACHE is just for local files and sets the O_DIRECT hint. It
> doesn't affect the cache semantics that the guest sees.
Thanks. I'll do this change.
>
>> +/*
>> + * We support iscsi url's on the form
>> + * iscsi://[%@][:]//
>> + */
>> +static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>> +{
>> + IscsiLun *iscsilun = bs->opaque;
>> + struct iscsi_context *iscsi = NULL;
>> + struct iscsi_url *iscsi_url = NULL;
>> + struct IscsiTask task;
>> + int ret;
>> +
>> + if ((BDRV_SECTOR_SIZE % 512) != 0) {
>> + error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. "
>> + "BDRV_SECTOR_SIZE(%lld) is not a multiple "
>> + "of 512", BDRV_SECTOR_SIZE);
>> + return -EINVAL;
>> + }
>
> Another way of sayin
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 01:08 AM, ronnie sahlberg wrote: I think it is reasonable to just not support iscsi at all for blocksize that is not multiple of 512 bytes since a read-modify-write cycle across a network would probably be prohibitively expensive. Agreed. .bdrv_flush() I can easily add a synchronous implementation of this unless your patch is expected to be merged in the near future. We need the same patch for NBD, so I wouldn't bother with the synchronous flush. Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Sep 15, 2011 at 08:51:00AM +1000, ronnie sahlberg wrote: > On Thu, Sep 15, 2011 at 12:36 AM, Christoph Hellwig wrote: > ... > >> > +/* > >> > + * We support iscsi url's on the form > >> > + * iscsi://[%@][:]// > >> > + */ > > > > Is having username + password on the command line really a that good idea? > > Also what about the more complicated iSCSI authentification schemes? > > In general it is a very bad idea. For local use on a private box it is > convenient to be able to use "%@" syntax. > For use on a shared box, libiscsi supports an alternative method too > by setting the username and/or password via environment variables : > LIBISCSI_CHAP_USERNAME=... LIBISCSI_CHAP_PASSWORD=... Environement variables are only a tiny bit better, since this still allows the password to leak to any processes which can read /proc/$PID/environ. It is also undesirable wrt many distro trouble shooting tools (eg Fedora/ RHEL's sosreport) which capture the contents of /proc/$PID/environ as part of their data collection process. This means your passwords will end up in attachments to bugzilla / issue tracker tickets. For block devs with encrypted QCow2 disks (and VNC/SPICE) QEMU requires the password to be set via the monitor. Since this iscsi: protocol is part of the block layer, IMHO, the password should be settable the same way via the monitor 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 :|
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 09:04 AM, Paolo Bonzini wrote: On 09/15/2011 01:08 AM, ronnie sahlberg wrote: I think it is reasonable to just not support iscsi at all for blocksize that is not multiple of 512 bytes since a read-modify-write cycle across a network would probably be prohibitively expensive. Agreed. .bdrv_flush() I can easily add a synchronous implementation of this unless your patch is expected to be merged in the near future. We need the same patch for NBD, so I wouldn't bother with the synchronous flush. Paolo It seems to me that using a qemu external initiator/target pairs like Orit's original design in http://wiki.qemu.org/Features/LiveBlockMigration#ISCSI_for_non_shared_storage would be a simpler (in terms of qemu code..) and flexible solution. I agree that embedding the iscsi initiation in qemu can simplify the end user life but since this scenario is expected to be used by higher level software it's not relevant here. The risk is to have to maintain more code that won't be as general as the tgtd/lio solutions out there.
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Am 15.09.2011 00:51, schrieb ronnie sahlberg: > On Thu, Sep 15, 2011 at 12:36 AM, Christoph Hellwig wrote: > ... +/* + * We support iscsi url's on the form + * iscsi://[%@][:]// + */ >> >> Is having username + password on the command line really a that good idea? >> Also what about the more complicated iSCSI authentification schemes? > > In general it is a very bad idea. For local use on a private box it is > convenient to be able to use "%@" syntax. > For use on a shared box, libiscsi supports an alternative method too > by setting the username and/or password via environment variables : > LIBISCSI_CHAP_USERNAME=... LIBISCSI_CHAP_PASSWORD=... I wonder if we could make it look like an encrypted image. qemu already has functionality to deal with passwords for block devices, so it seems to make sense to reuse that for iscsi passwords. Kevin
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
Am 15.09.2011 08:04, schrieb Paolo Bonzini: > On 09/15/2011 01:08 AM, ronnie sahlberg wrote: >> I think it is reasonable to just not support iscsi at all for >> blocksize that is not multiple of 512 bytes >> since a read-modify-write cycle across a network would probably be >> prohibitively expensive. > > Agreed. > >> .bdrv_flush() I can easily add a synchronous implementation of this >> unless your patch is expected to be merged >> in the near future. > > We need the same patch for NBD, so I wouldn't bother with the > synchronous flush. Doesn't Stefan's patch conflict with your bdrv_co_flush patch? Long term I think we should get rid of bdrv_flush and bdrv_aio_flush and make all drivers use bdrv_co_flush. Having three different interfaces for flush and emulation functions that do mappings for all missing implementations is just insane. Kevin
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 10:48 AM, Dor Laor wrote: We need the same patch for NBD, so I wouldn't bother with the synchronous flush. It seems to me that using a qemu external initiator/target pairs like Orit's original design in http://wiki.qemu.org/Features/LiveBlockMigration#ISCSI_for_non_shared_storage would be a simpler (in terms of qemu code..) and flexible solution. Yes, it would be simpler for QEMU because everything is done outside. However, iSCSI is a complex protocol and a complex suite of tools. With a simpler set of tools such as qemu-nbd/nbd, you can for example tunnel data over the libvirt connection. Possibly with encryption. Also, with iSCSI you're tied to raw, while qemu-nbd lets you use qcow2. iSCSI could be a better choice if everything in the QEMU block layer was done in terms of SCSI. However, we're a far cry from that. I agree that embedding the iscsi initiation in qemu can simplify the end user life but since this scenario is expected to be used by higher level software it's not relevant here. The risk is to have to maintain more code that won't be as general as the tgtd/lio solutions out there. I'm not sure I understand. You say "embedding the iSCSI initiator in qemu can simplify the end user life" but "the risk is to have to maintain [another iSCSI target]". I don't think anybody proposed adding an iSCSI target to QEMU (in fact tcm_vhost would even let you use lio instead of QEMU's SCSI target code!). Only an iSCSI initiator which is not much code because it's just a wrapper for libiscsi. In general, I'm not sure that libiscsi (instead of the in-kernel iSCSI initiator) is by definition a bad choice in high-end set-ups. If you want to do SCSI passthrough, it's likely better to use libiscsi rather than using the in-kernel initiator together with scsi-generic (scsi-generic sucks; bsg is better but also a useless level of indirection IMO). Perhaps Ronnie has rough performance numbers comparing in-kernel iSCSI with libiscsi? Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 11:10 AM, Kevin Wolf wrote: > We need the same patch for NBD, so I wouldn't bother with the > synchronous flush. Doesn't Stefan's patch conflict with your bdrv_co_flush patch? Yes. I'll include it myself in my NBD v2. Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Sep 15, 2011 at 11:48:35AM +0300, Dor Laor wrote: > On 09/15/2011 09:04 AM, Paolo Bonzini wrote: > >On 09/15/2011 01:08 AM, ronnie sahlberg wrote: > >>I think it is reasonable to just not support iscsi at all for > >>blocksize that is not multiple of 512 bytes > >>since a read-modify-write cycle across a network would probably be > >>prohibitively expensive. > > > >Agreed. > > > >>.bdrv_flush() I can easily add a synchronous implementation of this > >>unless your patch is expected to be merged > >>in the near future. > > > >We need the same patch for NBD, so I wouldn't bother with the > >synchronous flush. > > > >Paolo > > > > > > It seems to me that using a qemu external initiator/target pairs > like Orit's original design in > http://wiki.qemu.org/Features/LiveBlockMigration#ISCSI_for_non_shared_storage > would be a simpler (in terms of qemu code..) and flexible solution. > > I agree that embedding the iscsi initiation in qemu can simplify the > end user life but since this scenario is expected to be used by > higher level software it's not relevant here. The risk is to have to > maintain more code that won't be as general as the tgtd/lio > solutions out there. This should not be considered an either/or decision really. There are compelling benefits for having a iSCSI initiator inside QEMU which Ronnie outlined in the first mail in this thread. The enablement for non-root users is, on its own, enough justification IMHO. * Privacy: The iSCSI LUNs are private to the guest and are not visible either to the host, nor to any processes running on the host. * Ease of managment : If you have very many guests and very many, thousands of, iSCSI LUNs. It is inconvenient to have to expose all LUNs to the underlying host. * No root requirement. Since the iSCSI LUNs are not mounted as devices on the host, ordinary users can set up and use iSCSI resources without the need for root privilege on the host to map the devices to local scsi devices. There are several other benefits too * Since the network I/O for the iSCSI LUN is now done by the QEMU process instead of the kernel, each VM's iSCSI I/O traffic can be insolated & controlled via the cgroups network contorller / tc network classifier. * Portable across OS. Each OS has different tools for setting up iSCSI usage. A native driver lets QEMU users setup iSCSI in the same way regardless of the level of OS support for iSCSI. Finally experiance integrating with the Linux iscsiadm command line tools in libvirt, has shown that they can be quite a PITA to integrate with if your mgmt app wants reliable/predictable results from their usage regardless of what an admin may have previously setup on the host. So, IMHO, from a QEMU POV it makes perfect sense to have a builtin iSCSI initiator, as an alternative to using the OS' iSCSI tools externally. Vendors of applications managing KVM/QEMU are then free to decide which of the approaches they wish to support in their own products. 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 :|
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Sep 15, 2011 at 7:11 PM, Paolo Bonzini wrote: ... > Perhaps Ronnie has rough performance numbers comparing in-kernel iSCSI with > libiscsi? I did some tests some time ago just doing things like 'dd if=/dev/sda of=/dev/null' on the root disk from within a RHEL guest itself and performace was comparable. In some tests libiscsi was a few percent faster, in other tests it was a few percent slower. But overall, very unscientifically performed test, performance was not much different. Now, I am not an expert on tuning cache or tuning the kernel scsi and iscsi layer, so I would not really want to make too many statements in the area other than in some tests I performed, performance was good enough for my use. I would be very happy if someone competent in tuning qemu/scsi/open-iscsi (==that means, not me) would do a real comparasion. regards ronnie sahlberg
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 12:11 PM, Paolo Bonzini wrote: On 09/15/2011 10:48 AM, Dor Laor wrote: We need the same patch for NBD, so I wouldn't bother with the synchronous flush. It seems to me that using a qemu external initiator/target pairs like Orit's original design in http://wiki.qemu.org/Features/LiveBlockMigration#ISCSI_for_non_shared_storage would be a simpler (in terms of qemu code..) and flexible solution. Yes, it would be simpler for QEMU because everything is done outside. However, iSCSI is a complex protocol and a complex suite of tools. With a simpler set of tools such as qemu-nbd/nbd, you can for example tunnel data over the libvirt connection. Possibly with encryption. Also, with iSCSI you're tied to raw, while qemu-nbd lets you use qcow2. My main motivation for external iScsi is to provide qemu operations w/ non shared storage. Things like streaming an image w/o shared storage or block live migration can be done where the src host exposes iscsi target and the destination is the initiator. In case of qcow2, every snapshot in the chain should be exposed as a separate LUN. The idea is to ignore the data in the guest image and treat it as opaque. iSCSI could be a better choice if everything in the QEMU block layer was done in terms of SCSI. However, we're a far cry from that. I agree that embedding the iscsi initiation in qemu can simplify the end user life but since this scenario is expected to be used by higher level software it's not relevant here. The risk is to have to maintain more code that won't be as general as the tgtd/lio solutions out there. I'm not sure I understand. You say "embedding the iSCSI initiator in qemu can simplify the end user life" but "the risk is to have to maintain [another iSCSI target]". I don't think anybody proposed adding an iSCSI target to QEMU (in fact tcm_vhost would even let you use lio instead of QEMU's SCSI target code!). Only an iSCSI initiator which is not much code because it's just a wrapper for libiscsi. In general, I'm not sure that libiscsi (instead of the in-kernel iSCSI initiator) is by definition a bad choice in high-end set-ups. If you want to do SCSI passthrough, it's likely better to use libiscsi rather than using the in-kernel initiator together with scsi-generic (scsi-generic sucks; bsg is better but also a useless level of indirection IMO). Perhaps Ronnie has rough performance numbers comparing in-kernel iSCSI with libiscsi? Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, Sep 15, 2011 at 02:42:42PM +0300, Dor Laor wrote: > My main motivation for external iScsi is to provide qemu operations w/ non > shared storage. iSCSI with multiple initiator is shared storage.
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 01:42 PM, Dor Laor wrote: However, iSCSI is a complex protocol and a complex suite of tools. With a simpler set of tools such as qemu-nbd/nbd, you can for example tunnel data over the libvirt connection. Possibly with encryption. Also, with iSCSI you're tied to raw, while qemu-nbd lets you use qcow2. My main motivation for external iScsi is to provide qemu operations w/ non shared storage. Things like streaming an image w/o shared storage or block live migration can be done where the src host exposes iscsi target and the destination is the initiator. In case of qcow2, every snapshot in the chain should be exposed as a separate LUN. The idea is to ignore the data in the guest image and treat it as opaque. Then you need an iSCSI *target* that understands qcow2, like qemu-nbd but for iSCSI... that's exactly the thing you were worried about implementing. (Of course you could use qemu-nbd and expose /dev/nbd* via iSCSI, but it starts looking like an exercise in adding layers of indirections! :)). Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 02:46 PM, Christoph Hellwig wrote: On Thu, Sep 15, 2011 at 02:42:42PM +0300, Dor Laor wrote: My main motivation for external iScsi is to provide qemu operations w/ non shared storage. iSCSI with multiple initiator is shared storage. Exactly :) that's the magic.
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 02:01 PM, Dor Laor wrote: My main motivation for external iScsi is to provide qemu operations w/ non shared storage. iSCSI with multiple initiator is shared storage. Exactly :) that's the magic. I think we're on the same page, it's just a difference in terms. iSCSI or NBD would provide the sharing of the storage instead of NFS, basically, so the underlying storage is not shared -- right? Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, 2011-09-15 at 13:58 +0200, Paolo Bonzini wrote: > On 09/15/2011 01:42 PM, Dor Laor wrote: > >> > >> However, iSCSI is a complex protocol and a complex suite of tools. With > >> a simpler set of tools such as qemu-nbd/nbd, you can for example tunnel > >> data over the libvirt connection. Possibly with encryption. Also, with > >> iSCSI you're tied to raw, while qemu-nbd lets you use qcow2. > > > > My main motivation for external iScsi is to provide qemu operations w/ > > non shared storage. Things like streaming an image w/o shared storage or > > block live migration can be done where the src host exposes iscsi target > > and the destination is the initiator. In case of qcow2, every snapshot > > in the chain should be exposed as a separate LUN. The idea is to ignore > > the data in the guest image and treat it as opaque. > > Then you need an iSCSI *target* that understands qcow2, like qemu-nbd > but for iSCSI... that's exactly the thing you were worried about > implementing. Not really , this is just part of the target configuration. For each file in the chain we create a lun in the target that is backed by the file and we number the luns in chronological order (base is lun 1) . you can look at a tgtd configuration here http://wiki.qemu.org/Features/LiveBlockMigration#Example_single_base_master_image. Orit > > (Of course you could use qemu-nbd and expose /dev/nbd* via iSCSI, but it > starts looking like an exercise in adding layers of indirections! :)). > > Paolo >
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On 09/15/2011 02:34 PM, Orit Wasserman wrote: > Then you need an iSCSI*target* that understands qcow2, like qemu-nbd > but for iSCSI... that's exactly the thing you were worried about > implementing. Not really , this is just part of the target configuration. For each file in the chain we create a lun in the target that is backed by the file and we number the luns in chronological order (base is lun 1) . you can look at a tgtd configuration here http://wiki.qemu.org/Features/LiveBlockMigration#Example_single_base_master_image. That's surely a weird way to use iSCSI. :) It's clever, but I would call that shared storage, since you need to share the details of the snapshot chain between the source and destination, down to the file names. We need a more precise nomenclature. Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Thu, 2011-09-15 at 14:58 +0200, Paolo Bonzini wrote: > On 09/15/2011 02:34 PM, Orit Wasserman wrote: > >> > Then you need an iSCSI*target* that understands qcow2, like qemu-nbd > >> > but for iSCSI... that's exactly the thing you were worried about > >> > implementing. > > > > Not really , this is just part of the target configuration. > > For each file in the chain we create a lun in the target that is backed > > by the file and we number the luns in chronological order (base is lun > > 1) . you can look at a tgtd configuration here > > http://wiki.qemu.org/Features/LiveBlockMigration#Example_single_base_master_image. > > That's surely a weird way to use iSCSI. :) :) > It's clever, but I would call that shared storage, since you need to > share the details of the snapshot chain between the source and > destination, down to the file names. We need a more precise nomenclature. We store the parent base image inside the image (relative path). We can reverse engineer it in the destination , start at the highest lun ,read it's parent name , create a symbolic link to the parent (previous lun) and so on ... Orit > Paolo
Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI
On Wed, Sep 14, 2011 at 04:50:25PM +0100, Stefan Hajnoczi wrote: > I think in this case it will not make the code nicer. Since the > external iSCSI library is based on callbacks it would be necessary to > write the coroutines<->callbacks adapter functions. So for example, > the READ10 command would need a function that can be called in > coroutine context and yields while libiscsi does the I/O. When the > callback is invoked it will re-enter the coroutine. > > The area where coroutines are useful in the block layer is for image > formats. We already have common coroutines<->callback adapter > functions in block.c so it's possible to write sequential code for > image formats. They only need access to block layer functions which > have already been adapted. But as soon as you interact with a > callback-based API from the coroutine, then you need to write an > adapter yourself. So you plan on keeping the aio interface around forever? Qemu with two different I/O pathes was already more than painful enough, I don't think keeping three, and two of them beeing fairly complex is a good idea.
