dm core patches

Hi,

Here's the latest set of patches to core dm. Please apply.

Thanks,

- Joe

Export dm_vcalloc()

@@ -149,7 +149,7 @@
 	return 0;
 }
 
-static void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size)
+void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size)
 {
 	unsigned long size;
 	void *addr;

@@ -858,6 +858,7 @@
 }
 
 
+EXPORT_SYMBOL(dm_vcalloc);
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);
 EXPORT_SYMBOL(dm_table_event);

@@ -167,4 +167,6 @@
 int dm_stripe_init(void);
 void dm_stripe_exit(void);
 
+void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size);
+
 #endif

Lift to_bytes() and to_sectors() into dm.h

@@ -233,15 +233,6 @@
  *   interests of getting something for people to use I give
  *   you this clearly demarcated crap.
  *---------------------------------------------------------------*/
-static inline sector_t to_sector(unsigned int bytes)
-{
-	return bytes >> SECTOR_SHIFT;
-}
-
-static inline unsigned int to_bytes(sector_t sector)
-{
-	return sector << SECTOR_SHIFT;
-}
 
 /*
  * Decrements the number of outstanding ios that a bio has been

@@ -151,6 +151,16 @@
 	return dm_round_up(n, size) / size;
 }
 
+static inline sector_t to_sector(unsigned long n)
+{
+	return (n >> 9);
+}
+
+static inline unsigned long to_bytes(sector_t n)
+{
+	return (n << 9);
+}
+
 /*
  * The device-mapper can be driven through one of two interfaces;
  * ioctl or filesystem, depending which patch you have applied.

Get rid of struct dm_deferred_io in dm.c. [Chripstophe Saout]

@@ -27,11 +27,6 @@
 	atomic_t io_count;
 };
 
-struct deferred_io {
-	struct bio *bio;
-	struct deferred_io *next;
-};
-
 /*
  * Bits for the md->flags field.
  */

@@ -52,7 +47,7 @@
 	 */
 	atomic_t pending;
 	wait_queue_head_t wait;
-	struct deferred_io *deferred;
+	struct bio *deferred;
 
 	/*
 	 * The current mapping.

@@ -188,38 +183,20 @@
 	mempool_free(io, md->io_pool);
 }
 
-static inline struct deferred_io *alloc_deferred(void)
-{
-	return kmalloc(sizeof(struct deferred_io), GFP_NOIO);
-}
-
-static inline void free_deferred(struct deferred_io *di)
-{
-	kfree(di);
-}
-
 /*
  * Add the bio to the list of deferred io.
  */
 static int queue_io(struct mapped_device *md, struct bio *bio)
 {
-	struct deferred_io *di;
-
-	di = alloc_deferred();
-	if (!di)
-		return -ENOMEM;
-
 	down_write(&md->lock);
 
 	if (!test_bit(DMF_BLOCK_IO, &md->flags)) {
 		up_write(&md->lock);
-		free_deferred(di);
 		return 1;
 	}
 
-	di->bio = bio;
-	di->next = md->deferred;
-	md->deferred = di;
+	bio->bi_next = md->deferred;
+	md->deferred = bio;
 
 	up_write(&md->lock);
 	return 0;		/* deferred successfully */

@@ -743,14 +720,14 @@
 /*
  * Requeue the deferred bios by calling generic_make_request.
  */
-static void flush_deferred_io(struct deferred_io *c)
+static void flush_deferred_io(struct bio *c)
 {
-	struct deferred_io *n;
+	struct bio *n;
 
 	while (c) {
-		n = c->next;
-		generic_make_request(c->bio);
-		free_deferred(c);
+		n = c->bi_next;
+		c->bi_next = NULL;
+		generic_make_request(c);
 		c = n;
 	}
 }

@@ -832,7 +809,7 @@
 
 int dm_resume(struct mapped_device *md)
 {
-	struct deferred_io *def;
+	struct bio *def;
 
 	down_write(&md->lock);
 	if (!md->map ||

Am Di, den 10.02.2004 schrieb Joe Thornber um 17:59:

                                                  ^

Typo! :)

Make sure that we maintain ordering when deferring bios.

@@ -5,6 +5,7 @@
  */
 
 #include "dm.h"
+#include "dm-bio-list.h"
 
 #include <linux/init.h>
 #include <linux/module.h>

@@ -47,7 +48,7 @@
 	 */
 	atomic_t pending;
 	wait_queue_head_t wait;
-	struct bio *deferred;
+ 	struct bio_list deferred;
 
 	/*
 	 * The current mapping.

@@ -195,8 +196,7 @@
 		return 1;
 	}
 
-	bio->bi_next = md->deferred;
-	md->deferred = bio;
+	bio_list_add(&md->deferred, bio);
 
 	up_write(&md->lock);
 	return 0;		/* deferred successfully */

@@ -822,8 +822,7 @@
 	dm_table_resume_targets(md->map);
 	clear_bit(DMF_SUSPENDED, &md->flags);
 	clear_bit(DMF_BLOCK_IO, &md->flags);
-	def = md->deferred;
-	md->deferred = NULL;
+	def = bio_list_get(&md->deferred);
 	up_write(&md->lock);
 
 	flush_deferred_io(def);

Tidy up the error path for alloc_dev()

@@ -560,41 +560,28 @@
 
 	/* get a minor number for the dev */
 	r = persistent ? specific_minor(minor) : next_free_minor(&minor);
-	if (r < 0) {
-		kfree(md);
-		return NULL;
-	}
+	if (r < 0)
+		goto bad1;
 
 	memset(md, 0, sizeof(*md));
 	init_rwsem(&md->lock);
 	atomic_set(&md->holders, 1);
 
 	md->queue = blk_alloc_queue(GFP_KERNEL);
-	if (!md->queue) {
-		kfree(md);
-		return NULL;
-	}
+	if (!md->queue)
+		goto bad1;
 
 	md->queue->queuedata = md;
 	blk_queue_make_request(md->queue, dm_request);
 
 	md->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
 				     mempool_free_slab, _io_cache);
-	if (!md->io_pool) {
-		free_minor(minor);
-		blk_put_queue(md->queue);
-		kfree(md);
-		return NULL;
-	}
+ 	if (!md->io_pool)
+ 		goto bad2;
 
 	md->disk = alloc_disk(1);
-	if (!md->disk) {
-		mempool_destroy(md->io_pool);
-		free_minor(minor);
-		blk_put_queue(md->queue);
-		kfree(md);
-		return NULL;
-	}
+	if (!md->disk)
+		goto bad3;
 
 	md->disk->major = _major;
 	md->disk->first_minor = minor;

@@ -609,6 +596,16 @@
 	init_waitqueue_head(&md->eventq);
 
 	return md;
+
+
+ bad3:
+	mempool_destroy(md->io_pool);
+ bad2:
+	blk_put_queue(md->queue);
+	free_minor(minor);
+ bad1:
+	kfree(md);
+	return NULL;
 }
 
 static void free_dev(struct mapped_device *md)

With 32 bit sector_t the block device size _in bytes_ is also cut to 32 bit in __set_size when the block device is mount (a filesystem mounted). The argument should be cast to loff_t before expanding the sector count to a byte count and calling i_size_write.

[Christophe Saout]

@@ -639,7 +639,7 @@
 	bdev = bdget_disk(disk, 0);
 	if (bdev) {
 		down(&bdev->bd_inode->i_sem);
-		i_size_write(bdev->bd_inode, size << SECTOR_SHIFT);
+		i_size_write(bdev->bd_inode, (loff_t)size << SECTOR_SHIFT);
 		up(&bdev->bd_inode->i_sem);
 		bdput(bdev);
 	}

For some reason dm_table_create() was allocating GFP_NOIO rather than GFP_KERNEL.

@@ -205,7 +205,7 @@
 
 int dm_table_create(struct dm_table **result, int mode, unsigned num_targets)
 {
-	struct dm_table *t = kmalloc(sizeof(*t), GFP_NOIO);
+	struct dm_table *t = kmalloc(sizeof(*t), GFP_KERNEL);
 
 	if (!t)
 		return -ENOMEM;

Add sanity check to dm_table_add_target() against zero length targets. [Christophe Saout]

@@ -655,6 +655,11 @@
 	memset(tgt, 0, sizeof(*tgt));
 	set_default_limits(&tgt->limits);
 
+	if (!len) {
+		tgt->error = "zero-length target";
+		return -EINVAL;
+	}
+
 	tgt->type = dm_get_target_type(type);
 	if (!tgt->type) {
 		tgt->error = "unknown target type";

Remove redundant spin lock in dec_pending()

@@ -217,14 +217,8 @@
  */
 static inline void dec_pending(struct dm_io *io, int error)
 {
-	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
-	unsigned long flags;
-
-	if (error) {
-		spin_lock_irqsave(&_uptodate_lock, flags);
+	if (error)
 		io->error = error;
-		spin_unlock_irqrestore(&_uptodate_lock, flags);
-	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))

I just noticed that bio_clone copies the BIO_SEG_VALID bit from the original bio when it was set. When we modify bi_idx or bi_vcnt afterwards the segment counts are invalid and the bit must be dropped (though it is fairly unlikely that it has already been set). [Christophe Saout]

@@ -338,6 +338,7 @@
 	clone->bi_idx = idx;
 	clone->bi_vcnt = idx + bv_count;
 	clone->bi_size = to_bytes(len);
+	clone->bi_flags &= ~(1 << BIO_SEG_VALID);
 
 	return clone;
 }

Hi Joe,

when will you be submitting the DM multipath personality?

Sincerely,
Lars Marowsky-Brée <[email protected]>

High Availability & Clustering	      \ ever tried. ever failed. no matter.
SUSE Labs			      | try again. fail again. fail better.
Research & Development, SUSE LINUX AG \ 	-- Samuel Beckett

Not for a bit, it's still changing too much as I find more out about the hardware (see the [email protected] list).

- Joe

I checked the archives, but I couldn't find anything really 'in flux'. Your priority based approach seems just fine to me.

What is still missing? This is really a killer feature for 2.6. Any help I can offer?

Sincerely,
Lars Marowsky-Brée <[email protected]>

High Availability & Clustering	      \ ever tried. ever failed. no matter.
SUSE Labs			      | try again. fail again. fail better.
Research & Development, SUSE LINUX AG \ 	-- Samuel Beckett

I think the main concern now is over the testing of paths. Sending an io down an inactive path can be very expensive for some hardware configurations. So I'm considering changing a couple of things:

- Only ever send io to 1 priority group at a time (even test ios). To test the lower priority groups we'd have to periodically switch to them and use them for a bit for both test io and proper io.

- For some hardware there are better ways of testing the path than sending the test io. Should the drivers expose a test function ? In the absence of this we'd fallback to the test io method.

The other thing we need is to try and get the drivers to deferentiate between a media error and a path error, so that media errors get reported up quickly and don't cause false path failures. This is possibly an area that you could help with ?

- Joe

You are missing the obvious answer:

- Periodically checking paths is a user-space issue and doesn't belong into the kernel. User-space gets to handle this policy.

Again, with user-space taking care of this, it doesn't really matter.

Though exposing a test function does sound nice, even for user-space.

Moving it into kernel land is something which can always be done later, if there is a really pressing problem.

I thought the IO stack in 2.6 provided us with such sense keys already, which you'd then need to handle in the DM personality. Of course, drivers need to make sure they pass up appropriate sense-keys, but that's a hardware vendor issue and not something which should delay the DM personality...

Jens, do you have the pointer on this handy?

Sincerely,
Lars Marowsky-Brée <[email protected]>

High Availability & Clustering	      \ ever tried. ever failed. no matter.
SUSE Labs			      | try again. fail again. fail better.
Research & Development, SUSE LINUX AG \ 	-- Samuel Beckett

Yes, that is obvious, I had wanted to do failback automatically. But pushing it to userland does allow people to write hardware specific tests. I'll try it and see what people think.

Thanks,

- Joe

one thing you can do is provide a way for drivers to wake the userspace tester early. Say by default it polls every minute, but if the fiber channel driver gets a LIP UP event it (via a central API) makes the userspace daemon *now*.

I may be missing something obvious, but a LIP UP should be accompanied with a round of 'device detections' on that link, which already should trigger a few hotplug events, no?

So this seems pretty much solved.

Sincerely,
Lars Marowsky-Brée <[email protected]>

High Availability & Clustering	      \ ever tried. ever failed. no matter.
SUSE Labs			      | try again. fail again. fail better.
Research & Development, SUSE LINUX AG \ 	-- Samuel Beckett

not normaly; there are several reasons the loop can bounce briefly and right now the fiber drivers don't notify linux of that every time. Maybe that's for the better .... if it's a frequent thing that is short-timed then it would be obscene to yank the disks from under the user (and force-umount his fs) every few hours..

while in multipath you do want to at least stop using the current path if there is another path that is not in negotiation...

Be careful here. Your failback test packet cannot be a media access type as this could cause volume transition thrashing in some types of storage units so most likely you will use a test unit ready type packet. These small size tests are not very good checks on there own for optical based networks as the laser power needed to send them is really low (newer vertical cavity lasers have reduced these types of failures, but they still happens). Auto failback with heuristics and a credit based model allows the path to be failed back in with a quick ejection and a increasing time interval to start the whole cycle again. This keeps the systems from heading into a failover / failback storm.

-andmike
--
Michael Anderson
[email protected]

Right, such policy belongs to userpsace it seems.

The reason why I put it into the multipath target is to cover the case, where all paths are inoperational, the system is OOM _and_ the only chance to recover from that is the hope to unfail a path in order to release memory preasure.

'Sorry, userspace test handler can't run, your enterprise server is a pile of sh..' is not acceptable in case there's a path we could unfail IMO.

Regards,
Heinz -- The LVM Guy --

*** Software bugs are stupid.
Nevertheless it needs not so stupid people to solve them ***

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Heinz Mauelshagen Red Hat, Inc.
Consulting Development Engineer Am Sonnenhang 11
56242 Marienrachdorf Germany
[email protected] +49 2626 141200

                                                       FAX 924446

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

The mechanism is in place, but the SCSI stack still needs a few changes to pass down the correct errors. The easiest would be to pass down pseudo-sense keys (I'd rather just call them something else as not to confuse things, io error hints or something) to end_that_request_first(), changing uptodate from a bool to a hint.

I can help get this done, it's not something that should hold up dm-mp by any stretch.

Jens Axboe