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;
From:   SeongJae Park <sj@kernel.org>
To:     Pratyush Yadav <ptyadav@amazon.de>
Cc:     SeongJae Park <sj@kernel.org>, 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 2/2] xen-blkfront: Advertise feature-persistent as user requested
Date:   Wed, 31 Aug 2022 16:20:36 +0000
Message-Id: <20220831162036.93966-1-sj@kernel.org>
In-Reply-To: <20220831155045.kxopdchlc67fmi5n@yadavpratyush.com>
References: 
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Pratyush,

On Wed, 31 Aug 2022 15:50:45 +0000 Pratyush Yadav <ptyadav@amazon.de> wrote:

> On 25/08/22 04:15PM, SeongJae Park wrote:
> > Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> > when connect") made blkback to advertise its support of the persistent
> > grants feature only if the user sets the 'feature_persistent' parameter
> > of the driver and the frontend advertised its support of the feature.
> > However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> > 'feature_persistent' parameter when connect") made the blkfront to work
> > in the same way.  That is, blkfront also advertises its support of the
> > persistent grants feature only if the user sets the 'feature_persistent'
> > parameter of the driver and the backend advertised its support of the
> > feature.
> > 
> > Hence blkback and blkfront will never advertise their support of the
> > feature but wait until the other advertises the support, even though
> > users set the 'feature_persistent' parameters of the drivers.  As a
> > result, the persistent grants feature is disabled always regardless of
> > the 'feature_persistent' values[1].
> > 
> > The problem comes from the misuse of the semantic of the advertisement
> > of the feature.  The advertisement of the feature should means only
> > availability of the feature not the decision for using the feature.
> > However, current behavior is working in the wrong way.
> > 
> > This commit fixes the issue by making the blkfront advertises its
> > support of the feature as user requested via 'feature_persistent'
> > parameter regardless of the otherend's support of the feature.
> > 
> > [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> > 
> > Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect")
> > Cc: <stable@vger.kernel.org> # 5.10.x
> > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Suggested-by: Juergen Gross <jgross@suse.com>
> > Signed-off-by: SeongJae Park <sj@kernel.org>
> > ---
> >  drivers/block/xen-blkfront.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 8e56e69fb4c4..dfae08115450 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -213,6 +213,9 @@ struct blkfront_info
> >  	unsigned int feature_fua:1;
> >  	unsigned int feature_discard:1;
> >  	unsigned int feature_secdiscard:1;
> > +	/* Connect-time cached feature_persistent parameter */
> > +	unsigned int feature_persistent_parm:1;
> > +	/* Persistent grants feature negotiation result */
> >  	unsigned int feature_persistent:1;
> >  	unsigned int bounce:1;
> >  	unsigned int discard_granularity;
> > @@ -1848,7 +1851,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
> >  		goto abort_transaction;
> >  	}
> >  	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> > -			info->feature_persistent);
> > +			info->feature_persistent_parm);
> >  	if (err)
> >  		dev_warn(&dev->dev,
> >  			 "writing persistent grants feature to xenbus");
> > @@ -2281,7 +2284,8 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)
> >  	if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
> >  		blkfront_setup_discard(info);
> >  
> > -	if (feature_persistent)
> > +	info->feature_persistent_parm = feature_persistent;
> 
> Same question as before. Why not just use feature_persistent directly?

Same answer as before, due to the possible race[1].

[1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/

> 
> > +	if (info->feature_persistent_parm)
> >  		info->feature_persistent =
> >  			!!xenbus_read_unsigned(info->xbdev->otherend,
> >  					       "feature-persistent", 0);
> 
> Aside: IMO this would look nicer as below:
> 
> 	info->feature_persistent = feature_persistent && !!xenbus_read_unsigned();

Agreed, that would also make the code more consistent with the blkback side
code.

I would make the change in the next version of this patchset.


Thanks,
SJ