Re: [PATCH v4 9/9] scsi: sd_zbc: define storage element management operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: sd_zbc: concurrent I/O races with storage element modification due to unfrozen queue
- [Medium] scsi: sd_zbc: nr_zones calculation for ALL_ACCESS_STORAGE omits runt zone
--

commit f862e7a8a5445f6170e01d1055958c0159fb3b36
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: sd_zbc: define storage element management operations

This commit defines storage element management operations (report, remove,
restore) via struct blk_se_ops. These operations are intended for SMR disks
supporting the storage element depopulation feature.

> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 1bf5f7ea0d9c5..2f79a8d7b9d93 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
[ ... ]
> @@ -548,6 +548,239 @@ static void sd_zbc_print_zones(struct scsi_disk *sdkp)
>  			  sdkp->zone_info.zone_blocks);
>  }
>  
> +static void sd_zbc_parse_storage_element(struct scsi_disk *sdkp, u8 *desc,
> +					 struct blk_se *element)
> +{
> +	struct scsi_device *sdp = sdkp->device;
> +	sector_t zone_sectors = sd_zbc_zone_sectors(sdkp);
> +	u64 capacity;
> +
> +	memset(element, 0, sizeof(*element));
> +
> +	element->id = get_unaligned_be32(&desc[4]);
> +
> +	switch (desc[14]) {
> +	case SCSI_PHYS_ELEM_TYPE_ALL_ACCESS_STORAGE:
> +		element->type = BLK_SE_TYPE_RDWR;
> +		capacity = get_unaligned_be64(&desc[16]);
> +		if (!zone_sectors || capacity == ULLONG_MAX)
> +			element->nr_zones = 0;
> +		else
> +			element->nr_zones =
> +				logical_to_sectors(sdp, capacity) >>
> +					ilog2(zone_sectors);

[Severity: Medium]
For devices with a "runt" zone at the end, the total capacity is not a
perfect multiple of the zone size. Since the bitwise right shift performs
integer division that rounds down, could this silently truncate the
remainder blocks?

If so, does this cause the element to under-report the total number of zones
by one, creating a mismatch with the core block layer's zone count (which
properly rounds up)?

> +		break;
> +	case SCSI_PHYS_ELEM_TYPE_FRAC_ACCESS_STORAGE:
[ ... ]
> +static int sd_zbc_remove_storage_element(struct gendisk *disk,
> +					 unsigned int element_id)
> +{
> +	struct scsi_disk *sdkp = scsi_disk(disk);
> +	struct scsi_device *sdp = sdkp->device;
> +	const int timeout = sdp->request_queue->rq_timeout;
> +	struct scsi_sense_hdr sshdr;
> +	const struct scsi_exec_args exec_args = {
> +		.sshdr = &sshdr,
> +	};
> +	unsigned char cmd[16];
> +	int result;
> +
> +	if (!sdkp->modify_zones_supported)
> +		return -EOPNOTSUPP;
> +
> +	memset(cmd, 0, 16);
> +	cmd[0] = SERVICE_ACTION_IN_16;
> +	cmd[1] = SAI_REMOVE_ELEMENT_AND_MODIFY_ZONES;
> +	put_unaligned_be32(element_id, &cmd[10]);
> +
> +	result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN, NULL, 0,
> +				  timeout, 1, &exec_args);

[Severity: High]
The block layer explicitly unfreezes the queue before invoking the se_ops
callback. For example, in bdev_remove_storage_element():

    memflags = blk_mq_freeze_queue(disk->queue);
    blk_mq_unfreeze_queue(disk->queue, memflags);
    ret = disk->fops->se_ops->remove_element(disk, element_id);

Because the queue is unfrozen when sd_zbc_remove_storage_element() is
called, another process could issue direct I/Os (e.g. via io_uring) to the
queue. Can the hardware zone topology modification sent via this SCSI
command race with concurrent I/O operations and lead to I/O failures or
data corruption?

> +	if (result) {
> +		sd_printk(KERN_ERR, sdkp,
> +			  "REMOVE ELEMENT AND MODIFY ZONES failed\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009090945.1504194-1-dlemoal@kernel.org?part=9