Re: [win-pv-devel] [PATCH 1/3] Decode SCSIOP_UNMAP correctly into BLKIF_OP_DISCARD requests

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Owen smith
> Sent: 22 September 2014 12:00
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith
> Subject: [win-pv-devel] [PATCH 1/3] Decode SCSIOP_UNMAP correctly into
> BLKIF_OP_DISCARD requests
> 
> From: Owen Smith <owen.smith@xxxxxxxxxx>
> 
> SCSIOP_UNMAP requests contain a list of extents to discard and each
> BLKIF_OP_DISCARD only handles a single extent. Break a SRB doen into

s/doen/down

> multiple
> ring requests as neccessary.
> 

s/necessary/necessary

> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>  src/xenvbd/pdo.c | 101
> ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 86 insertions(+), 15 deletions(-)
> 
> diff --git a/src/xenvbd/pdo.c b/src/xenvbd/pdo.c
> index 9762944..c5b0b3f 100644
> --- a/src/xenvbd/pdo.c
> +++ b/src/xenvbd/pdo.c
> @@ -1445,6 +1445,41 @@ PrepareSyncCache(
>      return STATUS_SUCCESS;
>  }
> 
> +static FORCEINLINE ULONG64
> +__Get8Bytes(
> +    IN PUCHAR                   Bytes
> +    )
> +{
> +    return (ULONG64)Bytes[0] << 56 |
> +           (ULONG64)Bytes[1] << 48 |
> +           (ULONG64)Bytes[2] << 40 |
> +           (ULONG64)Bytes[3] << 32 |
> +           (ULONG64)Bytes[4] << 24 |
> +           (ULONG64)Bytes[5] << 16 |
> +           (ULONG64)Bytes[6] << 8  |
> +           (ULONG64)Bytes[7];
> +}
> +
> +static FORCEINLINE ULONG
> +__Get4Bytes(
> +    IN PUCHAR                   Bytes
> +    )
> +{
> +    return (ULONG)Bytes[0] << 24 |
> +           (ULONG)Bytes[1] << 16 |
> +           (ULONG)Bytes[2] << 8  |
> +           (ULONG)Bytes[3];
> +}
> +
> +static FORCEINLINE USHORT
> +__Get2Bytes(
> +    IN PUCHAR                   Bytes
> +    )
> +{
> +    return (USHORT)Bytes[0] << 8 |
> +           (USHORT)Bytes[1];
> +}
> +

Could we not use byteswap intrinsics here? See 
http://msdn.microsoft.com/en-us/library/a3140177.aspx

>  __checkReturn
>  static NTSTATUS
>  PrepareUnmap(
> @@ -1452,26 +1487,62 @@ PrepareUnmap(
>      __in PSCSI_REQUEST_BLOCK     Srb
>      )
>  {
> -    PXENVBD_SRBEXT  SrbExt = GetSrbExt(Srb);
> -    PXENVBD_REQUEST Request = __LookasideAlloc(&Pdo->RequestList);
> -    if (Request == NULL)
> -        return STATUS_UNSUCCESSFUL;
> +    PXENVBD_SRBEXT      SrbExt = GetSrbExt(Srb);
> +    PUNMAP_LIST_HEADER  Unmap = Srb->DataBuffer;
> +    ULONG               Count = __Get2Bytes(Unmap->DataLength) /
> __Get2Bytes(Unmap->BlockDescrDataLength);
> +    ULONG               Index;
> +    LIST_ENTRY          List;
> 
> -    SrbExt->Count = 1;
> -    // mark the SRB as pending, completion will check for pending to detect
> failures
> +    InitializeListHead(&List);
>      Srb->SrbStatus = SRB_STATUS_PENDING;
> +    SrbExt->Count = 0;
> 
> -    Request->Srb        = Srb;
> -    Request->Id         = PdoGetTag(Pdo);
> -    Request->Operation  = BLKIF_OP_DISCARD;
> -    Request->FirstSector = Cdb_LogicalBlock(Srb);
> -    Request->NrSectors   = Cdb_TransferBlock(Srb);
> -    Request->Flags       = 0;
> -    InitializeListHead(&Request->Segments);
> +    for (Index = 0; Index < Count; ++Index) {
> +        PUNMAP_BLOCK_DESCRIPTOR Descr = &Unmap->Descriptors[Index];
> +        PXENVBD_REQUEST         Request = __LookasideAlloc(&Pdo-
> >RequestList);
> +        if (Request == NULL)
> +            goto fail1;
> 
> -    __PdoIncBlkifOpCount(Pdo, Request);
> -    QueueAppend(&Pdo->PreparedReqs, &Request->Entry);
> +        ++SrbExt->Count;
> +        InsertTailList(&List, &Request->Entry);
> +
> +        Request->Srb            = Srb;
> +        Request->Id             = PdoGetTag(Pdo);
> +        Request->Operation      = BLKIF_OP_DISCARD;
> +        Request->FirstSector    = __Get8Bytes(Descr->StartingLba);
> +        Request->NrSectors      = __Get4Bytes(Descr->LbaCount);
> +        Request->Flags          = 0;
> +        InitializeListHead(&Request->Segments);
> +    }
> +
> +    for (;;) {

Any particular reason why not to use:

while (!IsListEmpty(&List)) ?

> +        PXENVBD_REQUEST Request;
> +        PLIST_ENTRY     Entry;
> +
> +        Entry = RemoveHeadList(&List);
> +        if (Entry == &List)
> +            break;
> +
> +        Request = CONTAINING_RECORD(Entry, XENVBD_REQUEST, Entry);
> +        __PdoIncBlkifOpCount(Pdo, Request);
> +        QueueAppend(&Pdo->PreparedReqs, &Request->Entry);
> +    }
>      return STATUS_SUCCESS;
> +
> +fail1:
> +    for (;;) {

Same here.

  Paul

> +        PXENVBD_REQUEST Request;
> +        PLIST_ENTRY     Entry;
> +
> +        Entry = RemoveHeadList(&List);
> +        if (Entry == &List)
> +            break;
> +
> +        Request = CONTAINING_RECORD(Entry, XENVBD_REQUEST, Entry);
> +        __LookasideFree(&Pdo->RequestList, Request);
> +        --SrbExt->Count;
> +    }
> +    return STATUS_NO_MEMORY;
>  }
> 
> 
> //=========================================================
> ====================
> --
> 2.1.0
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel

_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel