[ewg] Re: [PATCH 2/2] IB/iser: add backport & kernel addons for open-iscsiover iSER support for RHAS4 up3 and up4

[Erez Zilber]

>>
>> Added a git tree:
>>
>> http://www.openfabrics.org/git/?p=~erezz/ofed_1_2_iser_rh4.git;a=summary
> 
> Looks reasonable.
> 
> However, you are copying a ton of files from upstream kernel.
> Sticking extra files in include might interfere with newer
> kernels, so I don't have better ideas for this for 1.2
> (for 1.3 I am hoping we'll use the submodule support in git,
> so we'll be able to re-use headers as well).
> 
> But, for files *not* in "include/", I suggest that, instead of sticking our
> own version in addons, we should check out the files from upstream and tweak
> makefiles to pick them up: maintaining these in OFED tree long-term will
> be a
> problem.

Do you suggest to add a new mechanism to OFED that will do that?

> 
>> >> +
>> >> + struct iscsi_internal {
>> >> +  int daemon_pid;
>> >> +@@ -65,6 +69,8 @@ static DEFINE_SPINLOCK(iscsi_transport_l
>> >> + #define cdev_to_iscsi_internal(_cdev) \
>> >> +  container_of(_cdev, struct iscsi_internal, cdev)
>> >> +
>> >> ++extern int attribute_container_init(void);
>> >> ++
>> >
>> > This does not look scsi-related. Why does this belong here?
>>
>> This is a hack. In 2.6.20, attribute_container_init is called from
> drivers/base/init.c. Since I cannot do that, I'm calling it from the
> init function in scsi_transport_iscsi (because scsi_transport_iscsi uses
> the attribute container). Do you have a better suggestion?
> 
> Aha. No better ideas for the header, let it be for now.
> But the code in drivers/base/init.c can be checked out rather than
> copied over.

I'm using only a very small part of init.c. I'm not sure that we should copy it.

> 
>> diff --git a/kernel_addons/backport/2.6.9_U4/include/linux/memory.h
> b/kernel_addons/backport/2.6.9_U4/include/linux/memory.h
>> new file mode 100644
>> index 0000000..654ef55
>> --- /dev/null
>> +++ b/kernel_addons/backport/2.6.9_U4/include/linux/memory.h
>> @@ -0,0 +1,89 @@
>> +/*
>> + * include/linux/memory.h - generic memory definition
>> + *
>> + * This is mainly for topological representation. We define the
>> + * basic "struct memory_block" here, which can be embedded in per-arch
>> + * definitions or NUMA information.
>> + *
>> + * Basic handling of the devices is done in drivers/base/memory.c
>> + * and system devices are handled in drivers/base/sys.c.
>> + *
>> + * Memory block are exported via sysfs in the class/memory/devices/
>> + * directory.
>> + *
>> + */
> 
> 
> Sorry, why are we copying this here?
> Are you actually trying to work with hotplug memory?

Sorry, it seems that I don't really need memory.h. It was included from init.c, but it is not necessary. I made the fix on ofed_1_2_iser_rh4.git.

> 
> 
>> --- a/kernel_patches/fixes/iscsi_scsi_makefile.patch
>> +++ /dev/null
>> @@ -1,10 +0,0 @@
>> -Add a Makefile based on the kernel's drivers/scsi/Makefile in order
> to build open-iscsi.
>> -
>> -Signed-off-by: Erez Zilber <erezz at voltaire.com>
>> -
>> -diff -ruN ofa_1_2_kernel-20061228-0200/drivers/scsi/Makefile
> ofa_1_2_kernel-20061228-0200-open-iscsi/drivers/scsi/Makefile
>> ---- ofa_1_2_kernel-20061228-0200/drivers/scsi/Makefile  1970-01-01
> 02:00:00.000000000 +0200
>> -+++
> ofa_1_2_kernel-20061228-0200-open-iscsi/drivers/scsi/Makefile      
> 2006-12-28 17:01:22.000000000 +0200
>> -@@ -0,0 +1,2 @@
>> -+obj-$(CONFIG_SCSI_ISCSI_ATTRS) += scsi_transport_iscsi.o
>> -+obj-$(CONFIG_ISCSI_TCP)        += libiscsi.o   iscsi_tcp.o
>> diff --git a/ofed_scripts/makefile b/ofed_scripts/makefile
>> index 34a8996..62abe2c 100644
>> --- a/ofed_scripts/makefile
>> +++ b/ofed_scripts/makefile
>> @@ -60,6 +60,12 @@ kernel:
>>       @echo "Kernel version: $(KVERSION)"
>>       @echo "Modules directory: $(DESTDIR)/$(MODULES_DIR)"
>>       @echo "Kernel sources: $(KSRC)"
>> +     if [ -e $(CWD)/drivers/scsi/libiscsi.c ]; then \
>> +             mv $(CWD)/drivers/scsi/libiscsi.c
> $(CWD)/drivers/scsi/libiscsi_f.c; \
>> +     fi
>> +     if [ -e $(CWD)/drivers/scsi/scsi_transport_iscsi.c ]; then \
>> +             mv $(CWD)/drivers/scsi/scsi_transport_iscsi.c
> $(CWD)/drivers/scsi/scsi_transport_iscsi_f.c; \
>> +     fi
>>       env EXTRA_CFLAGS="$(OPENIB_KERNEL_EXTRA_CFLAGS)
> $(KERNEL_MEMTRACK_CFLAGS) -I$(CWD)/include
> -I$(CWD)/drivers/infiniband/include \
>>               -I$(CWD)/drivers/infiniband/ulp/ipoib \
>>               -I$(CWD)/drivers/infiniband/debug \
> 
> This looks pretty hacky. Moving files around during make will
> interfere with people trying to e.g. create a patch.
> What is this doing? Can't makefile just point to the right files?
> 

Here's the problem:

I'm trying to build a module that contains multiple object files (e.g. libiscsi). libiscsi contains libiscsi.o & scsi_scan.o. Something like:

libiscsi-y             := libiscsi_f.o scsi_scan.o

The problem is that if I'm doing something like:

libiscsi-y             := libiscsi.o scsi_scan.o

then, libiscsi.ko doesn't contain the symbols from libiscsi.o (only symbols from scsi_scan.o). We found 2 solutions for this problem:

1. Change the module name - this is problematic because open-iscsi startup script uses the original module name.
2. Change the file name (libiscsi.c -> libiscsi_f.c) - this is what I did.

I don't really like this hack, but I wasn't able to come up with something better. Do you know how to overcome this problem?

Thanks,
Erez