Re: [PATCH] xen/blkfront: Only check REQ_FUA for writes

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 02, 2023 at 04:40:17PM +0000, Ross Lagerwall wrote:
> > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > Sent: Tuesday, May 2, 2023 4:57 PM
> > To: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx <xen-devel@xxxxxxxxxxxxxxxxxxxx>; 
> > jgross@xxxxxxxx <jgross@xxxxxxxx>; sstabellini@xxxxxxxxxx 
> > <sstabellini@xxxxxxxxxx>; oleksandr_tyshchenko@xxxxxxxx 
> > <oleksandr_tyshchenko@xxxxxxxx>; axboe@xxxxxxxxx <axboe@xxxxxxxxx>
> > 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.

Right, can you add to the commit message that this was a bug in the
driver?  It wasn't clear to me whether that was the case or it was
legit for FUA to be used with requests != write.

> > 
> > Should this maybe be translated to a pair of flush cache and read
> > requests?
> > 
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > > ---
> > >  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.

Hm, yes, it would be better placed in some generic blk code rather
than (possibly) at every driver, otherwise people might complain that
xen-blkfront trows warnings for requests other drivers handle fine.

Would you be up for sending such a patch to generic blk layer?

For the code here:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

With the commit message slightly adjusted to make it clear read + fua
was a bug in the module?

Thanks, Roger.