Request for review of Linux iSCSI driver version 4.0.0.2

Request for review of Linux iSCSI driver version 4.0.0.2

[Naveen Burmi]

Folks,

Thanks to all of you for the review of linux-iscsi driver 4.0.0.1. As on
today, we have taken care of most of the review comments. There are few
others that we are in active discussion in this (scsi) mailing list and
will incorporate them in due course of time.

I would like to call for review of 4.0.0.2 Linux iSCSI driver that has
most of the review comments taken care of. It can be downloaded from

http://sf.net/projects/linux-iscsi/

Could you please review the same for inclusion in 2.6 kernel?

P.S: 4.0.0.2 has been tested successfully with linux-2.6.0-test9.

Here is the list of changes (Based on comments from 1st review):


THE FOLLOWING COMMENTS ARE TAKEN CARE OF:
----------------------------------------

1. Please kill your thousands of atio/aton reimplementations and look for
    proper linux functions like simple_strtoul.

2.  iscis-kernel.h:
        - only backward-compat cruft, should go away.

3.  - doesn't need at least smp_lock.h - please audit header
        usage.
        - kill stupid MODULE/MODULE_LICENSE ifdefs.
        - kill = 0 / = NULL initializers for static variables, not needed.
        - kill LINK_DIR/session->target_link_dir - kernel must
          not know about device pathes.
        - kill max_*_devices - not needed in kernelspace (and wrong
          in 2.6)
        - fix up strange indentation artefacts from indent, like:

4.  static struct iscsi_trace_entry
    trace_table[ISCSI_TRACE_COUNT];

should be:

static struct iscsi_trace_entry trace_table[ISCSI_TRACE_COUNT];

        - first argument to memset is void * - don't cast pointers
        to char *
        - dito for memcpy

5.  - lose stupid braces, e.g.

list_del_init(&(task->task_group_link));

should be just

list_del_init(&task->task_group_link);


6.  - no need to implement ->slave_alloc and ->slave_destroy if
        they are noops anyway.

7.  - kill that slab name randomization - if kmem_cache_destroy
        fails you module is buggy and needs to be fixed.

8.  - host and hba can't be NULL in ->queuecommand per defintion,
        similarly id, lun cdbsize and use_sg can't be bigger than you
        set in the template.

9.  - iscsi_info is far too noisy.  I'd suggest killing it and
        just setting the name field in the template propely

10.  - the procfs code is a mess.  Please move it over to
        proper per-device / per-host sysfs attributes as procfs
        support in HBA drivers is deprecated.

11.  - the ifdef MODULE around iscsi_init/iscsi_cleanup is bogus

12..  - lots of ifdef mess - iscsi_kernel.h still has lots of
          old kernel compat stuff - kill it.

13.  - target_reset_occured() is a dup of scsi_report_device_reset()

14.  - initialization is a total mess.  You try to emulated the
          pre-2.6 init ordering.  Use something like:

        shost = scsi_host_alloc();
        <content of ->detect>
        scsi_add_host()

        instead, and read the comment abote .legacy_hosts in the
        scsi_host_template defintion!  Similar inline the code
        in iscsi_release into the exitfunc.

15.  - iscsi.c is _far_ too big.  Needs to be split into a few
        files.  I'd suggest avoiding the name iscsi.c so you can
        build into iscsi.ko

16.  - the ioctl API looks very broken, I need more time to
        audit it.  As a start slit ctl_ioctl into a function per ioctl
        subcommand.

17.  - why can't you store a pointer to the session in host-private
        data of the scsi_device so you don't have to search in each
        ->queuecommand?

18.  - kill the is_root_disk special casing.  drivers can't know
        what the root disks is, there might not even be a single one.

19.  - compilation fails for me in line 480, looks like this is because
        the left-over ifdefs are wrong - just use set_user_nice 
unconditionally.
        Or even better stop messing with priorities, kill iscsi_daemonize and
        use daemonize() directly.

20.  - why do you have SLAB_NO_REAP optional?  Either it works or it doesn't..

21.  - you're only allocating one hba struct but still do complex list walking
        to find.  Either move to multiple hbas (and proper standard lists) or
        kill all that overhead.

22.  - what the heck is iscsi_set_if_addr?  You have absolutely no business
        messing with network device configuration.

23.  - device_flags() is strange.  Better put a separately allocated struct
        into the scsi_device (allocated in slave_alloc, freed in 
slave_destroy)
        that only contains the bitmask for now.  (and maybe the session, see
        above)

24.  - kfree(NULL) is fine, don't check for variables beeing non-NULL before
        freeing them.

25.  - this doesn't seem to be an issue for a 2.6-only driver:

 * not to log?  In 2.5 we can put aver..

26.  - shost->can_queue is not supposed to be changed once the host is online,
          as it's not locked down.

27.  - please produce a simple testcase for the bug mentioned above
          iscsi_eh_device_reset.

28.  - the bug mentioned above iscsi_eh_bus_reset should be long fixed.  if
        not please report to linux-scsi.

29.  - is the iscsi fake geometry documented somewhere?  If not why don't
        you simply use the default, CAM one?

30.  - lots of unchecked kmalloc()s

31. - Linux now has unshifted status codes.

32.  - kill ctl_open/ctl_release and move add an owner to the
        fops structure for refcounting.

33.  highlevel:
        - kill md5.c and iscsi-crc.c and use common cryptoapi
        md5 / common crc code instead.

34. README: - very outdated with 2.4/vendor issues

35. memset(session, 0, sizeof (*session));
    kfree(session);

        is rather bad for the cache.

36.  - please stop that preallocation, you destroy the effect of per-cpu
        caches with that.


CURRENTLY WORKING ON FOLLOWING COMMENTS:
---------------------------------------

1.  - the explanation of PREVENT_DATA_CORRUPTION doesn't make sense.

2.  highlevel:
        - kill scsi-crc.c and use common cryptoapi
         common crc code instead.

3. Makefile:
        - lots of unused kernel detection stuff

4.  - please produce a simple testcase for the bug mentioned above
          isidlayer queueing infrastructure.
        - dito for the reports luns and inquiry code in iscsi_detect_luns.
        I'm very very unhappy to see a lowlevel driver mess with all this.
        Even if you need to do it from a LLDD use the midlayer functionality
        for it.  But I still want to see a very good explanation why you need
        this a t all.

5.  - don't use <hosts.h> but <scsi/scsi_host.h>, similarly
        use <scsi/scsi*.h> instead of scsi.h if possible.  There's
        still some stuff not moved away from scsi.h yet, but you
        should at least avoid anything explicitly marked deprecated
        in that file and not in <scsi/scsi_*.h>

6.  - having multiple kmap() in the same process at the same time can kmap(),
        you redesign iscsi_xmit_task/iscsi_xmit_data/iscsi_recv_data not to do
        that. for the tx path use ->sendpage to avoid a data copy and kmapping
        altogether.

7.  - this comment indicates you don't want normal EH:

 * check condition with no sense.  We need to avoid this,
 * since the Linux SCSI code could put the command in
 * SCSI_STATE_FAILED, which it's error recovery doesn't appear
 * to handle correctly, and even if it does, we're trying to
 * bypass all of the Linux error recovery code
 * to avoid blocking all I/O to the HBA.  Fake some sense
 * data that gets a retry from Linux.
 
so don't use scsi_error.c and define your own eh_strategy_handler method.
 
8.  - please explain on this:
 
 * Philsophically we ought to complete the command and let the
 * midlayer or high-level driver deal with retries.  Since the
 * way the midlayer does retries is undesirable, we instead
 * keep the command in the driver, but requeue it for the same
 * cases the midlayer checks for retries.  This lets us ignore
 * the command's retry count, and do retries until the command
 * timer expires.
 
        why can't you simply mess with the retry count instead
        of duplicating all the retry logic.  really, the midlayer
        queues are there for a reason.
 
9.  - you can't allocate scsi_cmnds yourself but _must_ use scsi_get_command
 
10.  - dito for scsi_device.
 
11.  - the whole iscsi_do_cmnd use looks very very strange.  you're probably
        much better of to use the scsi_request based interfaces and use 
midlayer
        queueing and sometimes even higher level interfaces, e.g. your
        iscsi_send_tur could be replaced by an
        scsi_ioctl(sdp, SCSI_IOCTL_TEST_UNIT_READY, NULL)
        I still don't understand, though why the heck you need to do a TUR in
        a lowlevel driver and even more need a thread do a single TUR, started
        from another kerns of DRIVER_BYTE & co.

Thanks,
Naveen.

Re: Request for review of Linux iSCSI driver version 4.0.0.2

[Christoph Hellwig]

> 2.  iscis-kernel.h:
>         - only backward-compat cruft, should go away.

still too much left. the file should really be empty..
e.g. the HAS_ ifdefs should just go away, dito for all those
macros that aren't needed.

> 8.  - host and hba can't be NULL in ->queuecommand per defintion,
>         similarly id, lun cdbsize and use_sg can't be bigger than you
>         set in the template.

BUG_ON(host == NULL); isn't needed either, you'd get the same oops
when dereferencing a NULL pointer two lines later.

> 10.  - the procfs code is a mess.  Please move it over to
>         proper per-device / per-host sysfs attributes as procfs
>         support in HBA drivers is deprecated.

please don't add an attribute group, just use the shost_attrs field
in the host template. iscsi_show_device() doesn't look like a good
idea, what about adding an iscsi pseudo device as parent to identify
the device as iscsi in sysfs?

This whole find_keyword business is not good.  sysfs files are
supposed to have a single value.  We should probably do some
brainstorming on the right API for this..

RE: Request for review of Linux iSCSI driver version 4.0.0.2

[Surekha.PC]

>> 10.  - the procfs code is a mess.  Please move it over to
>>         proper per-device / per-host sysfs attributes as procfs
>>         support in HBA drivers is deprecated.

>please don't add an attribute group, just use the shost_attrs field in
the host template.
 
Can you please let me know why we should not use attribute group? Isn't
it recommended for the driver writers to use it?

The main reason for using this was to have all iSCSI specific files in
one place so that its' easier for the user to find them. There are many
iSCSI attributes which will need to be changed/read, so we preferred to
put in one place.

>iscsi_show_device() doesn't look like a good idea,what about adding an
iscsi pseudo 
device as parent to identify the device as iscsi in sysfs?

iscsi_show_device() creates sysfs file "device_type" which is checked
while parsing "udev.config" for iSCSI device activation. Since device
attributes like vendor, model etc reside in this directory, isn't it ok
to have it here?

If not, would like to know what field we need to use, for adding iscsi
peusdo device as parent, I doubt this can be done with shost_attrs and
sdev_attrs of host template.   

>This whole find_keyword business is not good.  sysfs files are supposed
to have a single >value.  We should probably do some brainstorming on
the right API for this..

Our driver has lot of user space attributes. Making each attribute as a
separate file will endup with lots of files. So we want to have all
target specific attributes together in one file and lun specific
attributes in another file for convinience. find_keyword() will help
search for these keys from the cmdline specified.

Pls let me know your suggestions.

Thanks,
surekha

Re: Request for review of Linux iSCSI driver version 4.0.0.2

['Christoph Hellwig']

On Mon, Dec 01, 2003 at 04:16:25PM +0530, Surekha.PC wrote:
> >please don't add an attribute group, just use the shost_attrs field in
> the host template.
>  
> Can you please let me know why we should not use attribute group? Isn't
> it recommended for the driver writers to use it?

I don't think so.  At least that's not how we designed the sysfs
interface for scsi.

> >iscsi_show_device() doesn't look like a good idea,what about adding an
> iscsi pseudo 
> device as parent to identify the device as iscsi in sysfs?
> 
> iscsi_show_device() creates sysfs file "device_type" which is checked
> while parsing "udev.config" for iSCSI device activation. Since device
> attributes like vendor, model etc reside in this directory, isn't it ok
> to have it here?

Well, the big question is what exactly does this device_type field
mean.  Currently it's only implemented for iscsi so rather meaningless.
What do you think would be the values for other types of HBAs (plain
parallel scsi, firewire/sbp2, usb-storage, fc, ide-scsi, sata?).

IMHO the better choice would be to implement a fake iscsi bus in
the driver model, then you'll just have to look what bus your
host resides on in sysfs.

> If not, would like to know what field we need to use, for adding iscsi
> peusdo device as parent, I doubt this can be done with shost_attrs and
> sdev_attrs of host template.   

just setup a bus_type for iscsi, create a fake struct device for it
(see scsi_debug.c for examples) and hand it as second argument to
scsi_add_host.

> >This whole find_keyword business is not good.  sysfs files are supposed
> to have a single >value.  We should probably do some brainstorming on
> the right API for this..
> 
> Our driver has lot of user space attributes. Making each attribute as a
> separate file will endup with lots of files.

Yes.

> So we want to have all
> target specific attributes together in one file and lun specific
> attributes in another file for convinience. find_keyword() will help
> search for these keys from the cmdline specified.

Well, the whole point of sysfs is to get out of the procfs debacle
by having a defined, easily parseable structure, and that is one
value per file.

RE: Request for review of Linux iSCSI driver version 4.0.0.2

[James Bottomley]

On Mon, 2003-12-01 at 04:46, Surekha.PC wrote:
> Our driver has lot of user space attributes. Making each attribute as a
> separate file will endup with lots of files. So we want to have all
> target specific attributes together in one file and lun specific
> attributes in another file for convinience. find_keyword() will help
> search for these keys from the cmdline specified.
> 
> Pls let me know your suggestions.

I count 35 total (for both host and target); that's well within the
range of the sysfs single file per value.  A lot of them also seem to be
debug attributes, so won't they go away (or at least only compile in
under a DEBUG #define) with the released product?

The idea behind sysfs is easily and uniformly to export data to the
user, and to import it from the user.  You could separate the properties
out into those which are useful to the end user, and those which are for
developers only.  If there's enough of the latter, we could ask the
sysfs people for a more flexible interface.

James