[Stgt-devel] [PATCH] Move device type to LUN

[FUJITA Tomonori]

From: Hannes Reinecke <hare at suse.de>
Subject: Re: [Stgt-devel] [PATCH] Move device type to LUN
Date: Mon, 18 Jun 2007 09:33:38 +0200

> FUJITA Tomonori wrote:
> > From: Hannes Reinecke <hare at suse.de>
> > Subject: Re: [Stgt-devel] [PATCH] Move device type to LUN
> > Date: Mon, 18 Jun 2007 08:29:13 +0200
> > 
> >> FUJITA Tomonori wrote:
> >>> From: FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp>
> >>> Subject: Re: [Stgt-devel] [PATCH] Move device type to LUN
> >>> Date: Fri, 15 Jun 2007 13:36:12 +0900
> >>>
> >>>> From: Hannes Reinecke <hare at suse.de>
> >>>> Subject: Re: [Stgt-devel] [PATCH] Move device type to LUN
> >>>> Date: Tue, 12 Jun 2007 14:44:03 +0200
> >>>>
> >>>>> FUJITA Tomonori wrote:
> >>>>>> From: Hannes Reinecke <hare at suse.de>
> >>>>>> Subject: Re: [Stgt-devel] [PATCH] Move device type to LUN
> >>>>>> Date: Mon, 11 Jun 2007 17:15:19 +0200
> >>>>>>
> >>>>>>> Pete Wyckoff wrote:
> >>>>>>>> hare at suse.de wrote on Tue, 05 Jun 2007 15:38 +0200:
> >>>>>>>>> This patch moves the device type down to the LUN structure.
> >>>>>>>>> And in doing so we now also have the proper peripheral device
> >>>>>>>>> type and peripheral device qualifier attributes for the INQUIRY
> >>>>>>>>> data.
> >>>>>>>> Makes sense to me too.
> >>>>>>>>
> >>>>>>>>> One thing puzzles me, though: do we support commands with no
> >>>>>>>>> LUN attached to it? IE is it valid to have 'cmd->dev == NULL'?
> >>>>>>>>> If so: where is the point here? If that's our handling of a
> >>>>>>>>> non-existing LUN 0 we should rather add a proper LUN 0 and
> >>>>>>>>> treat cmd->dev == NULL as an error case ...
> >>>>>>>> In the SCSI model, every device must have at least one LUN for
> >>>>>>>> handling REPORT LUNS and a couple other commands.  It is addressed
> >>>>>>>> as LUN 0 or using the "well-known" LUN for the command.  In the stgt
> >>>>>>>> abstraction, though, there is no magic LUN like this.  Instead
> >>>>>>>> things like spc_inqury use cmd->dev == NULL to handle this case.
> >>>>>>>>
> >>>>>>> Ah. Hence.
> >>>>>>>
> >>>>>>>> Perhaps it is reasonable to create a special device for these
> >>>>>>>> commands.  Up in target.c, you could assign cmd->dev to target->dev,
> >>>>>>>> where that is the special device, paralleling the way that
> >>>>>>>> target->cmd_queue is used.  As a side effect, lots of "if (cmd->dev)
> >>>>>>>> ... else ILLEGAL_REQUEST" clauses can be removed from device code
> >>>>>>>> too.
> >>>>>>>>
> >>>>>>> Well, I actually thought of creating a proper LUN 0 with type 0xc.
> >>>>>> We still need to handle cmd->dev == NULL case though probably we can
> >>>>>> remove cmd->dev == NULL case in device type code.
> >>>>>>
> >>>>>> I don't have storage systems that work in your way. Is it common?
> >>>>>>
> >>>>>>
> >>>>> Yes, quite common. HP and EMC (to name but a few) do it this way.
> >>>>> Most storage arrays actually refuse to attach any devices to LUN0, as
> >>>>> this is a pure management LUN.
> >>>>>
> >>>>> I will draft up a patch which creates a proper controller LUN0.
> >>>> Ok, here's a patch. This depends on the patch that I've just
> >>>> submitted.
> >>>>
> >>>> This adds scc support and fixes ibmvio. iSCSI works. ibmvio works for
> >>>> me, but it detects scsi devices in a unique way, so I'm not sure it
> >>>> works for everyone.
> >>>>
> >>>> As described, a scsi controler device is automatically created as lun0
> >>>> with a new target.
> >>>>
> >>>> lily:/home/fujita# ./tgt/usr/tgtadm --lld iscsi --op new --mode target --tid 1 -T iqn.2001-04.com.example:storage.disk2.tulip.sys1.xyz
> >>>> lily:/home/fujita# ./tgt/usr/tgtadm --op show --mode target
> >>>> Target 1: iqn.2001-04.com.example:storage.disk2.tulip.sys1.xyz
> >>>>     System information:
> >>>>         Driver: iscsi
> >>>>         Status: running
> >>>>     I_T nexus information:
> >>>>     LUN information:
> >>>>         LUN: 0
> >>>>             Type: raid
> >>>>             SCSI ID: deadbeaf1:0
> >>>>             SCSI SN: beaf10
> >>>>             Size: 0
> >>>>             Backing store: No backing store
> >>>>     Account information:
> >>>>     ACL information:
> >>>>
> >>>>
> >>>> The patch is still very hacky but shows how it works. Let me know this
> >>>> works or not.
> >>> I did some cleanups.
> >>>
> >>> http://git.kernel.org/?p=linux/kernel/git/tomo/tgt.git;a=shortlog;h=scc
> >>>
> >> Regarding the disk_type_names cleanups:
> >> - What is the rationale for this? Just truncating the full name to
> >> something unintelligible doesn't seem like the smartest move to me,
> >> especially as we're not facing any string length restrictions here ...
> > 
> > They are the names that lsscsi uses. But I don't have strong
> > preference.
> > 
> Well, lsscsi has string length limitations as the output has to be
> neatly arranged in colums; we're not facing these limitations here.
> So I'd prefer to have legible names.

ok, how about the following names:

static struct {
	int value;
	char *name;
} disk_type_names[] = {
	{TYPE_DISK, "disk"},
	{TYPE_TAPE, "tape"},
	{TYPE_PRINTER, "printer"},
	{TYPE_PROCESSOR, "processor"},
	{TYPE_WORM, "worm"},
	{TYPE_ROM, "cd/dvd"},
	{TYPE_SCANNER, "scanner"},
	{TYPE_MOD, "optical"},
	{TYPE_MEDIUM_CHANGER, "changer"},
	{TYPE_COMM, "communication"},
	{TYPE_RAID, "controller"},
	{TYPE_ENCLOSURE, "enclosure"},
	{TYPE_RBC, "rbc"},
	{TYPE_OSD, "osd"},
	{TYPE_NO_LUN, "No LUN"}
};


> >> Regarding device type registration cleanups:
> >> - We're losing the ability to select sg_bst for passthrough during
> >>   registration. Is that intentional?
> > 
> > Yeah. I plan to fix it though I'm not hurry since it needs bsg, which
> > is supposed to be merged into 2.6.23.
> Ah. Cool. I'll wait for it, then.
> 
> The other patches look ok to me. I see if I manage to give them a spin.

Let me know the results. Thanks.