[Stgt-devel] iscsi target works, minor fixes

[FUJITA Tomonori]

From: Pete Wyckoff <pw at osc.edu>
Subject: [Stgt-devel] iscsi target works, minor fixes
Date: Thu, 2 Nov 2006 19:45:48 -0500

> I've been playing with the user-space iscsi target, looking at the
> latest stgt SVN and linux-2.6-target git today.  One little fix I
> need in the userspace code is this:
> 
> Index: trunk/usr/bd_aio.c
> ===================================================================
> --- trunk/usr/bd_aio.c	(revision 609)
> +++ trunk/usr/bd_aio.c	(working copy)
> @@ -74,8 +74,8 @@ static void aio_event_handler(int fd, in
>  
>  	for (i = 0; i < nr; i++) {
>  		iocb = bai->events[i].obj;
> -		dprintf("%p\n", iocb->data);
> -		target_cmd_io_done(iocb->data, 0);
> +		dprintf("%p\n", bai->events[i].data);
> +		target_cmd_io_done(bai->events[i].data, 0);
>  	}
>  }
>  
> While I don't understand exactly what's going on, this data pointer
> from struct io_event is the correct struct cmd *, and without the
> patch, tgtd segfaults trying to access fields in the (bogus) cmd
> from iocb->data.

It works with 2.6.18 for me. The kernel aio code sets io_event->obj to
iocb. We set iocb->data to cmd.

I've not tested aiopoll patch with the current git tree. I'll do after
sending this mail. But we must replace the aiopoll patch with
something as soon as possible.

By the way, does the kernel crash when you kill the tgtd daemon (it's
due to the aiopoll problem)?

If so, try to remove the logical units before killing the daemon.

First, you can see the current configuration:

lily:/home/fujita/tgt# ./usr/tgtadm --lld iscsi --op show
tid 1: lld iscsi: iqn.2001-04.com.example:storage.disk2.amiens.sys1.xyz
        lun 0: path /dev/hdc1

Second, remove the lu:

lily:/home/fujita/tgt# ./usr/tgtadm --lld iscsi --op delete --tid 1 --lun 0

Then, see the configuration again:

lily:/home/fujita/tgt# ./usr/tgtadm --lld iscsi --op show
tid 1: lld iscsi: iqn.2001-04.com.example:storage.disk2.amiens.sys1.xyz

The lu has gone. Hopefully, you can kill the daemon safely.


> It could be related to the aiopoll-2.6.18-rc4.patch; perhaps that
> was the wrong one to apply?  There was one minor reject that was
> easy to fix up.  My kernel is v2.6.19-rc4 + the 9 patches in tomo's
> linux-2.6-target.git + aiopoll + other minor patches.

I see. I'll update the patch later.


> You may want this, but it doesn't change any behavior:
> 
> diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
> index e01f458..6e92b97 100644
> --- a/drivers/scsi/scsi_tgt_lib.c
> +++ b/drivers/scsi/scsi_tgt_lib.c
> @@ -108,7 +108,6 @@ struct scsi_cmnd *scsi_host_get_command(
>  
>  	rq->special = cmd;
>  	rq->cmd_type = REQ_TYPE_SPECIAL;
> -	rq->cmd_flags |= REQ_TYPE_BLOCK_PC;
>  	rq->end_io_data = tcmd;
>  
>  	bio_list_init(&tcmd->xfer_list);
> 
> 
> What goes in cmd_flags are bit fields like REQ_SOFTBARRIER.  You're
> not supposed to try to put another REQ_TYPE in there.  You do want
> SPECIAL, not BLOCK_PC, since you are allocating your own struct
> scsi_cmnd and putting it into rq->special.

Thanks. You are right. Seems that REQ_TYPE_BLOCK_PC blongs to
cmd_type. I just merged the fix in -mm kernel without seeing the
details.

Anyway, we can remove the cmd_type and cmd_flags parts. They are
useless now. The old version needs them but now we don't, I think.


> Basic mkfs and dd seem to work just fine.  Thanks for the code.

Nice. Thanks for testings. I think that the basic operations are ok,
but there are still many things to polish.