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

["Tu Dinh" <ngoc-tu.dinh@xxxxxxxxxx>]

On 04/09/2025 12:42, Owen Smith wrote:
> 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
>

Thanks,

After reading xenvbd ring/frontend code, I think I understand the logic:

* The feature flags populated here are provisional since
BLKIF_RSP_EOPNOTSUPP would cause these features to be force-disabled by
the frontend;
* Even if the feature flags are off or the backend requests failed, the
errors will be silently swallowed by xenvbd;
* The feature reinitialization in FrontendConnect/FrontendReadDiskInfo
serves to give the frontend another chance at trying these features.

I also noticed that MaximumUnmapLBACount and
MaximumUnmapBlockDescriptorCount are not set in TargetInquiryB0, does
this stop unmaps from working? It should probably be set in any case.

With that said:

Reviewed-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>

> 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
>     <mailto: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 = NULL;
>      >       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 <https://vates.tech>
>



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech