[Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[ronnie sahlberg]

List,

Please find attached a gzipped patch against master that adds support of iSCSI.
It is sent in gz format because of its uncompressed size, >100kb.


This patch adds support for attaching directorly to iSCSI resources,
both DISK/SBC and CDROM/MMC, without the need to make the devices
visible to the host.

There are two parts to this support :

./block/iscsi/* : This is a fully async iscsi initiator client
library. This directory does not contain any QEMU specific code with
the hope it can later be re-used by other projects that also need
iscsi-initiator functionality.

./block/iscsi.c : The QEMU block driver that intergrates the
iscsi-initiator library with QEMU.


The iscsi library is incomplete, but sufficient to interoperate with
TGTD. With trivial enhancements(target-NOP) it should work with most
other targets too.

The goal is to expand the library to add more features over time, so
that over time it will become a full-blown iscsi initiator
implementation as a library.

Some features that should be added are
* support for target initiated NOP.  TGTD does not use these but some
other targets do.
* support for uni- and bi-directional CHAP authentication.
* additional iscsi functionality

The syntax to specify a iscsi resource is
   iscsi://<host>[:port]/<target-iqn-name>/<lun>

Example :
   -drive file=iscsi://127.0.0.1:3260/iqn.ronnie.test/1

  -cdrom iscsi://127.0.0.1:3260/iqn.ronnie.test/2


Please review and/or commit.


regards
ronnie sahlberg

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[FUJITA Tomonori]

On Mon, 22 Nov 2010 09:17:50 +1100
ronnie sahlberg <ronniesahlberg@gmail.com> wrote:

> This patch adds support for attaching directorly to iSCSI resources,
> both DISK/SBC and CDROM/MMC, without the need to make the devices
> visible to the host.

This adds the iscsi initiator feature to qemu (as you did to dbench),
right?


> The syntax to specify a iscsi resource is
>    iscsi://<host>[:port]/<target-iqn-name>/<lun>
> 
> Example :
>    -drive file=iscsi://127.0.0.1:3260/iqn.ronnie.test/1
> 
>   -cdrom iscsi://127.0.0.1:3260/iqn.ronnie.test/2

Specifying lun looks odd (from the perspective of SCSI initiator)?

btw, how do we specify initiator's configuration (iscsi params, chap,
initiator iqn, etc)?

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[ronnie sahlberg]

On Mon, Nov 22, 2010 at 10:13 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Mon, 22 Nov 2010 09:17:50 +1100
> ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
>
>> This patch adds support for attaching directorly to iSCSI resources,
>> both DISK/SBC and CDROM/MMC, without the need to make the devices
>> visible to the host.
>
> This adds the iscsi initiator feature to qemu (as you did to dbench),
> right?

Yes.

>
>
>> The syntax to specify a iscsi resource is
>>    iscsi://<host>[:port]/<target-iqn-name>/<lun>
>>
>> Example :
>>    -drive file=iscsi://127.0.0.1:3260/iqn.ronnie.test/1
>>
>>   -cdrom iscsi://127.0.0.1:3260/iqn.ronnie.test/2
>
> Specifying lun looks odd (from the perspective of SCSI initiator)?
>

Ok.
I can easily to change this.
How would you prefer the iscsi:// url look like ?


> btw, how do we specify initiator's configuration (iscsi params, chap,
> initiator iqn, etc)?
>

You cant yet. :-(

Right now they are hardcoded to reasonable defaults I think will work
for most targets.
I want to add more of these features and make them settable from the
command line,
but that will take a lot of time.

Hopefully having a basic/non-flexible implementation as a first step
may even encourage others to
contribute to improve it until it is full featured.


regards
ronnie sahlberg

[Qemu-devel] Re: [PATCH 1/1] iscsi: add iSCSI block device support

[Kevin Wolf]

Am 21.11.2010 23:17, schrieb ronnie sahlberg:
> List,
> 
> Please find attached a gzipped patch against master that adds support of iSCSI.
> It is sent in gz format because of its uncompressed size, >100kb.

I think there's something missing in your mail. :-)

Anyway, I got the code from the KVM list, and I have some general comments:

* Please split this into some smaller patches and send them as a patch
  series. Inline the patches instead of a gzipped attachment, because
  that makes it a lot easier to comment the code when replying to your
  mail.
* Have a look at CODING_STYLE in the repository, especially at the
  sections about 80 chars per line and whitespace.
* Don't use printf() but error_report() for error messages
* Try to use qemu functions instead of POSIX ones so that the code
  will also work on Windows and other platforms. If you can't make it
  run on Windows, still use qemu functions where possible and change
  the Makefile to compile it only for POSIX hosts.
* Once you use qemu_malloc, you can drop your NULL checks. qemu_malloc
  never returns an error.
* Use QLISTs (qemu-queue.h) instead of introducing a new SLIST

Kevin

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[Christoph Hellwig]

I don't really see the point of this.  It's implemented as a protocol,
and thus the bottom of the qemu protocol stack.  So we might get SCSI
commands in from the guest, decode them, just to re-encode them again
and send them off to the network all inside qemu.

Do you have any performance numbers showing why the addition of the
code is nessecary over just using the kernel iscsi initiator?  Which
btw, is a lot more flexible as it also supports SG_IO passthrough
and thus accessing other devices not supporting the SBC command set.

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[Stefan Hajnoczi]

On Wed, Nov 24, 2010 at 5:04 PM, Christoph Hellwig <hch@lst.de> wrote:
> Do you have any performance numbers showing why the addition of the
> code is nessecary over just using the kernel iscsi initiator?  Which
> btw, is a lot more flexible as it also supports SG_IO passthrough
> and thus accessing other devices not supporting the SBC command set.

With userspace iSCSI initiator support setup becomes easy, portable,
and doesn't require admin privileges:
qemu -drive file=iscsi:...

That's pretty compelling.  I don't know if people who already use
iSCSI today will want to switch but it provides an easy entry into
iSCSI.

Stefan

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[Nicholas A. Bellinger]

On Wed, 2010-11-24 at 19:14 +0000, Stefan Hajnoczi wrote:
> On Wed, Nov 24, 2010 at 5:04 PM, Christoph Hellwig <hch@lst.de> wrote:
> > Do you have any performance numbers showing why the addition of the
> > code is nessecary over just using the kernel iscsi initiator?  Which
> > btw, is a lot more flexible as it also supports SG_IO passthrough
> > and thus accessing other devices not supporting the SBC command set.
> 
> With userspace iSCSI initiator support setup becomes easy, portable,
> and doesn't require admin privileges:
> qemu -drive file=iscsi:...
> 
> That's pretty compelling.  I don't know if people who already use
> iSCSI today will want to switch but it provides an easy entry into
> iSCSI.
> 

So I have had a bit of experience with the VirtualBox iSCSI initiator,
which is similar in the sense that it also runs in userspace, and is
independent of the host OS, etc..  The primary issue tends to be the
threading model, ie: that the stack is limited to a single thread, which
makes it very limiting in terms of scaling (in my experience) at
1 Gb/sec speeds.

But existing code aside, I think having a small userspace iSCSI
initiator built into QEMU that 'just works' across all host environments
would be a pretty useful thing, even if the scalibility / scope is
limited compared to existing kernel host level iSCSI initiator stacks,
et al.  I have not yet had a chance to look at Ronnie's code, but would
be interested to understand how the threading model currently functions.

Ronnie, I would recommending following Kevin's earlier advice and split
your work into a reviewable series of commits (preferrably generated by
git-format-patch) and repost the series for feedback to qemu-devel.  You
can read that as coded language for 'you will want to learn git to
submit your patch', but it really does work.  ;)

--nab

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[ronnie sahlberg]

Nicholas,
below.

On Thu, Nov 25, 2010 at 3:32 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:

> But existing code aside, I think having a small userspace iSCSI
> initiator built into QEMU that 'just works' across all host environments
> would be a pretty useful thing, even if the scalibility / scope is
> limited compared to existing kernel host level iSCSI initiator stacks,
> et al.  I have not yet had a chance to look at Ronnie's code, but would
> be interested to understand how the threading model currently functions.
>
> Ronnie, I would recommending following Kevin's earlier advice and split
> your work into a reviewable series of commits (preferrably generated by
> git-format-patch) and repost the series for feedback to qemu-devel.  You
> can read that as coded language for 'you will want to learn git to
> submit your patch', but it really does work.  ;)

Thanks,

I will work on the suggestions over the weekend, so Ill resend either
this weekend or next weekend.
So don't spend/waste time reviewing now.

As for the threading model.
Currently it is not threads safe, so all calls into the library would
have to be protected through a mutex if used from concurrent threads.
I couldn't see any such mutexes when looking at sheepdog as an
example, so do the block drivers need to be threads-safe in qemu?

One goal of the library is to be 100% async and to never make any
blocking syscalls.
So the library will never block and should be able to reach good
performance, even with one single thread.

(I assume your VirtualBOX performance problem was not the threading in
itself but that the threads use blocking calls?)


regards
ronnie sahlberg

Re: [Qemu-devel] [PATCH 1/1] iscsi: add iSCSI block device support

[Kevin Wolf]

Am 25.11.2010 06:22, schrieb ronnie sahlberg:
> Nicholas,
> below.
> 
> On Thu, Nov 25, 2010 at 3:32 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> 
>> But existing code aside, I think having a small userspace iSCSI
>> initiator built into QEMU that 'just works' across all host environments
>> would be a pretty useful thing, even if the scalibility / scope is
>> limited compared to existing kernel host level iSCSI initiator stacks,
>> et al.  I have not yet had a chance to look at Ronnie's code, but would
>> be interested to understand how the threading model currently functions.
>>
>> Ronnie, I would recommending following Kevin's earlier advice and split
>> your work into a reviewable series of commits (preferrably generated by
>> git-format-patch) and repost the series for feedback to qemu-devel.  You
>> can read that as coded language for 'you will want to learn git to
>> submit your patch', but it really does work.  ;)
> 
> Thanks,
> 
> I will work on the suggestions over the weekend, so Ill resend either
> this weekend or next weekend.
> So don't spend/waste time reviewing now.
> 
> As for the threading model.
> Currently it is not threads safe, so all calls into the library would
> have to be protected through a mutex if used from concurrent threads.
> I couldn't see any such mutexes when looking at sheepdog as an
> example, so do the block drivers need to be threads-safe in qemu?

No, block drivers are not threaded currently. You don't have to take
care of that yourself, everything is already protected by a mutex.

> One goal of the library is to be 100% async and to never make any
> blocking syscalls.
> So the library will never block and should be able to reach good
> performance, even with one single thread.

That would match what other block drivers do.

Kevin