[PATCH] xen-blk[front|back] FUA additions.

[PATCH] xen-blk[front|back] FUA additions.

[Konrad Rzeszutek Wilk]

I am proposing these two patches for 3.2. They allow the backend
to process the REQ_FUA request as well. Previous to these patches
it only did REQ_FLUSH. There is also a bug-fix for the logic
of how barrier/flushes were handled.

The patches are based on a branch which also has 'feature-discard'
patches, so they won't apply nativly on top of 3.1-rc5.

Please review and if there are no concerns I will send a proper
git pull to Jens to pick up the whole branch for 3.2.

 drivers/block/xen-blkback/blkback.c |   11 ++++++-----
 drivers/block/xen-blkfront.c        |   12 ++++++------
 drivers/block/xen-blkfront.c          |    8 ++++----
 3 files changed, 16 insertions(+), 15 deletions(-)

The full git tree is currently at my backup git tree:

git://oss.oracle.com/git/kwilk/xen.git stable/for-jens-3.2

[PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH.

[Konrad Rzeszutek Wilk]

During a FLUSH we can pass sector number that we want to
have flushed - which is what FUA requests are.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c |   11 ++++++-----
 drivers/block/xen-blkfront.c        |   12 ++++++------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 9713d5a..6ade8ab 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -603,7 +603,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		break;
 	case BLKIF_OP_FLUSH_DISKCACHE:
 		blkif->st_f_req++;
-		operation = WRITE_FLUSH;
+		operation = WRITE_FLUSH_FUA;
 		break;
 	case BLKIF_OP_DISCARD:
 		blkif->st_ds_req++;
@@ -618,7 +618,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 
 	/* Check that the number of segments is sane. */
 	nseg = req->nr_segments;
-	if (unlikely(nseg == 0 && operation != WRITE_FLUSH &&
+	if (unlikely(nseg == 0 && operation != WRITE_FLUSH_FUA &&
 				operation != REQ_DISCARD) ||
 	    unlikely(nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) {
 		pr_debug(DRV_PFX "Bad number of segments in request (%d)\n",
@@ -707,9 +707,10 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 
 	/* This will be hit if the operation was a flush or discard. */
 	if (!bio) {
-		BUG_ON(operation != WRITE_FLUSH && operation != REQ_DISCARD);
+		BUG_ON(operation != WRITE_FLUSH_FUA &&
+		       operation != REQ_DISCARD);
 
-		if (operation == WRITE_FLUSH) {
+		if (operation == WRITE_FLUSH_FUA) {
 			bio = bio_alloc(GFP_KERNEL, 0);
 			if (unlikely(bio == NULL))
 				goto fail_put_bio;
@@ -743,7 +744,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 
 	if (operation == READ)
 		blkif->st_rd_sect += preq.nr_sects;
-	else if (operation == WRITE || operation == WRITE_FLUSH)
+	else if (operation == WRITE || operation == WRITE_FLUSH_FUA)
 		blkif->st_wr_sect += preq.nr_sects;
 
 	return 0;
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 86e2c63..e205d91 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1220,10 +1220,9 @@ static void blkfront_connect(struct blkfront_info *info)
 	 *
 	 * If there are barriers, then we use flush.
 	 */
-	if (!err && barrier) {
-		info->feature_flush = REQ_FLUSH | REQ_FUA;
+	if (!err && barrier)
 		info->flush_op = BLKIF_OP_WRITE_BARRIER;
-	}
+
 	/*
 	 * And if there is "feature-flush-cache" use that above
 	 * barriers.
@@ -1232,10 +1231,11 @@ static void blkfront_connect(struct blkfront_info *info)
 			    "feature-flush-cache", "%d", &flush,
 			    NULL);
 
-	if (!err && flush) {
-		info->feature_flush = REQ_FLUSH;
+	if (!err && flush)
 		info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
-	}
+
+	if (info->flush_op)
+		info->feature_flush = REQ_FLUSH | REQ_FUA;
 
 	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
 			    "feature-discard", "%d", &discard,
-- 
1.7.4.1

[PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation.

[Konrad Rzeszutek Wilk]

By default we would set the info->flush_op=0, and zero maps
to BLKIF_OP_READ request code. So if the backend did not support
either barrier ('feature-barrier') or flush ('feature-flush-cache')
we would end up using that opcode for the flush/barrier request, meaning
we actually do a READ request. Fortunatly for us, __generic_make_request
checks q->flush_flags and realizes that we don't do FLUSH and removes
the REQ_FLUSH | REQ_FUA so we never end up issuing a READ request
for a flush request. However, other third party implementations of
__make_request might not be so smart, so lets fix this up.

CC: stable@kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index e205d91..c4aa9da 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -97,7 +97,7 @@ struct blkfront_info
 	struct blk_shadow shadow[BLK_RING_SIZE];
 	unsigned long shadow_free;
 	unsigned int feature_flush;
-	unsigned int flush_op;
+	int flush_op;
 	unsigned int feature_discard;
 	unsigned int discard_granularity;
 	unsigned int discard_alignment;
@@ -774,7 +774,7 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 				if (error == -EOPNOTSUPP)
 					error = 0;
 				info->feature_flush = 0;
-				info->flush_op = 0;
+				info->flush_op = -1; /* 0 is BLKIF_OP_READ */
 				xlvbd_flush(info);
 			}
 			/* fall through */
@@ -1207,7 +1207,7 @@ static void blkfront_connect(struct blkfront_info *info)
 	}
 
 	info->feature_flush = 0;
-	info->flush_op = 0;
+	info->flush_op = -1; /* 0 is BLKIF_OP_READ */
 
 	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
 			    "feature-barrier", "%d", &barrier,
@@ -1234,7 +1234,7 @@ static void blkfront_connect(struct blkfront_info *info)
 	if (!err && flush)
 		info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
 
-	if (info->flush_op)
+	if (info->flush_op > 0)
 		info->feature_flush = REQ_FLUSH | REQ_FUA;
 
 	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-- 
1.7.4.1

Re: [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH.

[Christoph Hellwig]

On Fri, Sep 09, 2011 at 02:30:59PM -0400, Konrad Rzeszutek Wilk wrote:
> During a FLUSH we can pass sector number that we want to
> have flushed - which is what FUA requests are.

No, that is not the case.

REQ_FLUSH without data		-> pure flush
REQ_FLUSH with data		-> preflush plus write
REQ_FUA				-> write and ranged postflush

Re: [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH.

[Konrad Rzeszutek Wilk]

On Sat, Sep 10, 2011 at 02:08:49PM -0400, Christoph Hellwig wrote:
> On Fri, Sep 09, 2011 at 02:30:59PM -0400, Konrad Rzeszutek Wilk wrote:
> > During a FLUSH we can pass sector number that we want to
> > have flushed - which is what FUA requests are.
> 
> No, that is not the case.
> 
> REQ_FLUSH without data		-> pure flush
> REQ_FLUSH with data		-> preflush plus write

Excellent. So we have been doing it right all along.

> REQ_FUA				-> write and ranged postflush

Ah, somehow I was thinking that you can't write data with a
REQ_FLUSH, but that is nonsense as the block/blk-flush.c
explains in great details.

Will drop this patch - and thanks for clarifying it!

Re: [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation.

[Jan Beulich]

>>> On 09.09.11 at 20:31, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> By default we would set the info->flush_op=0, and zero maps
> to BLKIF_OP_READ request code. So if the backend did not support
> either barrier ('feature-barrier') or flush ('feature-flush-cache')
> we would end up using that opcode for the flush/barrier request, meaning
> we actually do a READ request. Fortunatly for us, __generic_make_request
> checks q->flush_flags and realizes that we don't do FLUSH and removes
> the REQ_FLUSH | REQ_FUA so we never end up issuing a READ request
> for a flush request. However, other third party implementations of
> __make_request might not be so smart, so lets fix this up.

Wouldn't it be better to simply have blkif_queue_request() fail in that
case (and then it doesn't matter whether flush_op is set to 0 or -1)?
Not the least because older (forward-port) backends stall the incoming
queue and are possibly verbose for invalid requests seen.

Jan

> CC: stable@kernel.org 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkfront.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index e205d91..c4aa9da 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -97,7 +97,7 @@ struct blkfront_info
>  	struct blk_shadow shadow[BLK_RING_SIZE];
>  	unsigned long shadow_free;
>  	unsigned int feature_flush;
> -	unsigned int flush_op;
> +	int flush_op;
>  	unsigned int feature_discard;
>  	unsigned int discard_granularity;
>  	unsigned int discard_alignment;
> @@ -774,7 +774,7 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  				if (error == -EOPNOTSUPP)
>  					error = 0;
>  				info->feature_flush = 0;
> -				info->flush_op = 0;
> +				info->flush_op = -1; /* 0 is BLKIF_OP_READ */
>  				xlvbd_flush(info);
>  			}
>  			/* fall through */
> @@ -1207,7 +1207,7 @@ static void blkfront_connect(struct blkfront_info 
> *info)
>  	}
>  
>  	info->feature_flush = 0;
> -	info->flush_op = 0;
> +	info->flush_op = -1; /* 0 is BLKIF_OP_READ */
>  
>  	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
>  			    "feature-barrier", "%d", &barrier,
> @@ -1234,7 +1234,7 @@ static void blkfront_connect(struct blkfront_info 
> *info)
>  	if (!err && flush)
>  		info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
>  
> -	if (info->flush_op)
> +	if (info->flush_op > 0)
>  		info->feature_flush = REQ_FLUSH | REQ_FUA;
>  
>  	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,

Re: [Xen-devel] Re: [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation.

[Konrad Rzeszutek Wilk]

On Mon, Sep 12, 2011 at 09:01:25AM +0100, Jan Beulich wrote:
> >>> On 09.09.11 at 20:31, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > By default we would set the info->flush_op=0, and zero maps
> > to BLKIF_OP_READ request code. So if the backend did not support
> > either barrier ('feature-barrier') or flush ('feature-flush-cache')
> > we would end up using that opcode for the flush/barrier request, meaning
> > we actually do a READ request. Fortunatly for us, __generic_make_request
> > checks q->flush_flags and realizes that we don't do FLUSH and removes
> > the REQ_FLUSH | REQ_FUA so we never end up issuing a READ request
> > for a flush request. However, other third party implementations of
> > __make_request might not be so smart, so lets fix this up.
> 
> Wouldn't it be better to simply have blkif_queue_request() fail in that
> case (and then it doesn't matter whether flush_op is set to 0 or -1)?
> Not the least because older (forward-port) backends stall the incoming
> queue and are possibly verbose for invalid requests seen.

So like this:


diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 468bfec..e9d301c 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -380,7 +380,9 @@ static void do_blkif_request(struct request_queue *rq)
 
 		blk_start_request(req);
 
-		if (req->cmd_type != REQ_TYPE_FS) {
+		if ((req->cmd_type != REQ_TYPE_FS) ||
+		    ((req->cmd_flags & (REQ_FLUSH | REQ_FUA)) &&
+		    !info->flush_op)) {
 			__blk_end_request_all(req, -EIO);
 			continue;
 		}

Re: Re: [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation.

[Jan Beulich]

>>> On 16.09.11 at 21:15, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Sep 12, 2011 at 09:01:25AM +0100, Jan Beulich wrote:
>> >>> On 09.09.11 at 20:31, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > By default we would set the info->flush_op=0, and zero maps
>> > to BLKIF_OP_READ request code. So if the backend did not support
>> > either barrier ('feature-barrier') or flush ('feature-flush-cache')
>> > we would end up using that opcode for the flush/barrier request, meaning
>> > we actually do a READ request. Fortunatly for us, __generic_make_request
>> > checks q->flush_flags and realizes that we don't do FLUSH and removes
>> > the REQ_FLUSH | REQ_FUA so we never end up issuing a READ request
>> > for a flush request. However, other third party implementations of
>> > __make_request might not be so smart, so lets fix this up.
>> 
>> Wouldn't it be better to simply have blkif_queue_request() fail in that
>> case (and then it doesn't matter whether flush_op is set to 0 or -1)?
>> Not the least because older (forward-port) backends stall the incoming
>> queue and are possibly verbose for invalid requests seen.
> 
> So like this:

Yes, this looks small and neat.

Acked-by: Jan Beulich <jbeulich@suse.com>

> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 468bfec..e9d301c 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -380,7 +380,9 @@ static void do_blkif_request(struct request_queue *rq)
>  
>  		blk_start_request(req);
>  
> -		if (req->cmd_type != REQ_TYPE_FS) {
> +		if ((req->cmd_type != REQ_TYPE_FS) ||
> +		    ((req->cmd_flags & (REQ_FLUSH | REQ_FUA)) &&
> +		    !info->flush_op)) {
>  			__blk_end_request_all(req, -EIO);
>  			continue;
>  		}