Re: [PATCH] Clear operations exposed by backend on disconnect

[Owen Smith <owen.smith@xxxxxxxxx>]

This would mean that Discard, Barrier and Flush may change during a boot session, should the backend change.

It's currently possible to add features but not remove them, when migrating the storage backend - this can lead to the frontend issuing unsupported requests to the backend, which should be failed with BLKIF_RSP_EOPNOTSUPP, allowing the frontend to handle the failure. This is a particular issue with barrier/flush being issued from the same SCSIOP dependent on which feature(s) are supported.

While it's not a good idea to change the storage behaviour during a boot session, this is unavoidable if the backend changes its supported features. As I understand it, discard is relatively unproblematic should it be removed - discard requests are silently completed, though the backend doesn't receive the requests to free discarded regions. Changing support for barrier/flush requests may be more problematic, as these are used to signal to the backend to ensure that all data is present on the media - but migrating the storage should handle this before the features are changed.

Owen 

On Thu, Sep 4, 2025 at 10:48 AM Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx> wrote:

Hello,

On 03/09/2025 17:42, Owen Smith wrote:
> By clearing the backend operations, it will be possible to live migrate
> to a backend that exposes different operations. This will allow live migration
> to/from backends that expose different enablement for Barrier, Flush,
> and Discard operations without relying on the backend to return
> BLKIF_RSP_EOPNOTSUPP to disable an unsupported operation.
>
> Also removes the check for change while reading disk features and always
> log the values read, as this is only called once during connection.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>

The changes look OK by themselves, but I have a question:

Does this mean the availability of discard ops may now change under the
guest's feet? Looks like this has always been the case for barrier/flush
but I'm not sure if storage drivers are supposed to change their
behavior during a boot session, especially if other components may have
depended on some operations being present (e.g. for data durability
purposes).

> ---
>   src/xenvbd/frontend.c | 54 +++++++++++++++++++++----------------------
>   1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/src/xenvbd/frontend.c b/src/xenvbd/frontend.c
> index ece7527..40611cd 100644
> --- a/src/xenvbd/frontend.c
> +++ b/src/xenvbd/frontend.c
> @@ -912,17 +912,15 @@ FrontendReadDiskInfo(
>       IN  PXENVBD_FRONTEND    Frontend
>       )
>   {
> -    BOOLEAN                 Changed;
> -    BOOLEAN                 Discard;
>       BOOLEAN                 DiscardFeature = FALSE;
>       BOOLEAN                 DiscardEnable = TRUE;
>   
> -    Changed = FrontendReadFeature(Frontend,
> -                                  FeatureBarrier,
> -                                  &Frontend->DiskInfo.Barrier);
> -    Changed |= FrontendReadFeature(Frontend,
> -                                   FeatureFlushCache,
> -                                   &Frontend->DiskInfo.FlushCache);
> +    FrontendReadFeature(Frontend,
> +                        FeatureBarrier,
> +                        &Frontend->DiskInfo.Barrier);
> +    FrontendReadFeature(Frontend,
> +                        FeatureFlushCache,
> +                        &Frontend->DiskInfo.FlushCache);
>   
>       // discard related
>       FrontendReadFeature(Frontend,
> @@ -932,26 +930,19 @@ FrontendReadDiskInfo(
>                           FeatureDiscardEnable,
>                           &DiscardEnable);
>   
> -    Discard = DiscardFeature && DiscardEnable;
> +    Frontend->DiskInfo.Discard = DiscardFeature && DiscardEnable;
>   
> -    Changed |= (Discard != Frontend->DiskInfo.Discard);
> -
> -    Frontend->DiskInfo.Discard = Discard;
> -
> -    Changed |= FrontendReadFeature(Frontend,
> -                                   FeatureDiscardSecure,
> -                                   &Frontend->DiskInfo.DiscardSecure);
> -    Changed |= FrontendReadValue32(Frontend,
> -                                   FeatureDiscardAlignment,
> -                                   TRUE,
> -                                   &Frontend->DiskInfo.DiscardAlignment);
> -    Changed |= FrontendReadValue32(Frontend,
> -                                   FeatureDiscardGranularity,
> -                                   TRUE,
> -                                   &Frontend->DiskInfo.DiscardGranularity);
> -
> -    if (!Changed)
> -        return;
> +    FrontendReadFeature(Frontend,
> +                        FeatureDiscardSecure,
> +                        &Frontend->DiskInfo.DiscardSecure);
> +    FrontendReadValue32(Frontend,
> +                        FeatureDiscardAlignment,
> +                        TRUE,
> +                        &Frontend->DiskInfo.DiscardAlignment);
> +    FrontendReadValue32(Frontend,
> +                        FeatureDiscardGranularity,
> +                        TRUE,
> +                        &Frontend->DiskInfo.DiscardGranularity);
>   
>       Verbose("Target[%d] : Features: %s%s%s\n",
>                   Frontend->TargetId,
> @@ -1378,6 +1369,15 @@ FrontendDisconnect(
>           Base64Free(Frontend->Page83.Data);
>       Frontend->Page83.Data = ""> >       Frontend->Page83.Size = 0;
> +
> +    // Clear some DiskInfo values, so they can be re-read on connect.
> +    // Allows migration to a backend with different supported operations.
> +    Frontend->DiskInfo.Barrier = FALSE;
> +    Frontend->DiskInfo.FlushCache = FALSE;
> +    Frontend->DiskInfo.Discard = FALSE;
> +    Frontend->DiskInfo.DiscardSecure = FALSE;
> +    Frontend->DiskInfo.DiscardAlignment = 0;
> +    Frontend->DiskInfo.DiscardGranularity = 0;
>   }
>   __drv_requiresIRQL(DISPATCH_LEVEL)
>   static FORCEINLINE VOID



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech