Re: [PATCH] Allow BLKIF_OP_WRITE_BARRIER or BLKIF_OP_FLUSH_DISKCACHE...

[Owen Smith <owen.smith@xxxxxxxxxx>]

Yes, "remove unsupported feature" should be the correct comment

Owen

On Wed, Nov 6, 2024 at 11:00 AM Paul Durrant <xadimgnik@xxxxxxxxx> wrote:

On 21/10/2024 12:47, Owen Smith wrote:
> ... when a SCSIOP_SYNCHRONIZE_CACHE is issued, which should be the last
> operation issued by the crash kernel.
> Certain backends do not advertise WriteBarrier support, and use SyncDiskCache
> instead. Add support for creating crashdumps on these backends.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
>   src/xencrsh/frontend.c | 13 ++++++++++++-
>   src/xencrsh/frontend.h |  1 +
>   src/xencrsh/pdo.c      | 27 ++++++++++++++++++++++++---
>   3 files changed, 37 insertions(+), 4 deletions(-)
>

Reviewed-by: Paul Durrant <paul@xxxxxxx>

... with one query...

> diff --git a/src/xencrsh/frontend.c b/src/xencrsh/frontend.c
> index d88f498..0b9e46b 100644
> --- a/src/xencrsh/frontend.c
> +++ b/src/xencrsh/frontend.c
> @@ -268,6 +268,7 @@ FrontendInsertRequestOnRing(
>           }
>           break;
>       case BLKIF_OP_WRITE_BARRIER:
> +    case BLKIF_OP_FLUSH_DISKCACHE:
>           RingReq->operation          = Request->Operation;
>           RingReq->nr_segments        = 0;
>           RingReq->handle             = (USHORT)Frontend->DeviceId;
> @@ -470,6 +471,15 @@ __ReadFeatures(
>           Frontend->FeatureBarrier = FALSE;
>       }
>   
> +    Status = StoreRead(NULL, Frontend->BackendPath,
> +                        "feature-flush-cache", &Buffer);
> +    if (NT_SUCCESS(Status)) {
> +        Frontend->FeatureFlush = (strtoul(Buffer, NULL, 10) == 1);
> +        AustereFree(Buffer);
> +    } else {
> +        Frontend->FeatureFlush = FALSE;
> +    }
> +
>       Status = StoreRead(NULL, Frontend->BackendPath,
>                           "feature-discard", &Buffer);
>       if (NT_SUCCESS(Status)) {
> @@ -479,9 +489,10 @@ __ReadFeatures(
>           Frontend->FeatureDiscard = FALSE;
>       }
>   
> -    LogVerbose("Features: DomId=%d, RingOrder=0, %s %s\n",
> +    LogVerbose("Features: DomId=%d, RingOrder=0, %s %s %s\n",
>                   Frontend->BackendId,
>                   Frontend->FeatureBarrier ? "BARRIER" : "NOT_BARRIER",
> +                Frontend->FeatureFlush ? "FLUSH" : "NOT_FLUSH",
>                   Frontend->FeatureDiscard ? "DISCARD" : "NOT_DISCARD");
>   }
>   static VOID
> diff --git a/src/xencrsh/frontend.h b/src/xencrsh/frontend.h
> index 174e4d7..2e2c305 100644
> --- a/src/xencrsh/frontend.h
> +++ b/src/xencrsh/frontend.h
> @@ -62,6 +62,7 @@ typedef struct _XENVBD_FRONTEND {
>       // Capabilities
>       BOOLEAN                     Connected;
>       BOOLEAN                     FeatureBarrier;
> +    BOOLEAN                     FeatureFlush;
>       BOOLEAN                     FeatureDiscard;
>       BOOLEAN                     Paging;
>       BOOLEAN                     Hibernation;
> diff --git a/src/xencrsh/pdo.c b/src/xencrsh/pdo.c
> index 9fe3c53..76c4d59 100644
> --- a/src/xencrsh/pdo.c
> +++ b/src/xencrsh/pdo.c
> @@ -564,17 +564,31 @@ PrepareSyncCache(
>   {
>       PXENVBD_SRBEXT          SrbExt = GetSrbExt(Srb);
>       PXENVBD_REQUEST         Request;
> -
> +    UCHAR                   Operation;
> +
> +    if (Pdo->Frontend.FeatureBarrier) {
> +        Operation = BLKIF_OP_WRITE_BARRIER;
> +    } else if (Pdo->Frontend.FeatureFlush) {
> +        Operation = BLKIF_OP_FLUSH_DISKCACHE;
> +    } else {
> +        // cannot submit this to backend, just complete the SRB
> +        Srb->SrbStatus = SRB_STATUS_SUCCESS;
> +        Srb->ScsiStatus = 0x00; // SCSI_GOOD
> +
> +        FdoCompleteSrb(Pdo->Fdo, Srb);
> +        return;
> +    }
> +
>       SrbExt->NumRequests = 1;
>       Request = &SrbExt->Requests[0];
>       Request->Srb = Srb;
>   
> -    Request->Operation      = BLKIF_OP_WRITE_BARRIER;
> +    Request->Operation      = Operation;
>       Request->NrSegments     = 0;
>       Request->FirstSector    = Cdb_LogicalBlock(Srb);
>       Request->NrSectors      = 0;
>   
> -    __UpdateStats(Pdo, BLKIF_OP_WRITE_BARRIER);
> +    __UpdateStats(Pdo, Operation);
>       QueueInsertTail(&Pdo->PreparedSrbs, Srb);
>   }
>   
> @@ -692,6 +706,13 @@ PdoCompleteSubmittedRequest(
>               Pdo->Frontend.FeatureBarrier = FALSE;
>           }
>           break;
> +    case BLKIF_OP_FLUSH_DISKCACHE:
> +        LogVerbose("FLUSH\n");
> +        if (Status == BLKIF_RSP_EOPNOTSUPP) {
> +            // remove supported feature

Shouldn't this say 'remove unsupported feature'?

> +            Pdo->Frontend.FeatureFlush = FALSE;
> +        }
> +        break;
>       case BLKIF_OP_DISCARD:
>           LogVerbose("DISCARD\n");
>           if (Status == BLKIF_RSP_EOPNOTSUPP) {