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: Z14eaFcGH99rOUTDeArLataOAYr/c6sc/5jidefN90lyk2m1xKY+BxMFDyJPuyquHZLxm1gOC5
 2PcLlfdPFI87qRYQN1+3CzGVy/3ZTdRwA+jUlpUQ03Y3t5+93jQYl3MvdpjytHp6BFD1n+f2mH
 jRhgSKUZmcBHVqhQymGmyc+Ho2fFlWwJO9gb+MHGn/jYo1MtTki/aZ/4r/ynxxjlorX4iI9PsI
 KYZttSSFVepzKkPetBSqtOW8eoJxbag7afTTTjKNBSJJV6XM2kkrdLQlppDuXZIK57J8s7bCPL
 Pfo=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Kanchan Joshi <joshi.k@samsung.com>,
        "axboe@kernel.dk" <axboe@kernel.dk>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "selvakuma.s1@samsung.com" <selvakuma.s1@samsung.com>,
        "nj.shetty@samsung.com" <nj.shetty@samsung.com>,
        "javier.gonz@samsung.com" <javier.gonz@samsung.com>
Subject: Re: [PATCH v2 1/1] null_blk: synchronization fix for zoned device
Thread-Topic: [PATCH v2 1/1] null_blk: synchronization fix for zoned device
Thread-Index: AQHWlX4IaLdglWyy80qXrKErMeNvWA==
Date:   Mon, 28 Sep 2020 10:11:57 +0000
Message-ID: <CY4PR04MB37511C151D0F37DC62BB4CE6E7350@CY4PR04MB3751.namprd04.prod.outlook.com>
References: <20200928095549.184510-1-joshi.k@samsung.com>
 <CGME20200928095914epcas5p1beae8d5a201c35b598fde8288532d58d@epcas5p1.samsung.com>
 <20200928095549.184510-2-joshi.k@samsung.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5562aceb-aa40-40af-2fba-08d86396ef18
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Sep 2020 10:11:57.8530
 (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: 68CeXxX9Ne7/kImQN7VoFBFPSdmgTT2S+dpnYYb06NXP4PK0PxcaoTK3AUA2I16c+RBMVc1g0+mDn3ha30l9ZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR0401MB3633
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2020/09/28 18:59, Kanchan Joshi wrote:=0A=
> Parallel write,read,zone-mgmt operations accessing/altering zone state=0A=
> and write-pointer may get into race. Avoid the situation by using a new=
=0A=
> spinlock for zoned device.=0A=
> Concurrent zone-appends (on a zone) returning same write-pointer issue=0A=
> is also avoided using this lock.=0A=
> =0A=
> Fixes: e0489ed5daeb ("null_blk: Support REQ_OP_ZONE_APPEND")=0A=
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>=0A=
> ---=0A=
>  drivers/block/null_blk.h       |  1 +=0A=
>  drivers/block/null_blk_zoned.c | 22 ++++++++++++++++++----=0A=
>  2 files changed, 19 insertions(+), 4 deletions(-)=0A=
> =0A=
> diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h=0A=
> index daed4a9c3436..28099be50395 100644=0A=
> --- a/drivers/block/null_blk.h=0A=
> +++ b/drivers/block/null_blk.h=0A=
> @@ -44,6 +44,7 @@ struct nullb_device {=0A=
>  	unsigned int nr_zones;=0A=
>  	struct blk_zone *zones;=0A=
>  	sector_t zone_size_sects;=0A=
> +	spinlock_t zone_lock;=0A=
>  =0A=
>  	unsigned long size; /* device size in MB */=0A=
>  	unsigned long completion_nsec; /* time in ns to complete a request */=
=0A=
> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zone=
d.c=0A=
> index 3d25c9ad2383..e8d8b13aaa5a 100644=0A=
> --- a/drivers/block/null_blk_zoned.c=0A=
> +++ b/drivers/block/null_blk_zoned.c=0A=
> @@ -45,6 +45,7 @@ int null_init_zoned_dev(struct nullb_device *dev, struc=
t request_queue *q)=0A=
>  	if (!dev->zones)=0A=
>  		return -ENOMEM;=0A=
>  =0A=
> +	spin_lock_init(&dev->zone_lock);=0A=
>  	if (dev->zone_nr_conv >=3D dev->nr_zones) {=0A=
>  		dev->zone_nr_conv =3D dev->nr_zones - 1;=0A=
>  		pr_info("changed the number of conventional zones to %u",=0A=
> @@ -131,8 +132,11 @@ int null_report_zones(struct gendisk *disk, sector_t=
 sector,=0A=
>  		 * So use a local copy to avoid corruption of the device zone=0A=
>  		 * array.=0A=
>  		 */=0A=
> +		spin_lock_irq(&dev->zone_lock);=0A=
>  		memcpy(&zone, &dev->zones[first_zone + i],=0A=
>  		       sizeof(struct blk_zone));=0A=
> +		spin_unlock_irq(&dev->zone_lock);=0A=
> +=0A=
>  		error =3D cb(&zone, i, data);=0A=
>  		if (error)=0A=
>  			return error;=0A=
> @@ -277,18 +281,28 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd=
 *cmd, enum req_opf op,=0A=
>  blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_opf =
op,=0A=
>  				    sector_t sector, sector_t nr_sectors)=0A=
>  {=0A=
> +	blk_status_t sts;=0A=
> +	struct nullb_device *dev =3D cmd->nq->dev;=0A=
> +=0A=
> +	spin_lock_irq(&dev->zone_lock);=0A=
>  	switch (op) {=0A=
>  	case REQ_OP_WRITE:=0A=
> -		return null_zone_write(cmd, sector, nr_sectors, false);=0A=
> +		sts =3D null_zone_write(cmd, sector, nr_sectors, false);=0A=
> +		break;=0A=
>  	case REQ_OP_ZONE_APPEND:=0A=
> -		return null_zone_write(cmd, sector, nr_sectors, true);=0A=
> +		sts =3D null_zone_write(cmd, sector, nr_sectors, true);=0A=
> +		break;=0A=
>  	case REQ_OP_ZONE_RESET:=0A=
>  	case REQ_OP_ZONE_RESET_ALL:=0A=
>  	case REQ_OP_ZONE_OPEN:=0A=
>  	case REQ_OP_ZONE_CLOSE:=0A=
>  	case REQ_OP_ZONE_FINISH:=0A=
> -		return null_zone_mgmt(cmd, op, sector);=0A=
> +		sts =3D null_zone_mgmt(cmd, op, sector);=0A=
> +		break;=0A=
>  	default:=0A=
> -		return null_process_cmd(cmd, op, sector, nr_sectors);=0A=
> +		sts =3D null_process_cmd(cmd, op, sector, nr_sectors);=0A=
>  	}=0A=
> +	spin_unlock_irq(&dev->zone_lock);=0A=
> +=0A=
> +	return sts;=0A=
>  }=0A=
> =0A=
=0A=
Looks good.=0A=
=0A=
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>=0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=