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

[Michael S. Tsirkin]

> Quoting Erez Zilber <erezz at voltaire.com>:
> Subject: Re: [PATCH 2/2] IB/iser: add backport & kernel addons for open-iscsi over iSER support for RHAS4 up3 and up4
> 
> Michael S. Tsirkin wrote:
> >> Quoting Erez Zilber <erezz at voltaire.com>:
> >> Subject: [PATCH 2/2] IB/iser: add backport & kernel addons for open-iscsi over iSER support for RHAS4 up3 and up4
> >>
> >>
> >> Add the required backport patches & kernel addons for open-iscsi
> >> over iSER in RHAS4 up3 and up4.
> >>
> >> Signed-off-by: Erez Zilber <erezz at voltaire.com>
> > 
> > In addition to posting patches, could you pls publish a git tree to pull from,
> > please? This makes it easy to test-build the patch as our build system
> > knows how to do git checkout.
> 
> 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.

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

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


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

-- 
MST