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

[Kevin Wolf <kwolf@redhat.com>]

Am 04.03.2025 um 16:52 hat Alberto Faria geschrieben:
> Avoid emulating FUA when the driver supports it natively. This should
> provide better performance than a full flush after the write.
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>

Did you try out if you can see performance improvements in practice?
It's always nice to have numbers in the commit message for patches that
promise performance improvements.

>  hw/scsi/scsi-disk.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8cf50845ab..ce48e20ee6 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -43,6 +43,7 @@
>  #include "qemu/cutils.h"
>  #include "trace.h"
>  #include "qom/object.h"
> +#include "block/block_int-common.h"
>  
>  #ifdef __linux
>  #include <scsi/sg.h>
> @@ -75,7 +76,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);
>  };
>  
> @@ -86,6 +87,7 @@ typedef struct SCSIDiskReq {
>      uint32_t sector_count;
>      uint32_t buflen;
>      bool started;
> +    bool need_fua;
>      bool need_fua_emulation;
>      struct iovec iov;
>      QEMUIOVector qiov;
> @@ -553,7 +555,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);
> @@ -2384,7 +2386,9 @@ 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);
> +    r->need_fua_emulation = r->need_fua &&
> +        (blk_bs(s->qdev.conf.blk)->supported_write_flags & BDRV_REQ_FUA) == 0;

You can just use BDRV_REQ_FUA unconditionally. If the driver doesn't
support it directly, the block layer already emulates it internally. We
don't have to duplicate this here. If scsi_write_data() does a flush
directly for VERIFY (like scsi_read_data() already does),
scsi_write_do_fua() can go away completely.

However, we can only apply this to write requests. We still need to know
that FUA needs to be emulated for reads. scsi_read_data() issues a flush
for FUA requests and your patch would break it if writes support
BDRV_REQ_FUA.

Kevin