[PATCH 7/17] bottom-layer barrier support (fwd)

[PATCH 7/17] bottom-layer barrier support (fwd)

[Mikulas Patocka]

dm-__clone_and_map_empty_barrier.patch

Introduce __clone_and_map_empty_barrier that will pass empty barrier
to the targets. Each target receives as many requests as it set up
in num_flush_requests.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Index: linux-2.6.30-rc2-devel/drivers/md/dm.c
===================================================================
--- linux-2.6.30-rc2-devel.orig/drivers/md/dm.c	2009-04-15 16:35:46.000000000 +0200
+++ linux-2.6.30-rc2-devel/drivers/md/dm.c	2009-04-15 16:35:49.000000000 +0200
@@ -761,6 +761,8 @@ static struct bio *clone_bio(struct bio 
 	return clone;
 }
 
+static int __clone_and_map_empty_barrier(struct clone_info *ci);
+
 static int __clone_and_map(struct clone_info *ci)
 {
 	struct bio *clone, *bio = ci->bio;
@@ -768,6 +770,9 @@ static int __clone_and_map(struct clone_
 	sector_t len = 0, max;
 	struct dm_target_io *tio;
 
+	if (unlikely(bio_empty_barrier(bio)))
+		return __clone_and_map_empty_barrier(ci);
+
 	ti = dm_table_find_target(ci->map, ci->sector);
 	if (!dm_target_is_valid(ti))
 		return -EIO;
@@ -861,6 +866,31 @@ static int __clone_and_map(struct clone_
 	return 0;
 }
 
+static int __clone_and_map_empty_barrier(struct clone_info *ci)
+{
+	unsigned i, j, n;
+	n = dm_table_get_num_targets(ci->map);
+	for (i = 0; i < n; i++) {
+		struct bio *clone;
+		struct dm_target *ti = dm_table_get_target(ci->map, i);
+		for (j = 0; j < ti->num_flush_requests; j++) {
+			struct dm_target_io *tio = alloc_tio(ci->md);
+			tio->io = ci->io;
+			tio->ti = ti;
+			memset(&tio->info, 0, sizeof(tio->info));
+			tio->info.flush_request = j;
+
+			clone = bio_alloc_bioset(GFP_NOIO, 0, ci->md->bs);
+			__bio_clone(clone, ci->bio);
+			clone->bi_destructor = dm_bio_destructor;
+
+			__map_bio(ti, clone, tio);
+		}
+	}
+	ci->sector_count = 0;
+	return 0;
+}
+
 /*
  * Split the bio into several clones and submit it to targets.
  */
@@ -888,6 +918,8 @@ static void __split_and_process_bio(stru
 	ci.io->md = md;
 	ci.sector = bio->bi_sector;
 	ci.sector_count = bio_sectors(bio);
+	if (unlikely(bio_empty_barrier(bio)))
+		ci.sector_count = 1;
 	ci.idx = bio->bi_idx;
 
 	start_io_acct(ci.io);

[PATCH 8/17] bottom-layer barrier support

[Mikulas Patocka]

dm-flush-make-empty-barrier.patch

Modify dm_flush so that is passes empty barrier flushes to the targets.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6.30-rc2-devel/drivers/md/dm.c
===================================================================
--- linux-2.6.30-rc2-devel.orig/drivers/md/dm.c	2009-04-15 16:35:49.000000000 +0200
+++ linux-2.6.30-rc2-devel/drivers/md/dm.c	2009-04-15 16:35:54.000000000 +0200
@@ -175,6 +175,9 @@ struct mapped_device {
 
 	/* sysfs handle */
 	struct kobject kobj;
+
+	/* zero-length barrier that will be cloned and submitted to targets */
+	struct bio barrier_bio;
 };
 
 #define MIN_IOS 256
@@ -1475,6 +1478,13 @@ static int dm_wait_for_completion(struct
 static void dm_flush(struct mapped_device *md)
 {
 	dm_wait_for_completion(md, TASK_UNINTERRUPTIBLE);
+
+	bio_init(&md->barrier_bio);
+	md->barrier_bio.bi_bdev = md->bdev;
+	md->barrier_bio.bi_rw = WRITE_BARRIER;
+	__split_and_process_bio(md, &md->barrier_bio);
+
+	dm_wait_for_completion(md, TASK_UNINTERRUPTIBLE);
 }
 
 static void process_barrier(struct mapped_device *md, struct bio *bio)

[PATCH 9/17] bottom-layer barrier support

[Mikulas Patocka]

dm-linear-flush.patch

Flush support for the linear target.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-linear.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.30-rc1-devel/drivers/md/dm-linear.c
===================================================================
--- linux-2.6.30-rc1-devel.orig/drivers/md/dm-linear.c	2009-04-10 06:32:54.000000000 +0200
+++ linux-2.6.30-rc1-devel/drivers/md/dm-linear.c	2009-04-10 06:33:36.000000000 +0200
@@ -53,6 +53,7 @@ static int linear_ctr(struct dm_target *
 		goto bad;
 	}
 
+	ti->num_flush_requests = 1;
 	ti->private = lc;
 	return 0;
 
@@ -81,7 +82,8 @@ static void linear_map_bio(struct dm_tar
 	struct linear_c *lc = ti->private;
 
 	bio->bi_bdev = lc->dev->bdev;
-	bio->bi_sector = linear_map_sector(ti, bio->bi_sector);
+	if (bio_sectors(bio))
+		bio->bi_sector = linear_map_sector(ti, bio->bi_sector);
 }
 
 static int linear_map(struct dm_target *ti, struct bio *bio,

[PATCH 10/17] bottom-layer barrier support

[Mikulas Patocka]

dm-stripe-flush.patch

Flush support for the stripe target.

This sets ti->num_flush_requests to the number of stripes and
remaps individual flushr requests to the appropriate stripe devices.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-stripe.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Index: linux-2.6.30-rc1-devel/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.30-rc1-devel.orig/drivers/md/dm-stripe.c	2009-04-10 06:08:40.000000000 +0200
+++ linux-2.6.30-rc1-devel/drivers/md/dm-stripe.c	2009-04-10 06:33:40.000000000 +0200
@@ -167,6 +167,7 @@ static int stripe_ctr(struct dm_target *
 	sc->stripes = stripes;
 	sc->stripe_width = width;
 	ti->split_io = chunk_size;
+	ti->num_flush_requests = stripes;
 
 	sc->chunk_mask = ((sector_t) chunk_size) - 1;
 	for (sc->chunk_shift = 0; chunk_size; sc->chunk_shift++)
@@ -211,10 +212,18 @@ static int stripe_map(struct dm_target *
 		      union map_info *map_context)
 {
 	struct stripe_c *sc = (struct stripe_c *) ti->private;
+	sector_t offset, chunk;
+	uint32_t stripe;
 
-	sector_t offset = bio->bi_sector - ti->begin;
-	sector_t chunk = offset >> sc->chunk_shift;
-	uint32_t stripe = sector_div(chunk, sc->stripes);
+	if (unlikely(bio_empty_barrier(bio))) {
+		BUG_ON(map_context->flush_request >= sc->stripes);
+		bio->bi_bdev = sc->stripe[map_context->flush_request].dev->bdev;
+		return DM_MAPIO_REMAPPED;
+	}
+
+	offset = bio->bi_sector - ti->begin;
+	chunk = offset >> sc->chunk_shift;
+	stripe = sector_div(chunk, sc->stripes);
 
 	bio->bi_bdev = sc->stripe[stripe].dev->bdev;
 	bio->bi_sector = sc->stripe[stripe].physical_start +

[PATCH 11/17] bottom-layer barrier support

[Mikulas Patocka]

dm-crypt-flush.patch

Flush support for dm-crypt target.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-crypt.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6.30-rc1-devel/drivers/md/dm-crypt.c
===================================================================
--- linux-2.6.30-rc1-devel.orig/drivers/md/dm-crypt.c	2009-04-10 06:28:45.000000000 +0200
+++ linux-2.6.30-rc1-devel/drivers/md/dm-crypt.c	2009-04-10 06:33:42.000000000 +0200
@@ -1168,6 +1168,7 @@ static int crypt_ctr(struct dm_target *t
 		goto bad_crypt_queue;
 	}
 
+	ti->num_flush_requests = 1;
 	ti->private = cc;
 	return 0;
 
@@ -1226,6 +1227,12 @@ static int crypt_map(struct dm_target *t
 {
 	struct dm_crypt_io *io;
 
+	if (unlikely(bio_empty_barrier(bio))) {
+		struct crypt_config *cc = ti->private;
+		bio->bi_bdev = cc->dev->bdev;
+		return DM_MAPIO_REMAPPED;
+	}
+
 	io = crypt_io_alloc(ti, bio, bio->bi_sector - ti->begin);
 
 	if (bio_data_dir(io->base_bio) == READ)

[PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

dm-delay-flush.patch

Flush supoprt for dm-delay target.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-delay.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.30-rc1-devel/drivers/md/dm-delay.c
===================================================================
--- linux-2.6.30-rc1-devel.orig/drivers/md/dm-delay.c	2009-04-10 06:28:25.000000000 +0200
+++ linux-2.6.30-rc1-devel/drivers/md/dm-delay.c	2009-04-10 06:33:45.000000000 +0200
@@ -198,6 +198,7 @@ out:
 	mutex_init(&dc->timer_lock);
 	atomic_set(&dc->may_delay, 1);
 
+	ti->num_flush_requests = 1;
 	ti->private = dc;
 	return 0;
 
@@ -281,6 +282,8 @@ static int delay_map(struct dm_target *t
 		bio->bi_bdev = dc->dev_write->bdev;
 		bio->bi_sector = dc->start_write +
 				 (bio->bi_sector - ti->begin);
+		if (!bio_sectors(bio))
+			bio->bi_sector = 0;
 
 		return delay_bio(dc, dc->write_delay, bio);
 	}

[PATCH 13/17] bottom-layer barrier support

[Mikulas Patocka]

dm-multipath-flush.patch

Flush support for dm-multipath target.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-mpath.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6.30-rc1-devel/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.30-rc1-devel.orig/drivers/md/dm-mpath.c	2009-04-10 06:28:46.000000000 +0200
+++ linux-2.6.30-rc1-devel/drivers/md/dm-mpath.c	2009-04-10 06:33:51.000000000 +0200
@@ -824,6 +824,8 @@ static int multipath_ctr(struct dm_targe
 		goto bad;
 	}
 
+	ti->num_flush_requests = 1;
+
 	return 0;
 
  bad:

[PATCH 14/17] bottom-layer barrier support

[Mikulas Patocka]

dm-snapshot-flush.patch

Flush support for dm-snapshot target.

This patch just forwars the flush request to origin and snapshot device,
it doesn't flush exception store metadata.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.30-rc1-devel/drivers/md/dm-snap.c
===================================================================
--- linux-2.6.30-rc1-devel.orig/drivers/md/dm-snap.c	2009-04-10 06:24:53.000000000 +0200
+++ linux-2.6.30-rc1-devel/drivers/md/dm-snap.c	2009-04-10 06:33:48.000000000 +0200
@@ -678,6 +678,7 @@ static int snapshot_ctr(struct dm_target
 
 	ti->private = s;
 	ti->split_io = s->store->chunk_size;
+	ti->num_flush_requests = 1;
 
 	return 0;
 
@@ -1030,6 +1031,11 @@ static int snapshot_map(struct dm_target
 	chunk_t chunk;
 	struct dm_snap_pending_exception *pe = NULL;
 
+	if (unlikely(bio_empty_barrier(bio))) {
+		bio->bi_bdev = s->store->cow->bdev;
+		return DM_MAPIO_REMAPPED;
+	}
+
 	chunk = sector_to_chunk(s->store, bio->bi_sector);
 
 	/* Full snapshots are not usable */
@@ -1338,6 +1344,8 @@ static int origin_ctr(struct dm_target *
 	}
 
 	ti->private = dev;
+	ti->num_flush_requests = 1;
+
 	return 0;
 }
 
@@ -1353,6 +1361,9 @@ static int origin_map(struct dm_target *
 	struct dm_dev *dev = ti->private;
 	bio->bi_bdev = dev->bdev;
 
+	if (unlikely(bio_empty_barrier(bio)))
+		return DM_MAPIO_REMAPPED;
+
 	/* Only tell snapshots if this is a write */
 	return (bio_rw(bio) == WRITE) ? do_origin(dev, bio) : DM_MAPIO_REMAPPED;
 }

[PATCH 15/17] bottom-layer barrier support

[Mikulas Patocka]

dm-dmio-eopnotsupp-bits.patch

Add another field, eopnotsupp_bits. It is subset of error_bits, it represents
regions that returned -EOPNOTSUPP error (in this case, the bit is set in both
error_bits and eopnotsupp_bits).

This value will be used in further patches.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-io.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6.29-rc1-devel/drivers/md/dm-io.c
===================================================================
--- linux-2.6.29-rc1-devel.orig/drivers/md/dm-io.c	2009-01-22 04:14:13.000000000 +0100
+++ linux-2.6.29-rc1-devel/drivers/md/dm-io.c	2009-01-23 19:04:42.000000000 +0100
@@ -22,6 +22,7 @@ struct dm_io_client {
 /* FIXME: can we shrink this ? */
 struct io {
 	unsigned long error_bits;
+	unsigned long eopnotsupp_bits;
 	atomic_t count;
 	struct task_struct *sleeper;
 	struct dm_io_client *client;
@@ -107,8 +108,11 @@ static inline unsigned bio_get_region(st
  *---------------------------------------------------------------*/
 static void dec_count(struct io *io, unsigned int region, int error)
 {
-	if (error)
+	if (error) {
 		set_bit(region, &io->error_bits);
+		if (error == -EOPNOTSUPP)
+			set_bit(region, &io->eopnotsupp_bits);
+	}
 
 	if (atomic_dec_and_test(&io->count)) {
 		if (io->sleeper)
@@ -361,6 +365,7 @@ static int sync_io(struct dm_io_client *
 	}
 
 	io.error_bits = 0;
+	io.eopnotsupp_bits = 0;
 	atomic_set(&io.count, 1); /* see dispatch_io() */
 	io.sleeper = current;
 	io.client = client;
@@ -397,6 +402,7 @@ static int async_io(struct dm_io_client 
 
 	io = mempool_alloc(client->pool, GFP_NOIO);
 	io->error_bits = 0;
+	io->eopnotsupp_bits = 0;
 	atomic_set(&io->count, 1); /* see dispatch_io() */
 	io->sleeper = NULL;
 	io->client = client;

[PATCH 16/17] bottom-layer barrier support

[Mikulas Patocka]

dm-dmio-retry-barriers.patch

If -EOPNOTSUPP was returned and the request was a barrier request, retry it
without barrier.

Retry all regions (if there was just one region returning -EOPNOTSUPP), for
now, barriers are submitted only for 1-region requests, so it doesn't matter.

In the future, this can be fine-grained to retry only -EOPNOTSUPPed regions.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-io.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-2.6.29-rc1-devel/drivers/md/dm-io.c
===================================================================
--- linux-2.6.29-rc1-devel.orig/drivers/md/dm-io.c	2009-01-22 21:09:28.000000000 +0100
+++ linux-2.6.29-rc1-devel/drivers/md/dm-io.c	2009-01-22 21:09:37.000000000 +0100
@@ -364,6 +364,7 @@ static int sync_io(struct dm_io_client *
 		return -EIO;
 	}
 
+retry:
 	io.error_bits = 0;
 	io.eopnotsupp_bits = 0;
 	atomic_set(&io.count, 1); /* see dispatch_io() */
@@ -382,6 +383,11 @@ static int sync_io(struct dm_io_client *
 	}
 	set_current_state(TASK_RUNNING);
 
+	if (io.eopnotsupp_bits && (rw & (1 << BIO_RW_BARRIER))) {
+		rw &= ~(1 << BIO_RW_BARRIER);
+		goto retry;
+	}
+
 	if (error_bits)
 		*error_bits = io.error_bits;
 

[PATCH 17/17] bottom-layer barrier support

[Mikulas Patocka]

dm-snapshot-sync-exception-store.patch

Send barrier requests when updating the exception area.

Exception area updates need to be ordered w.r.t. data writes, so that
the writes are not reordered in hardware disk cache.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.30-rc2-devel/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-2.6.30-rc2-devel.orig/drivers/md/dm-snap-persistent.c	2009-04-10 06:16:42.000000000 +0200
+++ linux-2.6.30-rc2-devel/drivers/md/dm-snap-persistent.c	2009-04-20 08:41:18.000000000 +0200
@@ -636,7 +636,7 @@ static void persistent_commit_exception(
 	/*
 	 * Commit exceptions to disk.
 	 */
-	if (ps->valid && area_io(ps, WRITE))
+	if (ps->valid && area_io(ps, WRITE_BARRIER))
 		ps->valid = 0;
 
 	/*

Re: [PATCH 7/17] bottom-layer barrier support (fwd)

[Alasdair G Kergon]

On Mon, Apr 20, 2009 at 03:57:43AM -0400, Mikulas Patocka wrote:
> +static int __clone_and_map_empty_barrier(struct clone_info *ci)
> +{
> +	unsigned i, j, n;
> +	n = dm_table_get_num_targets(ci->map);
> +	for (i = 0; i < n; i++) {
> +		struct bio *clone;
> +		struct dm_target *ti = dm_table_get_target(ci->map, i);
> +		for (j = 0; j < ti->num_flush_requests; j++) {
> +			struct dm_target_io *tio = alloc_tio(ci->md);
> +			tio->io = ci->io;
> +			tio->ti = ti;
> +			memset(&tio->info, 0, sizeof(tio->info));
> +			tio->info.flush_request = j;
> +
> +			clone = bio_alloc_bioset(GFP_NOIO, 0, ci->md->bs);
> +			__bio_clone(clone, ci->bio);
> +			clone->bi_destructor = dm_bio_destructor;
> +
> +			__map_bio(ti, clone, tio);

I'm editing this one a bit, splitting that middle section out into a separate
function to improve readability.  (The compiler can optimise it back if it wants.)

Alasdair
-- 
agk@redhat.com

Re: [PATCH 8/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 20, 2009 at 03:58:20AM -0400, Mikulas Patocka wrote:
> dm-flush-make-empty-barrier.patch
> Modify dm_flush so that is passes empty barrier flushes to the targets.
 
> +	md->barrier_bio.bi_bdev = md->bdev;

There is no md->bdev in my tree - have I missed a dependency here?

Alasdair
-- 
agk@redhat.com

Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 20, 2009 at 04:01:11AM -0400, Mikulas Patocka wrote:
> Flush supoprt for dm-delay target.
 
> @@ -281,6 +282,8 @@ static int delay_map(struct dm_target *t
>  		bio->bi_bdev = dc->dev_write->bdev;
>  		bio->bi_sector = dc->start_write +
>  				 (bio->bi_sector - ti->begin);
> +		if (!bio_sectors(bio))
> +			bio->bi_sector = 0;

Why set bio->bi_sector twice on that path?
Should we instead guarantee it's 0 before calling?

>  
>  		return delay_bio(dc, dc->write_delay, bio);
>  	}

Is flushing different - do we really want to delay it?
(I'd have thought not.  But if so, should it be a separate option?)

Alasdair
-- 
agk@redhat.com

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 10:48:37PM +0100, Alasdair G Kergon wrote:
> On Mon, Apr 20, 2009 at 04:01:11AM -0400, Mikulas Patocka wrote:
> > Flush supoprt for dm-delay target.
> > @@ -281,6 +282,8 @@ static int delay_map(struct dm_target *t
> >  		bio->bi_sector = dc->start_write +
> >  				 (bio->bi_sector - ti->begin);

In fact, does bi_sector have a meaningful value within
the context of a specific target any more when processing
a barrier flush, or is it a bug if a target attempts to access it?
(For example, it could be set to ti->begin or 0.)

I think the situation deserves defining in a comment.

(Maybe we will remove "- ti->begin" from all the mapping functions one day.
Not mixed in with this patch set though.)

Alasdair
-- 
agk@redhat.com

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 10:48:37PM +0100, Alasdair G Kergon wrote:
> On Mon, Apr 20, 2009 at 04:01:11AM -0400, Mikulas Patocka wrote:
> > Flush supoprt for dm-delay target.

> >  		return delay_bio(dc, dc->write_delay, bio);

Oh - and how does this interact with read_delay?

Alasdair
-- 
agk@redhat.com

Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Mon, 27 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 20, 2009 at 04:01:11AM -0400, Mikulas Patocka wrote:
> > Flush supoprt for dm-delay target.
>  
> > @@ -281,6 +282,8 @@ static int delay_map(struct dm_target *t
> >  		bio->bi_bdev = dc->dev_write->bdev;
> >  		bio->bi_sector = dc->start_write +
> >  				 (bio->bi_sector - ti->begin);
> > +		if (!bio_sectors(bio))
> > +			bio->bi_sector = 0;
> 
> Why set bio->bi_sector twice on that path?
> Should we instead guarantee it's 0 before calling?

For non-barrier requests, it needs to be translated according to the 
table.

For zero-barrier request, it should be 0. (well, non-zero values may work, 
but I'm setting it zero, all other kernel code sets it to zero too).

> >  
> >  		return delay_bio(dc, dc->write_delay, bio);
> >  	}
> 
> Is flushing different - do we really want to delay it?
> (I'd have thought not.  But if so, should it be a separate option?)

Depending how you want it. I'd see it as a write and delay it as a write. 
If dm-delay is to be used for example for debugging, it should have a 
capability to delay all requests, including flushes.

> Alasdair
> -- 
> agk@redhat.com

Mikulas

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Mon, 27 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 27, 2009 at 10:48:37PM +0100, Alasdair G Kergon wrote:
> > On Mon, Apr 20, 2009 at 04:01:11AM -0400, Mikulas Patocka wrote:
> > > Flush supoprt for dm-delay target.
> > > @@ -281,6 +282,8 @@ static int delay_map(struct dm_target *t
> > >  		bio->bi_sector = dc->start_write +
> > >  				 (bio->bi_sector - ti->begin);
> 
> In fact, does bi_sector have a meaningful value within
> the context of a specific target any more when processing
> a barrier flush, or is it a bug if a target attempts to access it?
> (For example, it could be set to ti->begin or 0.)
> 
> I think the situation deserves defining in a comment.
> 
> (Maybe we will remove "- ti->begin" from all the mapping functions one day.
> Not mixed in with this patch set though.)

The code is correct as it is.

For example, if you have first 10 sectors mapped on linear target and next 
sectors mapped on delay target, you must subtract 10 from the value 
(-ti->begin) and add the start offset dc->start_write.

If you map the whole device as one target, this is meaningless. But if you 
map more targets into one table, this arithmetics is required.

The difference is the zero-barrier flush request --- it should have 
bi_sector always 0.

Mikulas

> Alasdair
> -- 
> agk@redhat.com
> 

Re: [PATCH 8/17] bottom-layer barrier support

[Mikulas Patocka]

On Mon, 27 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 20, 2009 at 03:58:20AM -0400, Mikulas Patocka wrote:
> > dm-flush-make-empty-barrier.patch
> > Modify dm_flush so that is passes empty barrier flushes to the targets.
>  
> > +	md->barrier_bio.bi_bdev = md->bdev;
> 
> There is no md->bdev in my tree - have I missed a dependency here?
> 
> Alasdair

That is from two patches:
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-bdev-rename-suspended_bdev-to-bdev.patch
and
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-bdev-keep-bdev-always-referenced.patch

They are old and fix memory-allocation-while-suspended violation.

Do you want to accept them?

There should be bdev value of the md device in bi_bdev, most targets don't 
care about it, but for correctness, we should supply a correct value.

If you don't want to accept the correct patches, we must make up some 
dummy value for bi_bdev and review targets that they don't care about the 
value (I consider it somehow hacky to place dummy values to bio fields for 
targets, and I'd rather put the "keep-bdev-always-referenced" patch in).

Another possibility would be to get bdev from submitted barrier request.

Which of these ways do you want to go?

Mikulas

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 07:09:32PM -0400, Mikulas Patocka wrote:
> The code is correct as it is.
 
I don't see that.

> The difference is the zero-barrier flush request --- it should have 
> bi_sector always 0.

But I'm asking at what point that is defined?
0 before calling the target's map function?
In which case it is wrong to perform:
	bio->bi_sector = dc->start_write + (bio->bi_sector - ti->begin);

Or if it's not set to zero until the
	bio->bi_sector = 0 
line, then what does it hold on entry to the function and what does that
mean?

Alasdair
-- 
agk@redhat.com

Re: [PATCH 8/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 07:21:01PM -0400, Mikulas Patocka wrote:
> That is from two patches:

> Do you want to accept them?
 
I don't know - please submit them to patchwork and I'll consider them.

Alasdair
-- 
agk@redhat.com

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

In other words, I'm asking for it to be defined and documented what the 
target map function receives as input for one of these flush requests.

Alasdair
-- 
agk@redhat.com

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Tue, 28 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 27, 2009 at 07:09:32PM -0400, Mikulas Patocka wrote:
> > The code is correct as it is.
>  
> I don't see that.
> 
> > The difference is the zero-barrier flush request --- it should have 
> > bi_sector always 0.
> 
> But I'm asking at what point that is defined?

It is not defined, the Linux way is that there is no specification and the 
code is considered the specification.

The point is that any code in the Linux kernel that creates zero-length 
barrier creates it with sector number 0. So I'm doing it the same way. I 
have no capability to review all the drivers if they accept zero-barriers 
with non-zero sector number or not. So I set it to zero.

> 0 before calling the target's map function?
> In which case it is wrong to perform:
> 	bio->bi_sector = dc->start_write + (bio->bi_sector - ti->begin);
> 
> Or if it's not set to zero until the
> 	bio->bi_sector = 0 
> line, then what does it hold on entry to the function and what does that
> mean?

On entry to the function it holds zero if it is processing zero-length 
barrier. The line "bio->bi_sector = dc->start_write + (bio->bi_sector - 
ti->begin)" can set it to non-zero, so I set it to zero again. If you 
want, you can somehow rearrange the code so that "bio->bi_sector = 
dc->start_write + (bio->bi_sector - ti->begin)" is performed only if the 
request has non-zero length.

Mikulas

> Alasdair
> -- 
> agk@redhat.com
> 

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Tue, 28 Apr 2009, Alasdair G Kergon wrote:

> In other words, I'm asking for it to be defined and documented what the 
> target map function receives as input for one of these flush requests.
> 
> Alasdair
> -- 
> agk@redhat.com

It receives what it's set in dm_flush and __clone_and_map_empty_barrier.

bio is standard zero barrier with set bi_bdev and bi_rw. 
tio->info.flush_request is the sequence number of the request. (typical 
devices require just one request, dm_stripe asks for several requests for 
each striped device).

Mikulas

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Mon, 27 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 27, 2009 at 10:48:37PM +0100, Alasdair G Kergon wrote:
> > On Mon, Apr 20, 2009 at 04:01:11AM -0400, Mikulas Patocka wrote:
> > > Flush supoprt for dm-delay target.
> 
> > >  		return delay_bio(dc, dc->write_delay, bio);
> 
> Oh - and how does this interact with read_delay?

There is separate read_delay and write_delay. Flushes are governed by 
write_delay.

Mikulas

> Alasdair
> -- 
> agk@redhat.com
> 

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 07:32:00PM -0400, Mikulas Patocka wrote:
> On entry to the function it holds zero if it is processing zero-length 
> barrier. 

OK - so that is a clear definition the writer of a target map function can
rely upon, good.

> The line "bio->bi_sector = dc->start_write + (bio->bi_sector - 
> ti->begin)" can set it to non-zero, so I set it to zero again. If you 
> want, you can somehow rearrange the code so that "bio->bi_sector = 
> dc->start_write + (bio->bi_sector - ti->begin)" is performed only if the 
> request has non-zero length.
 
Please do that.  In dm I try to enforce a rule that the values of variables
always correspond to their definitions - if a calculation needs intermediate
steps then different and meaningful intermediate variables are used
instead.  Here, bi_sector seems to be set to a meaningless intermediate
value which is a recipe for confusion.

Alasdair
-- 
agk@redhat.com

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Tue, 28 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 27, 2009 at 07:32:00PM -0400, Mikulas Patocka wrote:
> > On entry to the function it holds zero if it is processing zero-length 
> > barrier. 
> 
> OK - so that is a clear definition the writer of a target map function can
> rely upon, good.
> 
> > The line "bio->bi_sector = dc->start_write + (bio->bi_sector - 
> > ti->begin)" can set it to non-zero, so I set it to zero again. If you 
> > want, you can somehow rearrange the code so that "bio->bi_sector = 
> > dc->start_write + (bio->bi_sector - ti->begin)" is performed only if the 
> > request has non-zero length.
>  
> Please do that.  In dm I try to enforce a rule that the values of variables
> always correspond to their definitions - if a calculation needs intermediate
> steps then different and meaningful intermediate variables are used
> instead.  Here, bi_sector seems to be set to a meaningless intermediate
> value which is a recipe for confusion.

OK.

Mikulas

> Alasdair
> -- 
> agk@redhat.com
> 

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 07:41:47PM -0400, Mikulas Patocka wrote:
> There is separate read_delay and write_delay. Flushes are governed by 
> write_delay.
 
And the value of read_delay has no impact on the flush?

(Because of the "wait for all outstanding I/O" bit that eventually I
want to see removed to facilitate true barrier passing?)

Alasdair
-- 
agk@redhat.com

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Tue, 28 Apr 2009, Alasdair G Kergon wrote:

> On Mon, Apr 27, 2009 at 07:41:47PM -0400, Mikulas Patocka wrote:
> > There is separate read_delay and write_delay. Flushes are governed by 
> > write_delay.
>  
> And the value of read_delay has no impact on the flush?

No.

> (Because of the "wait for all outstanding I/O" bit that eventually I
> want to see removed to facilitate true barrier passing?)

With dm-delay, "true barrier passing" doesn't have sense. Dm-delay is 
designed to delay requests, so no one wants high performance from it :)

> Alasdair
> -- 
> agk@redhat.com

Mikulas

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Mikulas Patocka]

On Mon, 27 Apr 2009, Mikulas Patocka wrote:

> 
> 
> On Tue, 28 Apr 2009, Alasdair G Kergon wrote:
> 
> > On Mon, Apr 27, 2009 at 07:32:00PM -0400, Mikulas Patocka wrote:
> > > On entry to the function it holds zero if it is processing zero-length 
> > > barrier. 
> > 
> > OK - so that is a clear definition the writer of a target map function can
> > rely upon, good.
> > 
> > > The line "bio->bi_sector = dc->start_write + (bio->bi_sector - 
> > > ti->begin)" can set it to non-zero, so I set it to zero again. If you 
> > > want, you can somehow rearrange the code so that "bio->bi_sector = 
> > > dc->start_write + (bio->bi_sector - ti->begin)" is performed only if the 
> > > request has non-zero length.
> >  
> > Please do that.  In dm I try to enforce a rule that the values of variables
> > always correspond to their definitions - if a calculation needs intermediate
> > steps then different and meaningful intermediate variables are used
> > instead.  Here, bi_sector seems to be set to a meaningless intermediate
> > value which is a recipe for confusion.
> 
> OK.
> 
> Mikulas

Here it is:

Flush supoprt for dm-delay target.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-delay.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6.30-rc2-devel/drivers/md/dm-delay.c
===================================================================
--- linux-2.6.30-rc2-devel.orig/drivers/md/dm-delay.c	2009-04-20 09:53:22.000000000 +0200
+++ linux-2.6.30-rc2-devel/drivers/md/dm-delay.c	2009-04-28 01:44:08.000000000 +0200
@@ -198,6 +198,7 @@ out:
 	mutex_init(&dc->timer_lock);
 	atomic_set(&dc->may_delay, 1);
 
+	ti->num_flush_requests = 1;
 	ti->private = dc;
 	return 0;
 
@@ -279,8 +280,9 @@ static int delay_map(struct dm_target *t
 
 	if ((bio_data_dir(bio) == WRITE) && (dc->dev_write)) {
 		bio->bi_bdev = dc->dev_write->bdev;
-		bio->bi_sector = dc->start_write +
-				 (bio->bi_sector - ti->begin);
+		if (bio_sectors(bio))
+			bio->bi_sector = dc->start_write +
+					 (bio->bi_sector - ti->begin);
 
 		return delay_bio(dc, dc->write_delay, bio);
 	}

Re: Re: [PATCH 12/17] bottom-layer barrier support

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 07:49:44PM -0400, Mikulas Patocka wrote:
> Here it is:
 
> @@ -279,8 +280,9 @@ static int delay_map(struct dm_target *t
>  
>  	if ((bio_data_dir(bio) == WRITE) && (dc->dev_write)) {
>  		bio->bi_bdev = dc->dev_write->bdev;
> -		bio->bi_sector = dc->start_write +
> -				 (bio->bi_sector - ti->begin);
> +		if (bio_sectors(bio))
> +			bio->bi_sector = dc->start_write +
> +					 (bio->bi_sector - ti->begin);
  
Thanks:-)
Now it looks similar to the code in the other simple targets again.

Alasdair
-- 
agk@redhat.com