Re: [PATCH v13 2/2] nvmet: support reservation feature

[Christoph Hellwig <hch@lst.de>]

On Tue, Oct 08, 2024 at 06:21:14PM +0800, Guixin Liu wrote:
> This patch implements the reservation feature, includes:

s/includes/including/

> 1. reservation register(register, unregister and replace).
> 2. reservation acquire(acquire, preempt, preempt and abort).
> 3. reservation release(release and clear).
> 4. reservation report.
> 5. set feature and get feature of reservation notify mask.
> 6. get log page of reservation event.

Maybe indent this bulleted list (and the other ones) by two characters
to make it more readable?

> Not supported:
> 1. persistent reservation through power loss.
> 
> Test:

Tested-by:

> Use nvme-cli and fio to test all implemented sub features:
> 1. use nvme resv-register to register host a registrant or
> unregister or replace a new key.
> 2. use nvme resv-acquire to set host to the holder, and use fio
> to send read and write io in all reservation type. And also test
> preempt and "preempt and abort".
> 3. use nvme resv-report to show all registrants and reservation
> status.
> 4. use nvme resv-release to release all registrants.
> 5. use nvme get-log to get events generated by the preceding
> operations.

That's still very basic testing, but I have no really good answer
what good test suite would be.

> +static ssize_t nvmet_ns_resv_enable_store(struct config_item *item,
> +					const char *page, size_t count)

The indentation used in most nvme code is two tabs for prototype
continuations, can you change to that? I.e.:

static ssize_t nvmet_ns_resv_enable_store(struct config_item *item,
		const char *page, size_t count)

> +#define NVMET_PR_NOTIFI_MASK_ALL \
> +		(1 << NVME_PR_NOTIFY_BIT_REG_PREEMPTED | \
> +		 1 << NVME_PR_NOTIFY_BIT_RESV_RELEASED | \
> +		 1 << NVME_PR_NOTIFY_BIT_RESV_PREEMPTED)

And here I'd expect a single tab indent.

Except for these nitpicks the code looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>