On 04/15/2016 08:31 PM, Bart Van Assche wrote:
On 04/04/2016 03:00 AM, Hannes Reinecke wrote:
@@ -728,6 +729,10 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd
*cmd)
int ret = 0;
char *buf;
struct page *page = NULL;
+#ifdef CONFIG_SCSI_ZBC
+ struct blk_zone *zone;
+ unsigned long flags;
+#endif
There is a strong preference in the Linux kernel for avoiding #ifdefs
and to move code that depends on the value of a CONFIG_* variable into a
file for which the compilation depends on that CONFIG_* variable. Please
consider to move the ZBC code from sd_setup_discard_cmnd() into a new
function in sd_zbc.c.
Well; the integrity code also added some #ifdefs, so I thought it would
be acceptable, too.
I can reconsider it, of course, if preferred.
+#ifdef CONFIG_SCSI_ZBC
+ zone = blk_lookup_zone(rq->q, sector);
+ if (!zone) {
+ ret = BLKPREP_KILL;
+ goto out;
+ }
+ spin_lock_irqsave(&zone->lock, flags);
+ if (zone->state == BLK_ZONE_BUSY) {
+ sd_printk(KERN_INFO, sdkp,
+ "Discarding busy zone %zu/%zu\n",
+ zone->start, zone->len);
+ spin_unlock_irqrestore(&zone->lock, flags);
+ ret = BLKPREP_DEFER;
+ goto out;
+ }
+ if (!blk_zone_is_smr(zone)) {
+ sd_printk(KERN_INFO, sdkp,
+ "Discarding %s zone %zu/%zu\n",
+ blk_zone_is_cmr(zone) ? "CMR" : "unknown",
+ zone->start, zone->len);
+ spin_unlock_irqrestore(&zone->lock, flags);
+ ret = BLKPREP_DONE;
+ goto out;
+ }
+ if (blk_zone_is_empty(zone)) {
+ spin_unlock_irqrestore(&zone->lock, flags);
+ ret = BLKPREP_DONE;
+ goto out;
+ }
+ if (zone->start != sector ||
+ zone->len < nr_sectors) {
+ sd_printk(KERN_INFO, sdkp,
+ "Misaligned RESET WP, start %zu/%zu "
+ "len %zu/%u\n",
+ zone->start, sector, zone->len, nr_sectors);
+ spin_unlock_irqrestore(&zone->lock, flags);
+ ret = BLKPREP_KILL;
+ goto out;
+ }
+ /*
+ * Opportunistic setting, needs to be fixed up
+ * if RESET WRITE POINTER fails.
+ */
+ zone->wp = zone->start;
+ spin_unlock_irqrestore(&zone->lock, flags);
+#endif
> cmd->cmd_len = 16;
> cmd->cmnd[0] = ZBC_OUT;
> cmd->cmnd[1] = ZO_RESET_WRITE_POINTER;
Which mechanism prevents that zone->state is modified after it has been
checked and before the RESET WRITE POINTER command has finished?
See below.
@@ -990,6 +1041,13 @@ static int sd_setup_read_write_cmnd(struct
scsi_cmnd *SCpnt)
SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt, "block=%llu\n",
(unsigned long long)block));
+ if (sdkp->zoned == 1 || sdp->type == TYPE_ZBC) {
+ /* sd_zbc_lookup_zone lba is in block layer sector units */
+ ret = sd_zbc_lookup_zone(sdkp, rq, block, this_count);
+ if (ret != BLKPREP_OK)
+ goto out;
+ }
+
Which mechanism guarantees that the above code won't run concurrently
with zbc_parse_zones()?
See below. There is no overall lock (the zone layout is considered
immutable once set), but each zone has its own spinlock.
If the zone state is set to BUSY (see below) sd_zbc_lookup_zone will
return BLKPREP_DEFER, and the request won't be scheduled.
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 5debd49..35c75fa 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -65,6 +65,12 @@ struct scsi_disk {
struct scsi_device *device;
struct device dev;
struct gendisk *disk;
+#ifdef CONFIG_SCSI_ZBC
+ struct workqueue_struct *zone_work_q;
+ unsigned long zone_flags;
+#define SD_ZBC_ZONE_RESET 1
+#define SD_ZBC_ZONE_INIT 2
+#endif
The above two constants are only used in source file sd_zbc.c. Have you
considered to move the definition of these constants into sd_zbc.c?
+#undef SD_ZBC_DEBUG
Please use the dynamic_debug facility instead of #ifdef SD_ZBC_DEBUG +
sd_printk().
Okay, will be doing so.
+void sd_zbc_refresh_zone_work(struct work_struct *work)
+{
+ struct zbc_update_work *zbc_work =
+ container_of(work, struct zbc_update_work, zone_work);
+ struct scsi_disk *sdkp = zbc_work->sdkp;
+ struct request_queue *q = sdkp->disk->queue;
+ unsigned int zone_buflen;
+ int ret;
+ sector_t last_sector;
+ sector_t capacity = logical_to_sectors(sdkp->device,
sdkp->capacity);
+ sector_t zone_lba = sectors_to_logical(sdkp->device,
+ zbc_work->zone_sector);
+
+ zone_buflen = zbc_work->zone_buflen;
+ ret = sd_zbc_report_zones(sdkp, zone_lba, zbc_work->zone_buf,
+ zone_buflen);
+ if (ret)
+ goto done_free;
+
+ last_sector = zbc_parse_zones(sdkp, zbc_work->zone_buf,
zone_buflen);
+ if (last_sector != -1 && last_sector < capacity) {
+ if (test_bit(SD_ZBC_ZONE_RESET, &sdkp->zone_flags)) {
+#ifdef SD_ZBC_DEBUG
+ sd_printk(KERN_INFO, sdkp,
+ "zones in reset, cancelling refresh\n");
+#endif
+ ret = -EAGAIN;
+ goto done_free;
+ }
+
+ zbc_work->zone_sector = last_sector;
+ queue_work(sdkp->zone_work_q, &zbc_work->zone_work);
+ /* Kick request queue to be on the safe side */
+ goto done_start_queue;
+ }
+done_free:
+ kfree(zbc_work);
+ if (test_and_clear_bit(SD_ZBC_ZONE_INIT, &sdkp->zone_flags) &&
ret) {
+ sd_printk(KERN_INFO, sdkp,
+ "Cancelling zone initialisation\n");
+ }
+done_start_queue:
+ if (q->mq_ops)
+ blk_mq_start_hw_queues(q);
+ else {
+ unsigned long flags;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_start_queue(q);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+ }
+}
Which mechanism prevents concurrent execution of
sd_zbc_refresh_zone_work() and READ and WRITE commands?
When sd_zbc_refresh_zone_work is started it'll set all zones to be
updated to 'BUSY', and the prep_rq() function will defer any I/O
until REPORT_ZONES has returned and updated the state to something
other, like BLK_ZONE_OPEN.
Thanks for the review.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
