On Thu, Jun 13, 2024 at 04:05:08PM +0200, Christoph Hellwig wrote: > On Wed, Jun 12, 2024 at 05:56:15PM +0200, Roger Pau Monné wrote: > > Right. AFAICT advertising "feature-barrier" and/or > > "feature-flush-cache" could be done based on whether blkback > > understand those commands, not on whether the underlying storage > > supports the equivalent of them. > > > > Worst case we can print a warning message once about the underlying > > storage failing to complete flush/barrier requests, and that data > > integrity might not be guaranteed going forward, and not propagate the > > error to the upper layer? > > > > What would be the consequence of propagating a flush error to the > > upper layers? > > If you propage the error to the upper layer you will generate an > I/O error there, which usually leads to a file system shutdown. > > > Given the description of the feature in the blkif header, I'm afraid > > we cannot guarantee that seeing the feature exposed implies barrier or > > flush support, since the request could fail at any time (or even from > > the start of the disk attachment) and it would still sadly be a correct > > implementation given the description of the options. > > Well, then we could do something like the patch below, which keeps > the existing behavior, but insolates the block layer from it and > removes the only user of blk_queue_write_cache from interrupt > context: LGTM, I'm not sure there's much else we can do. > --- > From e6e82c769ab209a77302994c3829cf6ff7a595b8 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@xxxxxx> > Date: Thu, 30 May 2024 08:58:52 +0200 > Subject: xen-blkfront: don't disable cache flushes when they fail > > blkfront always had a robust negotiation protocol for detecting a write > cache. Stop simply disabling cache flushes in the block layer as the > flags handling is moving to the atomic queue limits API that needs > user context to freeze the queue for that. Instead handle the case > of the feature flags cleared inside of blkfront. This removes old > debug code to check for such a mismatch which was previously impossible > to hit, including the check for passthrough requests that blkfront > never used to start with. > > Signed-off-by: Christoph Hellwig <hch@xxxxxx> > --- > drivers/block/xen-blkfront.c | 44 +++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 9b4ec3e4908cce..e2c92d5095ff17 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -788,6 +788,14 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > * A barrier request a superset of FUA, so we can > * implement it the same way. (It's also a FLUSH+FUA, > * since it is guaranteed ordered WRT previous writes.) > + * > + * Note that can end up here with a FUA write and the > + * flags cleared. This happens when the flag was > + * run-time disabled and raced with I/O submission in > + * the block layer. We submit it as a normal write Since blkfront no longer signals that FUA is no longer available for the device, getting a request with FUA is not actually a race I think? > + * here. A pure flush should never end up here with > + * the flags cleared as they are completed earlier for > + * the !feature_flush case. > */ > if (info->feature_flush && info->feature_fua) > ring_req->operation = > @@ -795,8 +803,6 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > else if (info->feature_flush) > ring_req->operation = > BLKIF_OP_FLUSH_DISKCACHE; > - else > - ring_req->operation = 0; > } > ring_req->u.rw.nr_segments = num_grant; > if (unlikely(require_extra_req)) { > @@ -887,16 +893,6 @@ static inline void flush_requests(struct blkfront_ring_info *rinfo) > notify_remote_via_irq(rinfo->irq); > } > > -static inline bool blkif_request_flush_invalid(struct request *req, > - struct blkfront_info *info) > -{ > - return (blk_rq_is_passthrough(req) || > - ((req_op(req) == REQ_OP_FLUSH) && > - !info->feature_flush) || > - ((req->cmd_flags & REQ_FUA) && > - !info->feature_fua)); > -} > - > static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > const struct blk_mq_queue_data *qd) > { > @@ -908,23 +904,30 @@ static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > rinfo = get_rinfo(info, qid); > blk_mq_start_request(qd->rq); > spin_lock_irqsave(&rinfo->ring_lock, flags); > - if (RING_FULL(&rinfo->ring)) > - goto out_busy; > > - if (blkif_request_flush_invalid(qd->rq, rinfo->dev_info)) > - goto out_err; > + /* > + * Check if the backend actually supports flushes. > + * > + * While the block layer won't send us flushes if we don't claim to > + * support them, the Xen protocol allows the backend to revoke support > + * at any time. That is of course a really bad idea and dangerous, but > + * has been allowed for 10+ years. In that case we simply clear the > + * flags, and directly return here for an empty flush and ignore the > + * FUA flag later on. > + */ > + if (unlikely(req_op(qd->rq) == REQ_OP_FLUSH && !info->feature_flush)) > + goto out; Don't you need to complete the request here? Thanks, Roger.