[ofa-general] 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 general
mailing list