Re: [PATCH v3 2/3] nvme: make keep-alive synchronous operation

[Christoph Hellwig <hch@lst.de>]

On Tue, Oct 08, 2024 at 11:51:51AM +0530, Nilay Shroff wrote:
> This fix helps avoid race by implementing keep-alive as a synchronous
> operation so that admin queue-usage ref counter is decremented only

Please spell out q_usage_counter as requested in the first round.

> after keep-alive command finish execution and returns its status. This
> would ensure that we don't inadvertently destroy the fabric admin queue
> until we finish processing of nvme keep-alive request and its status and
> hence it's safe to delete the queue.

I still fail to see why this requires a synchronous operation vs just
calling blk_mq_free_request and thus decrementing q_usage_counter
afrer checking the controller state.

Maybe I'm just dumb and missing the obvious even after the last
explanation, but then the commit log needs to be improved to explain
it.


> -static enum rq_end_io_ret nvme_keep_alive_end_io(struct request *rq,
> -						 blk_status_t status)
> +static void nvme_keep_alive_finish(struct request *rq,
> +				blk_status_t status,
> +				struct nvme_ctrl *ctrl)

And as a nipick, this should be:

static void nvme_keep_alive_finish(struct request *rq, blk_status_t status,
		struct nvme_ctrl *ctrl)