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

Michael S. Tsirkin mst at dev.mellanox.co.il
Mon May 21 04:44:10 PDT 2007


> Quoting Erez Zilber <erezz at voltaire.com>:
> Subject: Re: [PATCH 2/2] IB/iser: add backport & kernel addons for open-iscsiover iSER support for RHAS4 up3 and up4
> 
> >>
> >> 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?

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.

> > 
> >> >> +
> >> >> + 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?

> >> 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?

> > 
> >> --- 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?

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



More information about the ewg mailing list