Re: [PATCH] virtio-scsi: replace target spinlock with seqcount

[Venkatesh Srinivas <venkateshs@google.com>]

Hi,

I think this patch has a small race involving just two commands:

1. The first command to a target is in virtscsi_pick_vq(), after
    atomic_inc_return(&tgt->tgt_lock)) but before write_seqcount_begin()
2. A second command to the same target enters virtscsi_pick_vq(). It
    will hit the same atomic inc, take the upper branch of the
    conditional, and read out a stale (or NULL) tgt->req_vq.

Specifically:

@@ -508,19 +507,33 @@ static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi,
  	unsigned long flags;
  	u32 queue_num;
  
-	spin_lock_irqsave(&tgt->tgt_lock, flags);
+	local_irq_save(flags);
+	if (atomic_inc_return(&tgt->reqs) > 1) {
+		unsigned long seq;
+
+		do {
+			seq = read_seqcount_begin(&tgt->tgt_seq);
+			vq = tgt->req_vq;
+		} while (read_seqcount_retry(&tgt->tgt_seq, seq));
+	} else {

A second virtscsi_pick_vq() here will read a stale or NULL tgt->req_vq.

+		/* no writes can be concurrent because of atomic_t */
+		write_seqcount_begin(&tgt->tgt_seq);
+
+		/* keep previous req_vq if there is reader found */
+		if (unlikely(atomic_read(&tgt->reqs) > 1)) {
+			vq = tgt->req_vq;
+			goto unlock;
+		}
  
-	if (atomic_inc_return(&tgt->reqs) > 1)
-		vq = tgt->req_vq;
-	else {
  		queue_num = smp_processor_id();
  		while (unlikely(queue_num >= vscsi->num_queues))
  			queue_num -= vscsi->num_queues;
-
  		tgt->req_vq = vq = &vscsi->req_vqs[queue_num];
+ unlock:
+		write_seqcount_end(&tgt->tgt_seq);
  	}
+	local_irq_restore(flags);
  
-	spin_unlock_irqrestore(&tgt->tgt_lock, flags);
  	return vq;

Thanks,
-- vs;