* [PATCH] xen-blk[front|back] FUA additions. @ 2011-09-09 18:30 Konrad Rzeszutek Wilk 2011-09-09 18:30 ` [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH Konrad Rzeszutek Wilk 2011-09-09 18:31 ` [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation Konrad Rzeszutek Wilk 0 siblings, 2 replies; 8+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-09-09 18:30 UTC (permalink / raw) To: linux-kernel; +Cc: axboe, xen-devel, jeremy.fitzhardinge, JBeulich 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH. 2011-09-09 18:30 [PATCH] xen-blk[front|back] FUA additions Konrad Rzeszutek Wilk @ 2011-09-09 18:30 ` Konrad Rzeszutek Wilk 2011-09-10 18:08 ` Christoph Hellwig 2011-09-09 18:31 ` [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation Konrad Rzeszutek Wilk 1 sibling, 1 reply; 8+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-09-09 18:30 UTC (permalink / raw) To: linux-kernel Cc: xen-devel, JBeulich, jeremy.fitzhardinge, axboe, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH. 2011-09-09 18:30 ` [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH Konrad Rzeszutek Wilk @ 2011-09-10 18:08 ` Christoph Hellwig 2011-09-11 0:33 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2011-09-10 18:08 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: linux-kernel, xen-devel, JBeulich, jeremy.fitzhardinge, axboe 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH. 2011-09-10 18:08 ` Christoph Hellwig @ 2011-09-11 0:33 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 8+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-09-11 0:33 UTC (permalink / raw) To: Christoph Hellwig Cc: axboe, xen-devel, linux-kernel, JBeulich, jeremy.fitzhardinge 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! ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation. 2011-09-09 18:30 [PATCH] xen-blk[front|back] FUA additions Konrad Rzeszutek Wilk 2011-09-09 18:30 ` [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH Konrad Rzeszutek Wilk @ 2011-09-09 18:31 ` Konrad Rzeszutek Wilk 2011-09-12 8:01 ` Jan Beulich 1 sibling, 1 reply; 8+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-09-09 18:31 UTC (permalink / raw) To: linux-kernel Cc: xen-devel, JBeulich, jeremy.fitzhardinge, axboe, Konrad Rzeszutek Wilk, stable 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation. 2011-09-09 18:31 ` [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation Konrad Rzeszutek Wilk @ 2011-09-12 8:01 ` Jan Beulich 2011-09-16 19:15 ` [Xen-devel] " Konrad Rzeszutek Wilk 0 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2011-09-12 8:01 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: jeremy.fitzhardinge, axboe, stable, xen-devel, linux-kernel >>> 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] Re: [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation. 2011-09-12 8:01 ` Jan Beulich @ 2011-09-16 19:15 ` Konrad Rzeszutek Wilk 2011-09-19 9:35 ` Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-09-16 19:15 UTC (permalink / raw) To: Jan Beulich; +Cc: axboe, xen-devel, jeremy.fitzhardinge, stable, linux-kernel 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; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Re: [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation. 2011-09-16 19:15 ` [Xen-devel] " Konrad Rzeszutek Wilk @ 2011-09-19 9:35 ` Jan Beulich 0 siblings, 0 replies; 8+ messages in thread From: Jan Beulich @ 2011-09-19 9:35 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: axboe, xen-devel, jeremy.fitzhardinge, stable, linux-kernel >>> 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; > } ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-09-19 9:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-09 18:30 [PATCH] xen-blk[front|back] FUA additions Konrad Rzeszutek Wilk 2011-09-09 18:30 ` [PATCH 1/3] xen/blk[front|back]: Use the full FLUSH | FUA instead of just FLUSH Konrad Rzeszutek Wilk 2011-09-10 18:08 ` Christoph Hellwig 2011-09-11 0:33 ` Konrad Rzeszutek Wilk 2011-09-09 18:31 ` [PATCH 3/3] xen-blkfront: If no barrier or flush is supported, use invalid operation Konrad Rzeszutek Wilk 2011-09-12 8:01 ` Jan Beulich 2011-09-16 19:15 ` [Xen-devel] " Konrad Rzeszutek Wilk 2011-09-19 9:35 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).