[dm-devel] Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Edward Goggin]

On Friday, September 08, 2006 2:00 PM, Mike Christie wrote
> 
> Mike Christie wrote:
> > Edward Goggin wrote:
> >> +
> >> +	rq->buffer = rq->data = h->buffer;
> >> +	rq->data_len = len;
> >> +	rq->bio = rq->biotail = NULL;
> >>  
> > 
> > I think I only suggested that you use the block layer map 
> functions in
> > the previous review. Now I will say, use them and fix the 
> core code to
> > not allocate memory or use a mempool since it will also fix the path
> > testers at the same time :) The reason is that the scsi 
> layer is trying
> > to make every thing run with scatterlists. In 2.6.18, every place is
> > converted (they should be at least), so you should not be 
> adding another
> > place where we send down a data buffer like this.
> > 
> 
> And if we are going to continue to do some scsi processing in dm I
> already did some helpers you could base yourself off of here
> 
> http://www.cs.wisc.edu/~michaelc/block/dm/v4/

I was first trying to submit these changes to the upstream code
(I chose 2.6.18-rc4) since I this is what I thought you had
suggested that I do when we talked about it briefly at the dm bof
session at OLS.

Should I not be taking this tack?

> 
> 
> In dm_scsi_end_rq, you could handle errors like transient transport
> failures for all hw handlers at one tine.
> 
> http://www.cs.wisc.edu/~michaelc/block/dm/v4/02-dm-scsi-helpers.patch

Again, doesn't this imply a change from what we already agreed upon?
That is fine, I just need to know.

> 
> And in fact a lot of your patch has already been done with this patch
> 
> http://www.cs.wisc.edu/~michaelc/block/dm/v4/03-emc.patch a year ago.

I certainly read your patch set at that time it was sent since this is
how I learned about the availability of the blk_execute_rq_nowait
interface.  Yet, I certainly didn't mean to plagiarize any of your
code.  I apologize if I have inadvertently done so.  I definitely
remember having a difficult time getting the io initiated by dm-emc.c
to work at all with my changes.  Turns out I was trying to DMA from
kernel static sections (as is your code).  Yet, I don't remember
looking back at your patch set for inspiration to solve the problem.

The primary intention of my patch is to (1) allow dm-emc.c to
distinguish between dm and non-dm initiated multipath path group
switch events, (2) provide a dm-multipath hw-handler status callback
for dm-emc.c, and (3) provide a somewhat more informative error
logging capability for the dm-multipath hw-handler interface.

I was also intent on having the possibly multiple ios initiated by
dm-emc.c not involve allocating memory for request or bio structures
from mempools or otherwise.  Doing so is proving especially difficult.

> 
> We really need to change how dm-multipath development works IMHO.
> Alasdair is just too overworked. It is not fair to him to have so much
> work piled on top of him and not fair to the community to 
> have problems
> like this sit around for so long.
> 
> The basics of the hw handler framework were done when I did 
> my pg group
> callouts that improved on Joe's idea to put everything in the path
> selectors. It took a year after that to get hw handlers, 
> which made some
> of the same architectural mistakes as well as odd coding 
> choices that I
> made, in the kernel. And now for another year we have been sitting on
> these type of bugs and problems where we need some of the hw handler
> functionality in the scsi layer as well as dm layer.
> 
> With Alasdair having to maintain every driver that dm handles 
> and those
> drivers increasing in complexity the work load is too large for one
> person and this makes coordination with other subsystems impossible.
> This is a large problem, when for something like dm-multipath it needs
> help from the block layer, and low levels like scsi.
> 
> We really need a system like in the scsi layer where there are driver
> maintainers and then one core maintainer of the subsystem. The core
> maintainer has final (well ok community has final say I guess) say but
> trusts the low level driver maintainers to some extent. The low level
> driver maintainers still have to post patches for review to the
> community and maintainer but the core maintainer does not have to do a
> line by line review of the dm target module and learn all the lower
> level details of the sub-subsytem/lld/dm target module/transport/spec
> unless it affects dm core or it is a suspicious patch.
> 
> Just my 2 cents.

Good ideas.  This approach should scale more effectively.

>