| Message ID | 20180404085418.29205-2-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 04/04/18 01:54, Damien Le Moal wrote: > static int sd_zbc_check_zone_size(struct scsi_disk *sdkp) > { > + u64 sdkp_zone_blocks = sdkp->zone_blocks; Shouldn't this variable be initialized to zero such that zone size changes are accepted even if the SAME field in the REPORT ZONES response is zero? Thanks, Bart.
Bart, On 4/5/18 00:08, Bart Van Assche wrote: > On 04/04/18 01:54, Damien Le Moal wrote: >> static int sd_zbc_check_zone_size(struct scsi_disk *sdkp) >> { >> + u64 sdkp_zone_blocks = sdkp->zone_blocks; > > Shouldn't this variable be initialized to zero such that zone size > changes are accepted even if the SAME field in the REPORT ZONES response > is zero? sdkp_zone_blocks will be 0 when sd_zbc_check_zone_size() is called for the first scan of a disk and will hold the current disk value if sd_zbc_check_zone_size() is called on a revalidate after first scan. If the initial value is 0, there is no check and the variable is first initialized. Otherwise, the value is compared to the zone size reported. In both cases, the zone size change will be cought. But granted, setting the value initially to 0 is easier to understand. I will also change: } else { sdkp->zone_blocks = zone_blocks; sdkp->zone_shift = ilog2(zone_blocks); } to } else if (sdkp->zone_blocks != zone_blocks) { sdkp->zone_blocks = zone_blocks; sdkp->zone_shift = ilog2(zone_blocks); } to make things really clear. Similarly to a capacity change, It may also be good to add a warning message. After all, on a drive swap, we can have the case where the capacity does not change, but the zone size does. Sending a v2. Best regards. -- Damien Le Moal, Western Digital
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index 89cf4498f535..b59454ed5087 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -403,6 +403,7 @@ static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf) */ static int sd_zbc_check_zone_size(struct scsi_disk *sdkp) { + u64 sdkp_zone_blocks = sdkp->zone_blocks; u64 zone_blocks = 0; sector_t block = 0; unsigned char *buf; @@ -412,8 +413,6 @@ static int sd_zbc_check_zone_size(struct scsi_disk *sdkp) int ret; u8 same; - sdkp->zone_blocks = 0; - /* Get a buffer */ buf = kmalloc(SD_ZBC_BUF_SIZE, GFP_KERNEL); if (!buf) @@ -446,11 +445,11 @@ static int sd_zbc_check_zone_size(struct scsi_disk *sdkp) /* Parse zone descriptors */ while (rec < buf + buf_len) { zone_blocks = get_unaligned_be64(&rec[8]); - if (sdkp->zone_blocks == 0) { - sdkp->zone_blocks = zone_blocks; - } else if (zone_blocks != sdkp->zone_blocks && + if (sdkp_zone_blocks == 0) { + sdkp_zone_blocks = zone_blocks; + } else if (zone_blocks != sdkp_zone_blocks && (block + zone_blocks < sdkp->capacity - || zone_blocks > sdkp->zone_blocks)) { + || zone_blocks > sdkp_zone_blocks)) { zone_blocks = 0; goto out; } @@ -467,7 +466,7 @@ static int sd_zbc_check_zone_size(struct scsi_disk *sdkp) } while (block < sdkp->capacity); - zone_blocks = sdkp->zone_blocks; + zone_blocks = sdkp_zone_blocks; out: if (!zone_blocks) {
When sd_revalidate() is executed for a ZBC disk (zoned block device), sd_zbc_read_zones() is called to revalidate the disk zone configuration. This executes sd_zbc_check_zone_size() to check that the disk zone sizes are in line with the defined constraints (all zones must be the same size and a power of 2 number of LBAs). As part of its execution, sd_zbc_check_zone_size() was temporarily setting sdkp->zone_blocks to 0. If during the execution of sd_zbc_check_zone_size() within sd_revalidate() context, another context issues a command which references sdkp->zone_blocks to check zone alignment of the command (e.g. a zone reset is issued and sd_zbc_setup_reset_cmnd() is called), an invalid value for the disk zone size is used and the alignment check fails. Simply fix this by using an on-stack variable inside sd_zbc_check_zone_size() instead of directly using sdkp->zone_blocks. This change is valid for both revalidate as well as for the first scan of the device. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> Cc: <stable@vger.kernel.org> --- drivers/scsi/sd_zbc.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)