[PATCH v2 0/2] scsi-disk: Add FUA write support

[PATCH v2 0/2] scsi-disk: Add FUA write support

[Alberto Faria]

Add scsi-disk support for Force Unit Access (FUA) writes. The first patch lets
us avoid FUA emulation when the underlying driver supports it natively. The
second patch makes scsi-disk devices advertise FUA support by default.

v2:
- Drop FUA write emulation logic since the block layer already does that.
- Add machine type compat for "dpofua".

Alberto Faria (2):
  scsi-disk: Add native FUA write support
  scsi-disk: Advertise FUA support by default

 hw/core/machine.c   |  1 +
 hw/scsi/scsi-disk.c | 45 +++++++++++----------------------------------
 2 files changed, 12 insertions(+), 34 deletions(-)

-- 
2.49.0

[PATCH v2 1/2] scsi-disk: Add native FUA write support

[Alberto Faria]

Simply propagate the FUA flag on write requests to the driver. The block
layer will emulate it if necessary.

Signed-off-by: Alberto Faria <afaria@redhat.com>
---
 hw/scsi/scsi-disk.c | 43 ++++++++++---------------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e59632e9b1..f62dcded64 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -74,7 +74,7 @@ struct SCSIDiskClass {
      */
     DMAIOFunc       *dma_readv;
     DMAIOFunc       *dma_writev;
-    bool            (*need_fua_emulation)(SCSICommand *cmd);
+    bool            (*need_fua)(SCSICommand *cmd);
     void            (*update_sense)(SCSIRequest *r);
 };
 
@@ -85,7 +85,7 @@ typedef struct SCSIDiskReq {
     uint32_t sector_count;
     uint32_t buflen;
     bool started;
-    bool need_fua_emulation;
+    bool need_fua;
     struct iovec iov;
     QEMUIOVector qiov;
     BlockAcctCookie acct;
@@ -389,24 +389,6 @@ static bool scsi_is_cmd_fua(SCSICommand *cmd)
     }
 }
 
-static void scsi_write_do_fua(SCSIDiskReq *r)
-{
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-
-    assert(r->req.aiocb == NULL);
-    assert(!r->req.io_canceled);
-
-    if (r->need_fua_emulation) {
-        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0,
-                         BLOCK_ACCT_FLUSH);
-        r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r);
-        return;
-    }
-
-    scsi_req_complete(&r->req, GOOD);
-    scsi_req_unref(&r->req);
-}
-
 static void scsi_dma_complete_noio(SCSIDiskReq *r, int ret)
 {
     assert(r->req.aiocb == NULL);
@@ -416,12 +398,7 @@ static void scsi_dma_complete_noio(SCSIDiskReq *r, int ret)
 
     r->sector += r->sector_count;
     r->sector_count = 0;
-    if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
-        scsi_write_do_fua(r);
-        return;
-    } else {
-        scsi_req_complete(&r->req, GOOD);
-    }
+    scsi_req_complete(&r->req, GOOD);
 
 done:
     scsi_req_unref(&r->req);
@@ -564,7 +541,7 @@ static void scsi_read_data(SCSIRequest *req)
 
     first = !r->started;
     r->started = true;
-    if (first && r->need_fua_emulation) {
+    if (first && r->need_fua) {
         block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0,
                          BLOCK_ACCT_FLUSH);
         r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_do_read_cb, r);
@@ -589,8 +566,7 @@ static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
     r->sector += n;
     r->sector_count -= n;
     if (r->sector_count == 0) {
-        scsi_write_do_fua(r);
-        return;
+        scsi_req_complete(&r->req, GOOD);
     } else {
         scsi_init_iovec(r, SCSI_DMA_BUF_SIZE);
         trace_scsi_disk_write_complete_noio(r->req.tag, r->qiov.size);
@@ -2391,7 +2367,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
         return 0;
     }
-    r->need_fua_emulation = sdc->need_fua_emulation(&r->req.cmd);
+    r->need_fua = sdc->need_fua(&r->req.cmd);
     if (r->sector_count == 0) {
         scsi_req_complete(&r->req, GOOD);
     }
@@ -3137,7 +3113,8 @@ BlockAIOCB *scsi_dma_writev(int64_t offset, QEMUIOVector *iov,
 {
     SCSIDiskReq *r = opaque;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-    return blk_aio_pwritev(s->qdev.conf.blk, offset, iov, 0, cb, cb_opaque);
+    int flags = r->need_fua ? BDRV_REQ_FUA : 0;
+    return blk_aio_pwritev(s->qdev.conf.blk, offset, iov, flags, cb, cb_opaque);
 }
 
 static char *scsi_property_get_loadparm(Object *obj, Error **errp)
@@ -3186,7 +3163,7 @@ static void scsi_disk_base_class_initfn(ObjectClass *klass, void *data)
     device_class_set_legacy_reset(dc, scsi_disk_reset);
     sdc->dma_readv = scsi_dma_readv;
     sdc->dma_writev = scsi_dma_writev;
-    sdc->need_fua_emulation = scsi_is_cmd_fua;
+    sdc->need_fua  = scsi_is_cmd_fua;
 }
 
 static const TypeInfo scsi_disk_base_info = {
@@ -3338,7 +3315,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     sdc->dma_readv   = scsi_block_dma_readv;
     sdc->dma_writev  = scsi_block_dma_writev;
     sdc->update_sense = scsi_block_update_sense;
-    sdc->need_fua_emulation = scsi_block_no_fua;
+    sdc->need_fua    = scsi_block_no_fua;
     dc->desc = "SCSI block device passthrough";
     device_class_set_props(dc, scsi_block_properties);
     dc->vmsd  = &vmstate_scsi_disk_state;
-- 
2.49.0

Re: [PATCH v2 1/2] scsi-disk: Add native FUA write support

[Kevin Wolf]

Am 11.04.2025 um 13:30 hat Alberto Faria geschrieben:
> Simply propagate the FUA flag on write requests to the driver. The block
> layer will emulate it if necessary.
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 43 ++++++++++---------------------------------
>  1 file changed, 10 insertions(+), 33 deletions(-)

> @@ -416,12 +398,7 @@ static void scsi_dma_complete_noio(SCSIDiskReq *r, int ret)
>  
>      r->sector += r->sector_count;
>      r->sector_count = 0;
> -    if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
> -        scsi_write_do_fua(r);
> -        return;
> -    } else {
> -        scsi_req_complete(&r->req, GOOD);
> -    }
> +    scsi_req_complete(&r->req, GOOD);
>  
>  done:
>      scsi_req_unref(&r->req);

This (and the same change in scsi_write_complete_noio()) breaks the
handling of VERIFY in scsi_write_data().

I think what VERIFY needs to do after this change is calling
blk_aio_flush() directly, similar to what scsi_read_data() does in the
first && r->needs_fua case.

The READ and WRITE commands look good to me with this change.

Kevin

[PATCH v2 2/2] scsi-disk: Advertise FUA support by default

[Alberto Faria]

Allow the guest to submit FUA requests directly, instead of forcing it
to emulate them using a regular flush.

Signed-off-by: Alberto Faria <afaria@redhat.com>
---
 hw/core/machine.c   | 1 +
 hw/scsi/scsi-disk.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 63c6ef93d2..e4e6474a4e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -46,6 +46,7 @@ GlobalProperty hw_compat_9_2[] = {
     { "migration", "multifd-clean-tls-termination", "false" },
     { "migration", "send-switchover-start", "off"},
     { "vfio-pci", "x-migration-multifd-transfer", "off" },
+    { "scsi-disk", "dpofua", "off" },
 };
 const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
 
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index f62dcded64..2f62f6069d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -3192,7 +3192,7 @@ static const Property scsi_hd_properties[] = {
     DEFINE_PROP_BIT("removable", SCSIDiskState, features,
                     SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
-                    SCSI_DISK_F_DPOFUA, false),
+                    SCSI_DISK_F_DPOFUA, true),
     DEFINE_PROP_UINT64("wwn", SCSIDiskState, qdev.wwn, 0),
     DEFINE_PROP_UINT64("port_wwn", SCSIDiskState, qdev.port_wwn, 0),
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
-- 
2.49.0

Re: [PATCH v2 2/2] scsi-disk: Advertise FUA support by default

[Kevin Wolf]

Am 11.04.2025 um 13:30 hat Alberto Faria geschrieben:
> Allow the guest to submit FUA requests directly, instead of forcing it
> to emulate them using a regular flush.
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> ---
>  hw/core/machine.c   | 1 +
>  hw/scsi/scsi-disk.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 63c6ef93d2..e4e6474a4e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -46,6 +46,7 @@ GlobalProperty hw_compat_9_2[] = {
>      { "migration", "multifd-clean-tls-termination", "false" },
>      { "migration", "send-switchover-start", "off"},
>      { "vfio-pci", "x-migration-multifd-transfer", "off" },
> +    { "scsi-disk", "dpofua", "off" },
>  };
>  const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);

This needs to go to hw_compat_10_0 now.

And shouldn't it be "scsi-hd" rather than "scsi-disk"? Did you test that
the property is disabled when you use an older machine type?

Kevin

Re: [PATCH v2 2/2] scsi-disk: Advertise FUA support by default

[Alberto Faria]

On Fri, Apr 25, 2025 at 4:05 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 11.04.2025 um 13:30 hat Alberto Faria geschrieben:
> > Allow the guest to submit FUA requests directly, instead of forcing it
> > to emulate them using a regular flush.
> >
> > Signed-off-by: Alberto Faria <afaria@redhat.com>
> > ---
> >  hw/core/machine.c   | 1 +
> >  hw/scsi/scsi-disk.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 63c6ef93d2..e4e6474a4e 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -46,6 +46,7 @@ GlobalProperty hw_compat_9_2[] = {
> >      { "migration", "multifd-clean-tls-termination", "false" },
> >      { "migration", "send-switchover-start", "off"},
> >      { "vfio-pci", "x-migration-multifd-transfer", "off" },
> > +    { "scsi-disk", "dpofua", "off" },
> >  };
> >  const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
>
> This needs to go to hw_compat_10_0 now.
>
> And shouldn't it be "scsi-hd" rather than "scsi-disk"? Did you test that
> the property is disabled when you use an older machine type?

Sorry about that, fixed in v3.

> Kevin
>