Help with implementing some form of barriers in 3.0 kernels.

PATCH In review Konrad Rzeszutek Wilk 6 participants 25 messages Sep 7, 2011 – Sep 16, 2011

Subsystems (2)

Block layer Xen block

lore

Share & tools

lore

Konrad Rzeszutek Wilk

author

2011-09-07 17:50 UTC #0 lore raw

Hey Christoph,

I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?

Or is there some other things that we should take in consideration?

Thanks!

Vivek Goyal

2011-09-07 18:17 UTC #1 lore raw

Quoted 7 lines · Konrad Rzeszutek Wilk

Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?

I think WRITE_FLUSH_FUA is not same as WRITE_BARRIER. Because it does not ensure request ordering. A request rq2 which is issued after rq1 (with WRITE_flush_FUA), can still finish before rq1. In the past WRITE_BARRIER would not allow that.

So AFAIK, WRITE_flush_fua is not WRITE_BARRIER.

Thanks
Vivek

Konrad Rzeszutek Wilk

author

2011-09-07 18:29 UTC #2 lore raw

Quoted 9 lines · Vivek Goyal

Quoted 7 lines · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?

Quoted 6 lines

I think WRITE_FLUSH_FUA is not same as WRITE_BARRIER. Because it does not ensure request ordering. A request rq2 which is issued after rq1 (with WRITE_flush_FUA), can still finish before rq1. In the past WRITE_BARRIER would not allow that.
So AFAIK, WRITE_flush_fua is not WRITE_BARRIER.

Ok, any thoughts on how to emulate it then perhaps? Mark each request after rq1 with WRITE_FUA? .. But then how long should the _FUA bit be set - perhaps until the rq1 has completed?

Quoted 3 lines

Thanks
Vivek

Vivek Goyal

2011-09-07 18:36 UTC #3 lore raw

Quoted 15 lines · Konrad Rzeszutek Wilk

Quoted 14 lines · Vivek Goyal
Quoted 7 lines · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?
I think WRITE_FLUSH_FUA is not same as WRITE_BARRIER. Because it does not ensure request ordering. A request rq2 which is issued after rq1 (with WRITE_flush_FUA), can still finish before rq1. In the past WRITE_BARRIER would not allow that.

Quoted 5 lines

Quoted 1 line
So AFAIK, WRITE_flush_fua is not WRITE_BARRIER.
Ok, any thoughts on how to emulate it then perhaps? Mark each request after rq1 with WRITE_FUA? .. But then how long should the _FUA bit be set - perhaps until the rq1 has completed?

I think sender of the request need to wait for the completion of rq1 before issuing rq2 for emulating request ordering.

Thanks
Vivek

Christoph Hellwig

2011-09-07 20:16 UTC #4 lore raw

In reply to Vivek Goyal

[Hmm, for some reason I never manage to receive Konrads mails directly,
but only get the replies, or copies via the list]

Quoted 9 lines · Vivek Goyal

Quoted 7 lines · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?

Quoted 6 lines

I think WRITE_FLUSH_FUA is not same as WRITE_BARRIER. Because it does not ensure request ordering. A request rq2 which is issued after rq1 (with WRITE_flush_FUA), can still finish before rq1. In the past WRITE_BARRIER would not allow that.
So AFAIK, WRITE_flush_fua is not WRITE_BARRIER.

Indeed. And while most guests won't care some will. E.g. reiserfs which is the standard filesystem in most SuSE guests, which happen to be fairly popular with Xen.

I'd suggest you look at the pre-2.6.36 barrier implementation and see if you can move that into xen-blkfront.

For the qemu side doing this is a bit easier as you'll just have to wait for all pending aio requests to complete. The current qemu xen disk code gets thus horribly wrong, though.

Vivek Goyal

2011-09-07 21:31 UTC #5 lore raw

Quoted 24 lines · Christoph Hellwig

[Hmm, for some reason I never manage to receive Konrads mails directly,
but only get the replies, or copies via the list]
Quoted 9 lines · Vivek Goyal
Quoted 7 lines · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?
Quoted 6 lines
I think WRITE_FLUSH_FUA is not same as WRITE_BARRIER. Because it does not ensure request ordering. A request rq2 which is issued after rq1 (with WRITE_flush_FUA), can still finish before rq1. In the past WRITE_BARRIER would not allow that.
So AFAIK, WRITE_flush_fua is not WRITE_BARRIER.
Indeed. And while most guests won't care some will. E.g. reiserfs which is the standard filesystem in most SuSE guests, which happen to be fairly popular with Xen.

Quoted 6 lines

I'd suggest you look at the pre-2.6.36 barrier implementation and see if you can move that into xen-blkfront.
For the qemu side doing this is a bit easier as you'll just have to wait for all pending aio requests to complete. The current qemu xen disk code gets thus horribly wrong, though.

I have a basic question. In old guest why BARRIER handling on request queue is not sufficient for sequencing and ordering of requests and why xen-blkfront and qemu have to do something about it.

I am also wondering if virtio-blk have similar issues?

Thanks
Vivek

Christoph Hellwig

2011-09-08 08:22 UTC #6 lore raw

Quoted 5 lines · Vivek Goyal

I have a basic question. In old guest why BARRIER handling on request queue is not sufficient for sequencing and ordering of requests and why xen-blkfront and qemu have to do something about it.
I am also wondering if virtio-blk have similar issues?

Mainline xen-blkfront was using ORDERED_TAG before, which meant bypassing the sequencer entirely, as well as not beeing able to implement plain cache flushes as used by ->fsync.

Jan Beulich

2011-09-08 08:02 UTC #7 lore raw

In reply to Christoph Hellwig

Quoted 19 lines

Quoted 1 line
Quoted 1 line · Christoph Hellwig
[Hmm, for some reason I never manage to receive Konrads mails directly,
but only get the replies, or copies via the list]
Quoted 14 lines · Vivek Goyal
Quoted 7 lines · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?
I think WRITE_FLUSH_FUA is not same as WRITE_BARRIER. Because it does not ensure request ordering. A request rq2 which is issued after rq1 (with WRITE_flush_FUA), can still finish before rq1. In the past WRITE_BARRIER would not allow that.

Quoted 8 lines

Quoted 1 line
So AFAIK, WRITE_flush_fua is not WRITE_BARRIER.
Indeed. And while most guests won't care some will. E.g. reiserfs which is the standard filesystem in most SuSE guests, which happen to be fairly popular with Xen.
I'd suggest you look at the pre-2.6.36 barrier implementation and see if you can move that into xen-blkfront.

That would need to be done in the backend, as the expectation cannot be to patch old frontends to deal with new backends.

Jan

Quoted 3 lines

For the qemu side doing this is a bit easier as you'll just have to wait for all pending aio requests to complete. The current qemu xen disk code gets thus horribly wrong, though.

Christoph Hellwig

2011-09-08 08:08 UTC #8 lore raw

Quoted 2 lines · Jan Beulich

That would need to be done in the backend, as the expectation cannot be to patch old frontends to deal with new backends.

Sorry, I meant blkback above.

Jan Beulich

2011-09-13 10:44 UTC #9 lore raw

In reply to Konrad Rzeszutek Wilk

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?

Quoted 3 lines

Or is there some other things that we should take in consideration?
Thanks!

Below is what Jan Kara came up with for addressing this - what do you think?

Jan

From: Jan Kara <[email protected]>
Date: Sun, 11 Sep 2011 14:39:18 +0200
Subject: [PATCH] xen: Add support for old BARRIER requests to xenblk driver

Recent kernels do not support BARRIER operation but only FLUSH operation. But older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host crash.

Signed-off-by Jan Kara <[email protected]>

block/elevator.c +1 -0

@@ -619,6 +619,7 @@ void elv_drain_elevator(struct request_q
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +14 -1

@@ -530,6 +530,7 @@ static int dispatch_rw_block_io(struct x
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -541,11 +542,12 @@ static int dispatch_rw_block_io(struct x
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -603,6 +605,17 @@ static int dispatch_rw_block_io(struct x
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

Konrad Rzeszutek Wilk

author

2011-09-14 08:59 UTC #10 lore raw

Quoted 9 lines · Jan Beulich

Quoted 9 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?

Quoted 6 lines

Quoted 3 lines
Or is there some other things that we should take in consideration?
Thanks!
Below is what Jan Kara came up with for addressing this - what do you think?

It looks like it would do it. I modified it a bit, testing it now.

commit 315c0cf1a5174b9aed494d7903133ce9ac76d6f1

Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100

xen: Add support for old BARRIER requests to xenblk driver

Signed-off-by Jan Kara <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

[v1: Added some extra functions, and other cases]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..b2143a8 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +19 -1

index 9713d5a..1df773c 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		 (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
 			 " error=%d\n", error);

@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -668,6 +675,17 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +2 -0

index bfb532e..42b0e46 100644
@@ -228,6 +228,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +16 -0

index 2b3aef0..b477aee 100644
@@ -421,6 +421,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;

@@ -706,6 +720,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Jan Beulich

2011-09-14 09:12 UTC #11 lore raw

Quoted 34 lines

Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
Quoted 9 lines · Jan Beulich
Quoted 9 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
Hey Christoph,
I was wondering what you think is the proper way of implementing a backend to support the 'barrier' type requests? We have this issue were there are 2.6.36 type guests that still use barriers and we would like to support them properly. But in 3.0 there are no barriers - hence the question whether WRITE_fLUSH_FUA would be equal to WRITE_BARRIER?
Quoted 6 lines
Quoted 3 lines
Or is there some other things that we should take in consideration?
Thanks!
Below is what Jan Kara came up with for addressing this - what do you think?
It looks like it would do it. I modified it a bit, testing it now.
commit 315c0cf1a5174b9aed494d7903133ce9ac76d6f1
Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100
xen: Add support for old BARRIER requests to xenblk driver
Recent kernels do not support BARRIER operation but only FLUSH
operation. But
older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host
crash.

Quoted 3 lines

Signed-off-by Jan Kara <[email protected]>
Signed-off-by Jan Beulich <[email protected]>
[v1: Added some extra functions, and other cases]

Ah, yes, of course - Jan probably didn't pay attention because in our variant of the driver these functions never got removed.

block/elevator.c void elv_drain_elevator(struct request_queue *q) · line 619

Quoted 11 lines

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..b2143a8 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL(elv_drain_elevator);

Now, if you modify it anyway, how about making this an EXPORT_SYMBOL_GPL()?

Jan

Quoted 115 lines

block/elevator.c +0 -0

 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +5 -0

index 9713d5a..1df773c 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req 
 *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		 (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"

drivers/block/xen-blkback/blkback.c +14 -1

 			 " error=%d\n", error);

@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -668,6 +675,17 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +2 -0

index bfb532e..42b0e46 100644
@@ -228,6 +228,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +14 -0

index 2b3aef0..b477aee 100644
@@ -421,6 +421,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction 
 xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info 
 *be)

{

drivers/block/xen-blkback/xenbus.c +2 -0

 	struct xenbus_device *dev = be->dev;

@@ -706,6 +720,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Konrad Rzeszutek Wilk

author

2011-09-14 09:30 UTC #12 lore raw

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Below is what Jan Kara came up with for addressing this - what do you think?

Jens,

Could you take a look at the block/elevator patch below?

Quoted 17 lines

Quoted 17 lines
It looks like it would do it. I modified it a bit, testing it now.
commit 315c0cf1a5174b9aed494d7903133ce9ac76d6f1
Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100
xen: Add support for old BARRIER requests to xenblk driver
Recent kernels do not support BARRIER operation but only FLUSH
operation. But
older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host
crash.

Quoted 6 lines

Quoted 3 lines
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Jan Beulich <[email protected]>
[v1: Added some extra functions, and other cases]
Ah, yes, of course - Jan probably didn't pay attention because in our variant of the driver these functions never got removed.

<nods> Which is OK - but you might want to migrate in the the 'feature-write-flush'

Quoted 15 lines

block/elevator.c void elv_drain_elevator(struct request_queue *q) · line 619
Quoted 12 lines
Signed-off-by Konrad Rzeszutek Wilk <[email protected]>
block/elevator.c +1 -0
index a3b64bc..b2143a8 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL(elv_drain_elevator);
Now, if you modify it anyway, how about making this an EXPORT_SYMBOL_GPL()?

The rest of that file had EXPORT_SYMBOL.. so I figured I will follow the same appearance. But I am OK doing it via EXPORT_SYMBOL_GPL. Lets ask Jens.

Quoted 119 lines

Jan

Quoted 115 lines

block/elevator.c +0 -0

 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +5 -0

index 9713d5a..1df773c 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req 
 *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		 (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"

drivers/block/xen-blkback/blkback.c +14 -1

 			 " error=%d\n", error);

@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -668,6 +675,17 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +2 -0

index bfb532e..42b0e46 100644
@@ -228,6 +228,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +14 -0

index 2b3aef0..b477aee 100644
@@ -421,6 +421,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction 
 xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info 
 *be)

{

drivers/block/xen-blkback/xenbus.c +2 -0

 	struct xenbus_device *dev = be->dev;

@@ -706,6 +720,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Jan Beulich

2011-09-14 10:15 UTC #13 lore raw

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines · Konrad Rzeszutek Wilk
[v1: Added some extra functions, and other cases]
Ah, yes, of course - Jan probably didn't pay attention because in our variant of the driver these functions never got removed.
<nods> Which is OK - but you might want to migrate in the the 'feature-write-flush'

We really have both. I just never removed the barrier one.

Jan

Konrad Rzeszutek Wilk

author

2011-09-14 14:33 UTC #14 lore raw

In reply to Jan Beulich

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +10 -0

+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
 
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}

I also had to add:

drivers/block/xen-blkback/blkback.c +4 -0

index eaf49d1..20fddbc 100644
@@ -679,6 +679,10 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		struct request_queue *q = bdev_get_queue(preq.bdev);
 		unsigned long flags;
 
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
 		/* Emulate the original behavior of write barriers */
 		spin_lock_irqsave(q->queue_lock, flags);
 		elv_drain_elevator(q);

drivers/block/xen-blkback/common.h +1 -0

index 52d8893..722714a 100644
@@ -157,6 +157,7 @@ struct xen_vbd {
 	/* Cached size parameter. */
 	sector_t		size;
 	bool			flush_support;
+	bool			barrier_support;
 };
 
 struct backend_info;

drivers/block/xen-blkback/xenbus.c +4 -1

index da1e27a..7189ecd 100644
@@ -384,6 +384,9 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 	if (q && q->flush_flags)
 		vbd->flush_support = true;
 
+	if (q && q->elevator && q->elevator->ops)
+		vbd->barrier_support = true;
+
 	DPRINTK("Successful creation of handle=%04x (dom=%u)\n",
 		handle, blkif->domid);
 	return 0;

@@ -728,7 +731,7 @@ again:
 	if (err)
 		goto abort;
 
-	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.barrier_support);
 
 	err = xen_blkbk_discard(xbt, be);

Otherwise it would crash.. Thought I am not sure why the elevator is not set (the guest is on a LVM LV).

Konrad Rzeszutek Wilk

author

2011-09-14 15:01 UTC #15 lore raw

PATCH 1/1 : xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

On Wed, Sep 14, 2011 at 10:32:47AM -0400, Konrad Rzeszutek Wilk wrote:

Let me just post the one that I've succesfully tested. Not sure about the need for the check for q->elvator in the dispatch_rw_block_io so if you guys want me to remove it I can certainly do it.

commit a5a12f729697161522ecae0564915cf4145cb65c

Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100

xen/blkback: Add support for old BARRIER requests - 'feature-barrier'

Signed-off-by Jan Kara <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

[v1: Added some extra functions, and other cases]

[v2: Added check for q->elevator]

[v3: Changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..4e6c5b6 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL_GPL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +23 -1

index 9713d5a..573744e 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		   (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
 			 " error=%d\n", error);

@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -668,6 +675,21 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +3 -0

index bfb532e..9f1990b 100644
@@ -157,6 +157,7 @@ struct xen_vbd {
 	/* Cached size parameter. */
 	sector_t		size;
 	bool			flush_support;
+	bool			barrier_support;
 };
 
 struct backend_info;

@@ -228,6 +229,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +19 -0

index 2b3aef0..ec0975b 100644
@@ -376,6 +376,9 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 	if (q && q->flush_flags)
 		vbd->flush_support = true;
 
+	if (q && q->elevator)
+		vbd->barrier_support = true;
+
 	DPRINTK("Successful creation of handle=%04x (dom=%u)\n",
 		handle, blkif->domid);
 	return 0;

@@ -421,6 +424,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;

@@ -706,6 +723,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.barrier_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Jan Beulich

2011-09-14 16:13 UTC #16 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Quoted 84 lines

Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
On Wed, Sep 14, 2011 at 10:32:47AM -0400, Konrad Rzeszutek Wilk wrote:
Let me just post the one that I've succesfully tested. Not sure about the need for the check for q->elvator in the dispatch_rw_block_io so if you guys want me to remove it I can certainly do it.
commit a5a12f729697161522ecae0564915cf4145cb65c
Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100
xen/blkback: Add support for old BARRIER requests - 'feature-barrier'
Recent kernels do not support BARRIER operation but only FLUSH
operation. But
older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host
crash.
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Jan Beulich <[email protected]>
[v1: Added some extra functions, and other cases]
[v2: Added check for q->elevator]
[v3: Changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL]
Signed-off-by Konrad Rzeszutek Wilk <[email protected]>
block/elevator.c +1 -0
index a3b64bc..4e6c5b6 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL_GPL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled
drivers/block/xen-blkback/blkback.c +5 -0
index 9713d5a..573744e 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req 
 *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		   (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
drivers/block/xen-blkback/blkback.c +3 -1
 			 " error=%d\n", error);
@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {
@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;
@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;
@@ -668,6 +675,21 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}

drivers/block/xen-blkback/blkback.c static int dispatch_rw_block_io(struct xen_blkif *blkif, · line 685

Quoted 10 lines

drivers/block/xen-blkback/blkback.c +8 -0

 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}

You shouldn't return here - barrier requests may be "non-empty" (i.e. carry data to be written), and hence you'd need to make sure the writes get carried out nevertheless.

Jan - this suggests that the placement isn't correct either: The draining of the queue - as I understand it - should be happening *after* these writes completed, not before they get started.

Jan

Quoted 80 lines

drivers/block/xen-blkback/blkback.c +7 -0

+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +3 -0

index bfb532e..9f1990b 100644
@@ -157,6 +157,7 @@ struct xen_vbd {
 	/* Cached size parameter. */
 	sector_t		size;
 	bool			flush_support;
+	bool			barrier_support;
 };
 
 struct backend_info;

@@ -228,6 +229,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +3 -0

index 2b3aef0..ec0975b 100644
@@ -376,6 +376,9 @@ static int xen_vbd_create(struct xen_blkif *blkif, 
 blkif_vdev_t handle,
 	if (q && q->flush_flags)
 		vbd->flush_support = true;
 
+	if (q && q->elevator)
+		vbd->barrier_support = true;
+
 	DPRINTK("Successful creation of handle=%04x (dom=%u)\n",
 		handle, blkif->domid);

drivers/block/xen-blkback/xenbus.c +14 -0

 	return 0;

@@ -421,6 +424,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction 
 xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info 
 *be)

{

drivers/block/xen-blkback/xenbus.c +2 -0

 	struct xenbus_device *dev = be->dev;

@@ -706,6 +723,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.barrier_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Konrad Rzeszutek Wilk

author

2011-09-15 12:51 UTC #17 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Quoted 4 lines

(quoted diff) +4 -0

+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +4 -0
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
You shouldn't return here - barrier requests may be "non-empty" (i.e. carry data to be written), and hence you'd need to make sure the writes get carried out nevertheless.

In theory (and in practice) the 'feature-barrier' is set to zero if !q->elevator. So the frontend should not even try WRITE_BARRIER.

I think the better approach will be to outright fail the WRITE_BARRIER request if 'feature-barrier=0' instead of just trying (which is what this tries).

On a unrelated topic is what to do with devices which are bio based but not request based. From the DM directory, only multipath is capable of doing requests, while the rest are all bio. There is code to quisce the bio's: dm_suspend, but it is not apparent to me how to make it be exposed.

Quoted 4 lines

Jan - this suggests that the placement isn't correct either: The draining of the queue - as I understand it - should be happening *after* these writes completed, not before they get started.

So that looks like it should be just moved a bit, like this new patch:

commit 315c0cf1a5174b9aed494d7903133ce9ac76d6f1

Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100

xen: Add support for old BARRIER requests to xenblk driver

Signed-off-by Jan Kara <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

[v1: Added some extra functions, and other cases]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..b2143a8 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +19 -1

index 9713d5a..1df773c 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		 (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
 			 " error=%d\n", error);

@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -668,6 +675,17 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +2 -0

index bfb532e..42b0e46 100644
@@ -228,6 +228,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +16 -0

index 2b3aef0..b477aee 100644
@@ -421,6 +421,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;

@@ -706,6 +720,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Jan Beulich

2011-09-15 13:00 UTC #18 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Quoted 29 lines

Quoted 5 lines
Quoted 5 lines · Konrad Rzeszutek Wilk
drivers/block/xen-blkback/blkback.c +4 -0
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
You shouldn't return here - barrier requests may be "non-empty" (i.e. carry data to be written), and hence you'd need to make sure the writes get carried out nevertheless.
In theory (and in practice) the 'feature-barrier' is set to zero if !q->elevator. So the frontend should not even try WRITE_BARRIER.
I think the better approach will be to outright fail the WRITE_BARRIER request if 'feature-barrier=0' instead of just trying (which is what this tries).
On a unrelated topic is what to do with devices which are bio based but not request based. From the DM directory, only multipath is capable of doing requests, while the rest are all bio. There is code to quisce the bio's: dm_suspend, but it is not apparent to me how to make it be exposed.
Quoted 1 line

Quoted 5 lines

Quoted 3 lines
Jan - this suggests that the placement isn't correct either: The draining of the queue - as I understand it - should be happening *after* these writes completed, not before they get started.
So that looks like it should be just moved a bit, like this new patch:

Looks like this is the patch after the very first modifications you made to it - I don't see anything being moved, and I no longer see any of the subsequent changes you did.

Jan

Quoted 142 lines

commit 315c0cf1a5174b9aed494d7903133ce9ac76d6f1

Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100

xen: Add support for old BARRIER requests to xenblk driver

Recent kernels do not support BARRIER operation but only FLUSH
operation. But
older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host
crash.

Signed-off-by Jan Kara <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

[v1: Added some extra functions, and other cases]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..b2143a8 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +5 -0

index 9713d5a..1df773c 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req 
 *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		 (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"

drivers/block/xen-blkback/blkback.c +14 -1

 			 " error=%d\n", error);

@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {

@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -668,6 +675,17 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

drivers/block/xen-blkback/common.h +2 -0

index bfb532e..42b0e46 100644
@@ -228,6 +228,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +14 -0

index 2b3aef0..b477aee 100644
@@ -421,6 +421,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction 
 xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info 
 *be)

{

drivers/block/xen-blkback/xenbus.c +2 -0

 	struct xenbus_device *dev = be->dev;

@@ -706,6 +720,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Konrad Rzeszutek Wilk

author

2011-09-15 14:22 UTC #19 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Quoted 33 lines · Jan Beulich

Quoted 33 lines
Quoted 5 lines
Quoted 5 lines · Konrad Rzeszutek Wilk
(quoted diff) +4 -0
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
You shouldn't return here - barrier requests may be "non-empty" (i.e. carry data to be written), and hence you'd need to make sure the writes get carried out nevertheless.
In theory (and in practice) the 'feature-barrier' is set to zero if !q->elevator. So the frontend should not even try WRITE_BARRIER.
I think the better approach will be to outright fail the WRITE_BARRIER request if 'feature-barrier=0' instead of just trying (which is what this tries).
On a unrelated topic is what to do with devices which are bio based but not request based. From the DM directory, only multipath is capable of doing requests, while the rest are all bio. There is code to quisce the bio's: dm_suspend, but it is not apparent to me how to make it be exposed.
Quoted 4 lines
Jan - this suggests that the placement isn't correct either: The draining of the queue - as I understand it - should be happening *after* these writes completed, not before they get started.

Quoted 5 lines

Quoted 1 line
So that looks like it should be just moved a bit, like this new patch:
Looks like this is the patch after the very first modifications you made to it - I don't see anything being moved, and I no longer see any of the subsequent changes you did.

duh!
commit 9e8970f64bceffd90113f6cbb426af93cb7043dd

Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100

xen/blkback: Add support for old BARRIER requests - 'feature-barrier'

Signed-off-by Jan Kara <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

[v1: Added some extra functions, and other cases]

[v2: Added check for q->elevator]

[v3: Changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL]

[v4: Moved the drain _after_ the write request and fix failling]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..4e6c5b6 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL_GPL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +18 -2

index 9713d5a..738966e 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		   (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
 			 " error=%d\n", error);

@@ -601,6 +606,9 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		if (!blkif->vbd.barrier_support)
+			goto fail_response;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -667,7 +674,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 			goto fail_response;
 		}
 	}
-
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

@@ -746,6 +752,16 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	else if (operation == WRITE || operation == WRITE_FLUSH)
 		blkif->st_wr_sect += preq.nr_sects;
 
+	if (req->operation == BLKIF_OP_WRITE_BARRIER) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
 	return 0;
 
  fail_flush:

drivers/block/xen-blkback/common.h +3 -0

index bfb532e..9f1990b 100644
@@ -157,6 +157,7 @@ struct xen_vbd {
 	/* Cached size parameter. */
 	sector_t		size;
 	bool			flush_support;
+	bool			barrier_support;
 };
 
 struct backend_info;

@@ -228,6 +229,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +19 -0

index 2b3aef0..ec0975b 100644
@@ -376,6 +376,9 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 	if (q && q->flush_flags)
 		vbd->flush_support = true;
 
+	if (q && q->elevator)
+		vbd->barrier_support = true;
+
 	DPRINTK("Successful creation of handle=%04x (dom=%u)\n",
 		handle, blkif->domid);
 	return 0;

@@ -421,6 +424,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;

@@ -706,6 +723,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.barrier_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Jan Kara

2011-09-15 15:13 UTC #20 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Quoted 52 lines · Konrad Rzeszutek Wilk

Quoted 33 lines · Jan Beulich
Quoted 33 lines
Quoted 5 lines
Quoted 5 lines · Konrad Rzeszutek Wilk
drivers/block/xen-blkback/blkback.c +4 -0
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
You shouldn't return here - barrier requests may be "non-empty" (i.e. carry data to be written), and hence you'd need to make sure the writes get carried out nevertheless.
In theory (and in practice) the 'feature-barrier' is set to zero if !q->elevator. So the frontend should not even try WRITE_BARRIER.
I think the better approach will be to outright fail the WRITE_BARRIER request if 'feature-barrier=0' instead of just trying (which is what this tries).
On a unrelated topic is what to do with devices which are bio based but not request based. From the DM directory, only multipath is capable of doing requests, while the rest are all bio. There is code to quisce the bio's: dm_suspend, but it is not apparent to me how to make it be exposed.
Quoted 4 lines
Jan - this suggests that the placement isn't correct either: The draining of the queue - as I understand it - should be happening *after* these writes completed, not before they get started.
Quoted 5 lines
Quoted 1 line
So that looks like it should be just moved a bit, like this new patch:
Looks like this is the patch after the very first modifications you made to it - I don't see anything being moved, and I no longer see any of the subsequent changes you did.
duh!
commit 9e8970f64bceffd90113f6cbb426af93cb7043dd
Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100
xen/blkback: Add support for old BARRIER requests - 'feature-barrier'
Recent kernels do not support BARRIER operation but only FLUSH operation. But older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host crash.

Quoted 6 lines

Signed-off-by Jan Kara <[email protected]>
Signed-off-by Jan Beulich <[email protected]>
[v1: Added some extra functions, and other cases]
[v2: Added check for q->elevator]
[v3: Changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL]
[v4: Moved the drain _after_ the write request and fix failling]

As I said in an email to Jan Beulich, this is wrong. If you drain after
the request is submitted, you have no control over the ordering of the data in the BARRIER request and previous requests. And that is wrong. So drain was originally at the right place. Only we should probably set the operation to WRITE_FLUSH_FUA (not just WRITE_FLUSH) so we flush the disk caches also after the request completes.

Honza

Quoted 138 lines

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

block/elevator.c +1 -0

index a3b64bc..4e6c5b6 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL_GPL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled

drivers/block/xen-blkback/blkback.c +18 -2

index 9713d5a..738966e 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		   (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
 			 " error=%d\n", error);

@@ -601,6 +606,9 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		if (!blkif->vbd.barrier_support)
+			goto fail_response;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;

@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;

@@ -667,7 +674,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 			goto fail_response;
 		}
 	}
-
 	/*
 	 * If we have failed at this point, we need to undo the M2P override,
 	 * set gnttab_set_unmap_op on all of the grant references and perform

@@ -746,6 +752,16 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	else if (operation == WRITE || operation == WRITE_FLUSH)
 		blkif->st_wr_sect += preq.nr_sects;
 
+	if (req->operation == BLKIF_OP_WRITE_BARRIER) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		/* Emulate the original behavior of write barriers */
+		spin_lock_irqsave(q->queue_lock, flags);
+		elv_drain_elevator(q);
+		__blk_run_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
 	return 0;
 
  fail_flush:

drivers/block/xen-blkback/common.h +3 -0

index bfb532e..9f1990b 100644
@@ -157,6 +157,7 @@ struct xen_vbd {
 	/* Cached size parameter. */
 	sector_t		size;
 	bool			flush_support;
+	bool			barrier_support;
 };
 
 struct backend_info;

@@ -228,6 +229,8 @@ int xen_blkif_schedule(void *arg);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);

drivers/block/xen-blkback/xenbus.c +19 -0

index 2b3aef0..ec0975b 100644
@@ -376,6 +376,9 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 	if (q && q->flush_flags)
 		vbd->flush_support = true;
 
+	if (q && q->elevator)
+		vbd->barrier_support = true;
+
 	DPRINTK("Successful creation of handle=%04x (dom=%u)\n",
 		handle, blkif->domid);
 	return 0;

@@ -421,6 +424,20 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 	return err;
 }
 
+int xen_blkbk_barrier(struct xenbus_transaction xbt,
+		      struct backend_info *be, int state)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-barrier");
+
+	return err;
+}
+
 int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;

@@ -706,6 +723,8 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.barrier_support);
+
 	err = xen_blkbk_discard(xbt, be);
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Christoph Hellwig

2011-09-15 15:16 UTC #21 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Btw, I'm really against exporting any of the low-level request/elevator code interfaces. I have plans for huge changes in that area, and having someone poke into it is just a bad idea.

Try to take a look at the barrier era dm implementation of them, which did the draining on the bio layer and copy & paste that.

Konrad Rzeszutek Wilk

author

2011-09-16 09:24 UTC #22 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

Quoted 3 lines · Christoph Hellwig

Btw, I'm really against exporting any of the low-level request/elevator code interfaces. I have plans for huge changes in that area, and having someone poke into it is just a bad idea.

Thanks for the heads up.

Quoted 3 lines

Try to take a look at the barrier era dm implementation of them, which did the draining on the bio layer and copy & paste that.

Jan (K), do you want to poke at that and re-use the patch of yours which I have been altering?

Jan Kara

2011-09-15 13:24 UTC #23 lore raw

Re: [PATCH]: xen/blkback: Add support for old BARRIER requests - 'feature-barrier', was "Help with implementing some form of barriers in 3.0 kernels."

In reply to Jan Beulich

Quoted 99 lines · Jan Beulich

Quoted 84 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
On Wed, Sep 14, 2011 at 10:32:47AM -0400, Konrad Rzeszutek Wilk wrote:
Let me just post the one that I've succesfully tested. Not sure about the need for the check for q->elvator in the dispatch_rw_block_io so if you guys want me to remove it I can certainly do it.
commit a5a12f729697161522ecae0564915cf4145cb65c
Author: Jan Kara <[email protected]>
Date:   Tue Sep 13 11:44:07 2011 +0100
xen/blkback: Add support for old BARRIER requests - 'feature-barrier'
Recent kernels do not support BARRIER operation but only FLUSH
operation. But
older xenblk frontends still use the BARRIER operation to achieve data integrity requirements. So add support for BARRIER operation into xenblk backend so that all guests do not corrupt their filesystem on host
crash.
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Jan Beulich <[email protected]>
[v1: Added some extra functions, and other cases]
[v2: Added check for q->elevator]
[v3: Changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL]
Signed-off-by Konrad Rzeszutek Wilk <[email protected]>
block/elevator.c +1 -0
index a3b64bc..4e6c5b6 100644
@@ -616,6 +616,7 @@ void elv_drain_elevator(struct request_queue *q)
 		       q->elevator->elevator_type->elevator_name, q->nr_sorted);
 	}
 }
+EXPORT_SYMBOL_GPL(elv_drain_elevator);
 
 /*
  * Call with queue lock held, interrupts disabled
drivers/block/xen-blkback/blkback.c +5 -0
index 9713d5a..573744e 100644
@@ -464,6 +464,11 @@ static void __end_block_io_op(struct pending_req 
 *pending_req, int error)
 		pr_debug(DRV_PFX "flush diskcache op failed, not supported\n");
 		xen_blkbk_flush_diskcache(XBT_NIL, pending_req->blkif->be, 0);
 		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
+	} else if ((pending_req->operation == BLKIF_OP_WRITE_BARRIER) &&
+		   (error == -EOPNOTSUPP)) {
+		pr_debug(DRV_PFX "write barrier op failed, not supported\n");
+		xen_blkbk_barrier(XBT_NIL, pending_req->blkif->be, 0);
+		pending_req->status = BLKIF_RSP_EOPNOTSUPP;
 	} else if (error) {
 		pr_debug(DRV_PFX "Buffer not up-to-date at end of operation,"
drivers/block/xen-blkback/blkback.c +3 -1
 			 " error=%d\n", error);
@@ -590,6 +595,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	struct bio *biolist[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	int i, nbio = 0;
 	int operation;
+	bool drain = false;
 	struct blk_plug plug;
 
 	switch (req->operation) {
@@ -601,6 +607,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_wr_req++;
 		operation = WRITE_ODIRECT;
 		break;
+	case BLKIF_OP_WRITE_BARRIER:
+		drain = true;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
 		operation = WRITE_FLUSH;
@@ -609,7 +617,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		blkif->st_ds_req++;
 		operation = REQ_DISCARD;
 		break;
-	case BLKIF_OP_WRITE_BARRIER:
 	default:
 		operation = 0; /* make gcc happy */
 		goto fail_response;
@@ -668,6 +675,21 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		}
drivers/block/xen-blkback/blkback.c static int dispatch_rw_block_io(struct xen_blkif *blkif, · line 685
Quoted 10 lines
drivers/block/xen-blkback/blkback.c +8 -0
 	}
 
+	if (drain) {
+		struct request_queue *q = bdev_get_queue(preq.bdev);
+		unsigned long flags;
+
+		if (!q->elevator) {
+			__end_block_io_op(pending_req, -EOPNOTSUPP);
+			return -EOPNOTSUPP;
+		}
You shouldn't return here - barrier requests may be "non-empty" (i.e. carry data to be written), and hence you'd need to make sure the writes get carried out nevertheless.

Quoted 3 lines

Jan - this suggests that the placement isn't correct either: The draining of the queue - as I understand it - should be happening *after* these writes completed, not before they get started.

The draining should really happen before. It was like that in older
kernels as well. But you are right that we should probably make the operation be WRITE_FLUSH_FUA to properly simulate the old behavior.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Mike Snitzer

2011-09-14 15:35 UTC #24 lore raw

In reply to Konrad Rzeszutek Wilk

Quoted 65 lines · Konrad Rzeszutek Wilk

Quoted 6 lines

Quoted 5 lines
+ if (drain) {
+ struct request_queue *q = bdev_get_queue(preq.bdev);
+ unsigned long flags;
+

Quoted 6 lines

Quoted 6 lines
+ /* Emulate the original behavior of write barriers */
+ spin_lock_irqsave(q->queue_lock, flags);
+ elv_drain_elevator(q);
+ __blk_run_queue(q);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+ }

I also had to add:

drivers/block/xen-blkback/blkback.c +4 -0

index eaf49d1..20fddbc 100644
@@ -679,6 +679,10 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
                struct request_queue *q = bdev_get_queue(preq.bdev);
                unsigned long flags;
 
+               if (!q->elevator) {
+                       __end_block_io_op(pending_req, -EOPNOTSUPP);
+                       return -EOPNOTSUPP;
+               }
                /* Emulate the original behavior of write barriers */
                spin_lock_irqsave(q->queue_lock, flags);
                elv_drain_elevator(q);

drivers/block/xen-blkback/common.h +1 -0

index 52d8893..722714a 100644
@@ -157,6 +157,7 @@ struct xen_vbd {
        /* Cached size parameter. */
        sector_t                size;
        bool                    flush_support;
+       bool                    barrier_support;
  };
 
  struct backend_info;

drivers/block/xen-blkback/xenbus.c +4 -1

index da1e27a..7189ecd 100644
@@ -384,6 +384,9 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
        if (q && q->flush_flags)
                vbd->flush_support = true;
 
+       if (q && q->elevator && q->elevator->ops)
+               vbd->barrier_support = true;
+
        DPRINTK("Successful creation of handle=%04x (dom=%u)\n",
                handle, blkif->domid);
        return 0;

@@ -728,7 +731,7 @@ again:
        if (err)
                goto abort;
 
-       err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
+       err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.barrier_support);
 
        err = xen_blkbk_discard(xbt, be);

Quoted 2 lines

Otherwise it would crash.. Thought I am not sure why the elevator is not set (the guest is on a LVM LV).

bio-based DM devices do not have an elevator. But that doesn't mean barriers (or FLUSH/FUA) aren't supported and/or needed by the underlying devices.