On 14/07/2020 15:02, Hannes Reinecke wrote:

On 7/14/20 3:16 PM, John Garry wrote:

quoted

Hi Hannes,

quoted

   static struct pqi_io_request *pqi_alloc_io_request(
-    struct pqi_ctrl_info *ctrl_info)
+    struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd)
   {
       struct pqi_io_request *io_request;
+    unsigned int limit = PQI_RESERVED_IO_SLOTS;
       u16 i = ctrl_info->next_io_request_slot;    /* benignly racy */
   -    while (1) {
+    if (scmd) {
+        u32 blk_tag = blk_mq_unique_tag(scmd->request);
+
+        i = blk_mq_unique_tag_to_tag(blk_tag) + limit;
           io_request = &ctrl_info->io_request_pool[i];

This looks ok

quoted

-        if (atomic_inc_return(&io_request->refcount) == 1)
-            break;
-        atomic_dec(&io_request->refcount);
-        i = (i + 1) % ctrl_info->max_io_slots;
+        if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {
+            atomic_dec(&io_request->refcount);
+            return NULL;
+        }
+    } else {
+        while (1) {
+            io_request = &ctrl_info->io_request_pool[i];
+            if (atomic_inc_return(&io_request->refcount) == 1)
+                break;
+            atomic_dec(&io_request->refcount);
+            i = (i + 1) % limit;

To me, the range we use here looks incorrect. I would assume we should
restrict range to [max_io_slots - PQI_RESERVED_IO_SLOTS, max_io_slots).

But then your reserved commands support would solve that.

This branch of the 'if' condition will only be taken for internal
commands, for which we only allow up to PQI_RESERVED_IO_SLOTS.
And we set the 'normal' I/O commands above at an offset, so we're fine here.

Here is the code:

----8<----
	unsigned int limit = PQI_RESERVED_IO_SLOTS;
	u16 i = ctrl_info->next_io_request_slot; /* benignly racy */

	if (scmd) {
		u32 blk_tag = blk_mq_unique_tag(scmd->request);

		i = blk_mq_unique_tag_to_tag(blk_tag) + limit;
		io_request = &ctrl_info->io_request_pool[i];
		if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {
			atomic_dec(&io_request->refcount);
			return NULL;
		}
	} else {
		while (1) {
			io_request = &ctrl_info->io_request_pool[i];
			if (atomic_inc_return(&io_request->refcount) == 1)
				break;
			atomic_dec(&io_request->refcount);
			i = (i + 1) % limit;
		}
	}

	/* benignly racy */
	ctrl_info->next_io_request_slot = (i + 1) % ctrl_info->max_io_slots;

---->8----

Is how we set ctrl_info->next_io_request_slot ok? Should it be:

ctrl_info->next_io_request_slot = (i + 1) % limit;

And also moved into 'else' leg for good measure.

Thanks,
John