Re: [PATCH] device-mapper: multipath hardware handler for EMC

[Lars Marowsky-Bree <lmb@suse.de>]

On 2005-02-11T19:58:41, Christoph Hellwig <hch@infradead.org> wrote:

> > +/* Code borrowed from dm-lsi-rdac by Mike Christie */
> 
> Any reason that module isn't submitted?

No idea why.

> > +	bio->bi_bdev = path->dev->bdev;
> > +	bio->bi_sector = 0;
> > +	bio->bi_private = path;
> > +	bio->bi_end_io = emc_endio;
> > +
> > +	page = alloc_page(GFP_ATOMIC);
> > +	if (!page) {
> > +		DMERR("dm-emc: get_failover_bio: alloc_page() failed.");
> > +		bio_put(bio);
> > +		return NULL;
> > +	}
> > +
> > +	if (bio_add_page(bio, page, data_size, 0) != data_size) {
> > +		DMERR("dm-emc: get_failover_bio: alloc_page() failed.");
> > +		__free_page(page);
> > +		bio_put(bio);
> > +		return NULL;
> > +	}
> > +
> > +	return bio;
> 
> this would benefit from goto unwinding.

OK.

> > +	if (h->short_trespass) {
> > +		memcpy(page22, short_trespass_pg, data_size);
> > +	} else {
> > +		memcpy(page22, long_trespass_pg, data_size);
> > +	}
> 	 memcpy(page22, h->short_trespass ?
> 	 	short_trespass_pg : long_trespass_pg, data_size);
> 
> ?

Yes, I first did some other things there than just copying the commands
around, it can surely benefit from cleanup.

> > +static struct emc_handler *alloc_emc_handler(void)
> > +{
> > +	struct emc_handler *h = kmalloc(sizeof(*h), GFP_KERNEL);
> > +
> > +	if (h) {
> > +		h->lock = SPIN_LOCK_UNLOCKED;
> > +	}
> 
> 	if (h)
> 		spin_lock_init(&h->lock);

Came in via the copy, good catch.

> > +static unsigned emc_err(struct hw_handler *hwh, struct bio *bio)
> > +{
> > +	/* FIXME: Patch from axboe still missing */
> 
> it's in -mm now afaik??

No, it's not. That's the request sense keys, but here we're dealing with
the bio.

> > +#if 0
> > +	int sense;
> > +
> > +	if (bio->bi_error & BIO_SENSE) {
> > +		sense = bio->bi_error & 0xffffff; /* sense key / asc / ascq */
> > +
> > +		if (sense == 0x020403) {
> 
> please use the sense handling helpers from Doug Gilbert so you can handle
> the descriptor sense format aswell.  (And make the code a lot clear).

I'll go look them up.

> Also please try to use constants instead of magic numbers.

Noted. I'll clean this part up when I actually have sense keys to try,
so far this was mostly about getting that tiny bit of logic in.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business