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

[Guixin Liu]

在 2024/10/10 17:30, Christoph Hellwig 写道:
> On Tue, Oct 08, 2024 at 06:21:14PM +0800, Guixin Liu wrote:
>> This patch implements the reservation feature, includes:
> s/includes/including/
Sure, change to "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?
Indeed.
>> Not supported:
>> 1. persistent reservation through power loss.
>>
>> Test:
> Tested-by:

Well, I found that "Tested-by: " will be recognized by checkpatch.pl,

it reports warning and error:

WARNING: Use a single space after Tested-by:
#17:
Tested-by:

ERROR: Unrecognized email address: ''
#17:
Tested-by:


I will change "Test" to "Test cases".

>> 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)
OK, I will change this in v14.
>> +#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.
OK.
>
> Except for these nitpicks the code looks good to me:
>
> Reviewed-by: Christoph Hellwig <hch at lst.de>

Thanks, I will send v14 soon.

Best Regards,

Guixin Liu