> From: Roger Pau Monne <roger....@citrix.com> > Sent: Tuesday, May 2, 2023 4:57 PM > To: Ross Lagerwall <ross.lagerw...@citrix.com> > Cc: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>; > jgr...@suse.com <jgr...@suse.com>; sstabell...@kernel.org > <sstabell...@kernel.org>; oleksandr_tyshche...@epam.com > <oleksandr_tyshche...@epam.com>; ax...@kernel.dk <ax...@kernel.dk> > Subject: Re: [PATCH] xen/blkfront: Only check REQ_FUA for writes > > On Wed, Apr 26, 2023 at 05:40:05PM +0100, Ross Lagerwall wrote: > > The existing code silently converts read operations with the > > REQ_FUA bit set into write-barrier operations. This results in data > > loss as the backend scribbles zeroes over the data instead of returning > > it. > > > > While the REQ_FUA bit doesn't make sense on a read operation, at least > > one well-known out-of-tree kernel module does set it and since it > > results in data loss, let's be safe here and only look at REQ_FUA for > > writes. > > Do we know what's the intention of the out-of-tree kernel module with > it's usage of FUA for reads?
It was just a plain bug that has now been fixed: https://github.com/veeam/blksnap/commit/e3b3e7369642b59e01c647934789e5e20b380c62 I think this patch is still worthwile since reads becoming writes is asking for data corruption. > > Should this maybe be translated to a pair of flush cache and read > requests? > > > Signed-off-by: Ross Lagerwall <ross.lagerw...@citrix.com> > > --- > > drivers/block/xen-blkfront.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > > index 23ed258b57f0..c1890c8a9f6e 100644 > > --- a/drivers/block/xen-blkfront.c > > +++ b/drivers/block/xen-blkfront.c > > @@ -780,7 +780,8 @@ static int blkif_queue_rw_req(struct request *req, > > struct blkfront_ring_info *ri > > ring_req->u.rw.handle = info->handle; > > ring_req->operation = rq_data_dir(req) ? > > BLKIF_OP_WRITE : BLKIF_OP_READ; > > - if (req_op(req) == REQ_OP_FLUSH || req->cmd_flags & REQ_FUA) { > > + if (req_op(req) == REQ_OP_FLUSH || > > + (req_op(req) == REQ_OP_WRITE && (req->cmd_flags & > > REQ_FUA))) { > > Should we print some kind of warning maybe once that we have received > a READ request with the FUA flag set, and the FUA flag will have no > effect? > I thought of adding something like this but I couldn't find any other block layer code doing a similar check (also it seems more appropriate in the core block layer). WARN_ONCE(req_op(req) != REQ_OP_WRITE && (req->cmd_flags & REQ_FUA)); I can add it if the maintainers want it. Thanks, Ross
