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

[Erez Zilber]

>> >
>> > 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?
> 
> No, this is the same mechanism that we use for the rest of the files:
> check them out of the kernel tree.
> Look at file ofed_scripts/ofed_checkout.sh
> 
> But I stress that we can not do this for files under
> include/ *unless* they only include packet structure definitions.
> Otherwise we'll get weird data corruption on newer kernels.

See below.

> 
>> >
>> >> >> +
>> >> >> + 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.
> 
> OK then.
> What about the stuff like scsi.c?

I have the following files in backport/2.6.9_UX/include/src/:

attribute_container.c - almost identical to the file on 2.6.20. I had to change one line in it.

init.c - only a small part of the original file in 2.6.20

klist.c - almost identical to the file on 2.6.20. I had to change one line in it.

kref_new.c - based on kref.c

scsi.c - only a small part of the original file in 2.6.20

scsi_lib.c - only a small part of the original file in 2.6.20

scsi_scan.c - only a small part of the original file in 2.6.20

transport_class.c - identical to 2.6.20

So, the only file identical to 2.6.20 is transport_class.c. We can copy it from 2.6.20, but since it's only a single file, I'm not sure if it will make a real difference.

> 
>> >> 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.
> 
> Pls check other headers you pull in - is there something you can skip?

No.


>> >
>> > 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?
> 
> I do not have the time to look into this in a deep way.
> But it seems that you can just add a file libiscsi_f.c with
> 
> #include "libiscsi.c"
> 
> would this work?
> 
> --
> MST
> 

Yes, it works ok. I've updated my git tree. 

Do you think that there are other fixes to be made? Else, I'll be glad to have it in the next OFED rc.

Thanks,
Erez