Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: sAs2j1i6hKoOjq5sp0EzGtUbv9VQyCXY+mFwrLvMx2qu0rQuh8QL+7iY+VTcJBsjiT2r1/iVoO
 +d8ZZtlzX7CYKl6r7SPTBQw4X94XcQaBnzhEjqJ1r0TxGr9w28o1hyiuTCn/OvQz+StjBgFJq3
 mOKHxdvHSNy6jRSAGqAAEgCwQ07T3B42gCu/RLU4KYjNUhw6/7lruQTFbuVNTw9LrQikIZQzTd
 JkOiNd8Bu3hkXRDWkt8vWUq4d3x55siJY5Tveey4B4htjrPTUM3+HirXbpAHqKK48gkM9V1LOi
 Wf8=
From:   Niklas Cassel <Niklas.Cassel@wdc.com>
To:     Damien Le Moal <Damien.LeMoal@wdc.com>
CC:     Jonathan Corbet <corbet@lwn.net>, Jens Axboe <axboe@kernel.dk>,
        Keith Busch <kbusch@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Subject: Re: [PATCH 1/2] block: add max_open_zones to blk-sysfs
Thread-Topic: [PATCH 1/2] block: add max_open_zones to blk-sysfs
Thread-Index: AQHWQ8iO/B03U39S+Uih8dmO//K+66j0U6aA
Date:   Thu, 2 Jul 2020 12:37:57 +0000
Message-ID: <20200702123755.GA609677@localhost.localdomain>
References: <20200616102546.491961-1-niklas.cassel@wdc.com>
 <20200616102546.491961-2-niklas.cassel@wdc.com>
 <CY4PR04MB3751C2C03ACCA263541DA348E76F0@CY4PR04MB3751.namprd04.prod.outlook.com>
In-Reply-To: <CY4PR04MB3751C2C03ACCA263541DA348E76F0@CY4PR04MB3751.namprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-ID: <22FC15E947CE2648B994A6E81934AA5F@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 89a1fe45-8594-4606-b1a3-08d81e84bf92
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Jul 2020 12:37:57.0514
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: vYVYXf+Pe468K8HyOI66baYyl2rX65IsUdoREgUzMF1oNvxbMTtEM0N33EeQO7w9Lw1bmb7T/ngpvaY2aRvloQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB4677
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jun 30, 2020 at 01:49:41AM +0000, Damien Le Moal wrote:
> On 2020/06/16 19:28, Niklas Cassel wrote:
> > diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
> > index c08f6281b614..af156529f3b6 100644
> > --- a/drivers/nvme/host/zns.c
> > +++ b/drivers/nvme/host/zns.c
> > @@ -82,6 +82,7 @@ int nvme_update_zone_info(struct gendisk *disk, struc=
t nvme_ns *ns,
> > =20
> >  	q->limits.zoned =3D BLK_ZONED_HM;
> >  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
> > +	blk_queue_max_open_zones(q, le32_to_cpu(id->mor) + 1);
> >  free_data:
> >  	kfree(id);
> >  	return status;
> > diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> > index 183a20720da9..aa3564139b40 100644
> > --- a/drivers/scsi/sd_zbc.c
> > +++ b/drivers/scsi/sd_zbc.c
> > @@ -717,6 +717,10 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, unsi=
gned char *buf)
> >  	/* The drive satisfies the kernel restrictions: set it up */
> >  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
> >  	blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE);
> > +	if (sdkp->zones_max_open =3D=3D U32_MAX)
> > +		blk_queue_max_open_zones(q, 0);
> > +	else
> > +		blk_queue_max_open_zones(q, sdkp->zones_max_open);
>=20
> This is correct only for host-managed drives. Host-aware models define th=
e
> "OPTIMAL NUMBER OF OPEN SEQUENTIAL WRITE PREFERRED ZONES" instead of a ma=
ximum
> number of open sequential write required zones.
>=20
> Since the standard does not actually explicitly define what the value of =
the
> maximum number of open sequential write required zones should be for a
> host-aware drive, I would suggest to always have the max_open_zones value=
 set to
> 0 for host-aware disks.

Isn't this already the case?

At least according to the comments:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri=
vers/scsi/sd_zbc.c?h=3Dv5.8-rc3#n555

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri=
vers/scsi/sd_zbc.c?h=3Dv5.8-rc3#n561

We seem to set

sdkp->zones_max_open =3D 0;

for host-aware, and

sdkp->zones_max_open =3D get_unaligned_be32(&buf[16]);

for host-managed.

So the blk_queue_max_open_zones(q, sdkp->zones_max_open) call in
sd_zbc_read_zones() should already export this new sysfs property
as 0 for host-aware disks.


Kind regards,
Niklas

>=20
> >  	nr_zones =3D round_up(sdkp->capacity, zone_blocks) >> ilog2(zone_bloc=
ks);
> > =20
> >  	/* READ16/WRITE16 is mandatory for ZBC disks */
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 8fd900998b4e..2f332f00501d 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -520,6 +520,7 @@ struct request_queue {
> >  	unsigned int		nr_zones;
> >  	unsigned long		*conv_zones_bitmap;
> >  	unsigned long		*seq_zones_wlock;
> > +	unsigned int		max_open_zones;
> >  #endif /* CONFIG_BLK_DEV_ZONED */
> > =20
> >  	/*
> > @@ -729,6 +730,17 @@ static inline bool blk_queue_zone_is_seq(struct re=
quest_queue *q,
> >  		return true;
> >  	return !test_bit(blk_queue_zone_no(q, sector), q->conv_zones_bitmap);
> >  }
> > +
> > +static inline void blk_queue_max_open_zones(struct request_queue *q,
> > +		unsigned int max_open_zones)
> > +{
> > +	q->max_open_zones =3D max_open_zones;
> > +}
> > +
> > +static inline unsigned int queue_max_open_zones(const struct request_q=
ueue *q)
> > +{
> > +	return q->max_open_zones;
> > +}
> >  #else /* CONFIG_BLK_DEV_ZONED */
> >  static inline unsigned int blk_queue_nr_zones(struct request_queue *q)
> >  {
> > @@ -744,6 +756,14 @@ static inline unsigned int blk_queue_zone_no(struc=
t request_queue *q,
> >  {
> >  	return 0;
> >  }
> > +static inline void blk_queue_max_open_zones(struct request_queue *q,
> > +		unsigned int max_open_zones)
> > +{
> > +}
>=20
> Why is this one necessary ? For the !CONFIG_BLK_DEV_ZONED case, no driver=
 should
> ever call this function.

Will remove in v2.=