Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Sep 2022 09:53:22 +0000
From:   Pratyush Yadav <ptyadav@amazon.de>
To:     SeongJae Park <sj@kernel.org>
CC:     <jgross@suse.com>, <roger.pau@citrix.com>,
        <marmarek@invisiblethingslab.com>, <mheyne@amazon.de>,
        <xen-devel@lists.xenproject.org>, <axboe@kernel.dk>,
        <linux-block@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <stable@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user
 requested
Message-ID: <20220902095207.y3whbc5mw4hyqphg@yadavpratyush.com>
References: <20220831165824.94815-1-sj@kernel.org>
 <20220831165824.94815-2-sj@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220831165824.94815-2-sj@kernel.org>
Precedence: bulk

On 31/08/22 04:58PM, SeongJae Park wrote:
> The advertisement of the persistent grants feature (writing
> 'feature-persistent' to xenbus) should mean not the decision for using
> the feature but only the availability of the feature.  However, commit
> aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
> grants") made a field of blkback, which was a place for saving only the
> negotiation result, to be used for yet another purpose: caching of the
> 'feature_persistent' parameter value.  As a result, the advertisement,
> which should follow only the parameter value, becomes inconsistent.
> 
> This commit fixes the misuse of the semantic by making blkback saves the
> parameter value in a separate place and advertises the support based on
> only the saved value.
> 
> Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
> Cc: <stable@vger.kernel.org> # 5.10.x
> Suggested-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
>  drivers/block/xen-blkback/common.h | 3 +++
>  drivers/block/xen-blkback/xenbus.c | 6 ++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index bda5c815e441..a28473470e66 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -226,6 +226,9 @@ struct xen_vbd {
>         sector_t                size;
>         unsigned int            flush_support:1;
>         unsigned int            discard_secure:1;
> +       /* Connect-time cached feature_persistent parameter value */
> +       unsigned int            feature_gnt_persistent_parm:1;

Continuing over from the previous version:

> > If feature_gnt_persistent_parm is always going to be equal to 
> > feature_persistent, then why introduce it at all? Why not just use 
> > feature_persistent directly? This way you avoid adding an extra flag 
> > whose purpose is not immediately clear, and you also avoid all the 
> > mess with setting this flag at the right time.
>
> Mainly because the parameter should read twice (once for 
> advertisement, and once later just before the negotitation, for 
> checking if we advertised or not), and the user might change the 
> parameter value between the two reads.
>
> For the detailed available sequence of the race, you could refer to the 
> prior conversation[1].
>
> [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/

Okay, I see. Thanks for the pointer. But still, I think it would be 
better to not maintain two copies of the value. How about doing:

	blkif->vbd.feature_gnt_persistent =
		xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);

This makes it quite clear that we only enable persistent grants if 
_both_ ends support it. We can do the same for blkfront.

> +       /* Persistent grants feature negotiation result */
>         unsigned int            feature_gnt_persistent:1;
>         unsigned int            overflow_max_grants:1;
>  };
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index ee7ad2fb432d..c0227dfa4688 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -907,7 +907,7 @@ static void connect(struct backend_info *be)
>         xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
> 
>         err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> -                       be->blkif->vbd.feature_gnt_persistent);
> +                       be->blkif->vbd.feature_gnt_persistent_parm);
>         if (err) {
>                 xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
>                                  dev->nodename);
> @@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
>                 return -ENOSYS;
>         }
> 
> -       blkif->vbd.feature_gnt_persistent = feature_persistent &&
> +       blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
> +       blkif->vbd.feature_gnt_persistent =
> +               blkif->vbd.feature_gnt_persistent_parm &&
>                 xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> 
>         blkif->vbd.overflow_max_grants = 0;
> --
> 2.25.1
> 

-- 
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879