Re: [dm-devel] [PATCH v3 10/19] nvme: Move NVMe and Block PR types to an array
On 10/27/22 10:18 AM, Keith Busch wrote:
> On Wed, Oct 26, 2022 at 06:19:36PM -0500, Mike Christie wrote:
>> For Reservation Report support we need to also convert from the NVMe spec
>> PR type back to the block PR definition. This moves us to an array, so in
>> the next patch we can add another helper to do the conversion without
>> having to manage 2 switches.
>>
>> Signed-off-by: Mike Christie
>> ---
>> drivers/nvme/host/pr.c | 42 +++---
>> include/linux/nvme.h | 9 +
>> 2 files changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
>> index df7eb2440c67..5c4611d15d9c 100644
>> --- a/drivers/nvme/host/pr.c
>> +++ b/drivers/nvme/host/pr.c
>> @@ -6,24 +6,28 @@
>>
>> #include "nvme.h"
>>
>> -static char nvme_pr_type(enum pr_type type)
>> +static const struct {
>> +enum nvme_pr_type nvme_type;
>> +enum pr_typeblk_type;
>> +} nvme_pr_types[] = {
>> +{ NVME_PR_WRITE_EXCLUSIVE, PR_WRITE_EXCLUSIVE },
>> +{ NVME_PR_EXCLUSIVE_ACCESS, PR_EXCLUSIVE_ACCESS },
>> +{ NVME_PR_WRITE_EXCLUSIVE_REG_ONLY, PR_WRITE_EXCLUSIVE_REG_ONLY },
>> +{ NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY, PR_EXCLUSIVE_ACCESS_REG_ONLY },
>> +{ NVME_PR_WRITE_EXCLUSIVE_ALL_REGS, PR_WRITE_EXCLUSIVE_ALL_REGS },
>> +{ NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, PR_EXCLUSIVE_ACCESS_ALL_REGS },
>> +};
>
> Wouldn't it be easier to use the block type as the array index to avoid
> the whole looped lookup?
>
> enum nvme_pr_type types[] = {
> .PR_WRITE_EXCLUSIVE = NVME_PR_WRITE_EXCLUSIVE,
> .PR_EXCLUSIVE_ACCESS = NVME_PR_EXCLUSIVE_ACCESS,
> ...
> };
It would be. However,
1. I wasn't sure how future proof we wanted it and I might have
misinterpreted Chaitanya's original review comment. The part of
the comment about handling "every new nvme_type" made me think that
we were worried there would be new types in the future. So I thought
we wanted it to be really generic and be able to handle cases where
the values could be funky like -1 in the future.
2. I also need to go from NVME_PR type to PR type, so we need a
second array. So we can either have 2 arrays or 1 array and 2
loops (the next patch in this set added the second loop).
If we don't care about #1 then I can I see 2 arrays is nicer.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 10/19] nvme: Move NVMe and Block PR types to an array
On Wed, Oct 26, 2022 at 06:19:36PM -0500, Mike Christie wrote:
> For Reservation Report support we need to also convert from the NVMe spec
> PR type back to the block PR definition. This moves us to an array, so in
> the next patch we can add another helper to do the conversion without
> having to manage 2 switches.
>
> Signed-off-by: Mike Christie
> ---
> drivers/nvme/host/pr.c | 42 +++---
> include/linux/nvme.h | 9 +
> 2 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> index df7eb2440c67..5c4611d15d9c 100644
> --- a/drivers/nvme/host/pr.c
> +++ b/drivers/nvme/host/pr.c
> @@ -6,24 +6,28 @@
>
> #include "nvme.h"
>
> -static char nvme_pr_type(enum pr_type type)
> +static const struct {
> + enum nvme_pr_type nvme_type;
> + enum pr_typeblk_type;
> +} nvme_pr_types[] = {
> + { NVME_PR_WRITE_EXCLUSIVE, PR_WRITE_EXCLUSIVE },
> + { NVME_PR_EXCLUSIVE_ACCESS, PR_EXCLUSIVE_ACCESS },
> + { NVME_PR_WRITE_EXCLUSIVE_REG_ONLY, PR_WRITE_EXCLUSIVE_REG_ONLY },
> + { NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY, PR_EXCLUSIVE_ACCESS_REG_ONLY },
> + { NVME_PR_WRITE_EXCLUSIVE_ALL_REGS, PR_WRITE_EXCLUSIVE_ALL_REGS },
> + { NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, PR_EXCLUSIVE_ACCESS_ALL_REGS },
> +};
Wouldn't it be easier to use the block type as the array index to avoid
the whole looped lookup?
enum nvme_pr_type types[] = {
.PR_WRITE_EXCLUSIVE = NVME_PR_WRITE_EXCLUSIVE,
.PR_EXCLUSIVE_ACCESS = NVME_PR_EXCLUSIVE_ACCESS,
...
};
?
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 10/19] nvme: Move NVMe and Block PR types to an array
On 10/27/22 12:16 PM, Keith Busch wrote: > On Thu, Oct 27, 2022 at 12:13:06PM -0500, michael.chris...@oracle.com wrote: >> Oh wait there was also a >> >> 3. The pr_types come from userspace so if it passes us 10 >> and we just do: >> >> types[pr_type] >> >> then we would crash due an out of bounds error. >> >> Similarly I thought there could be a bad target that does the >> same thing. > > Well, you'd of course have to check the boundaries before accessing if > you were to implement this scheme. :) Yeah :) Sorry I didn't write that reply well. I was more trying to say that we would still need a wrapper function to check the bounds, so either we have 2 arrays and 2 helper functions or 1 array and 2 helper functions. I'll see what other people's opinions are and just do what you guys end up preferring. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 10/19] nvme: Move NVMe and Block PR types to an array
On Thu, Oct 27, 2022 at 12:13:06PM -0500, michael.chris...@oracle.com wrote: > Oh wait there was also a > > 3. The pr_types come from userspace so if it passes us 10 > and we just do: > > types[pr_type] > > then we would crash due an out of bounds error. > > Similarly I thought there could be a bad target that does the > same thing. Well, you'd of course have to check the boundaries before accessing if you were to implement this scheme. :) But considering this isn't a performance path, perhaps these kinds of optimizations are not worth it. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 10/19] nvme: Move NVMe and Block PR types to an array
On 10/27/22 12:06 PM, Mike Christie wrote:
> On 10/27/22 10:18 AM, Keith Busch wrote:
>> On Wed, Oct 26, 2022 at 06:19:36PM -0500, Mike Christie wrote:
>>> For Reservation Report support we need to also convert from the NVMe spec
>>> PR type back to the block PR definition. This moves us to an array, so in
>>> the next patch we can add another helper to do the conversion without
>>> having to manage 2 switches.
>>>
>>> Signed-off-by: Mike Christie
>>> ---
>>> drivers/nvme/host/pr.c | 42 +++---
>>> include/linux/nvme.h | 9 +
>>> 2 files changed, 32 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
>>> index df7eb2440c67..5c4611d15d9c 100644
>>> --- a/drivers/nvme/host/pr.c
>>> +++ b/drivers/nvme/host/pr.c
>>> @@ -6,24 +6,28 @@
>>>
>>> #include "nvme.h"
>>>
>>> -static char nvme_pr_type(enum pr_type type)
>>> +static const struct {
>>> + enum nvme_pr_type nvme_type;
>>> + enum pr_typeblk_type;
>>> +} nvme_pr_types[] = {
>>> + { NVME_PR_WRITE_EXCLUSIVE, PR_WRITE_EXCLUSIVE },
>>> + { NVME_PR_EXCLUSIVE_ACCESS, PR_EXCLUSIVE_ACCESS },
>>> + { NVME_PR_WRITE_EXCLUSIVE_REG_ONLY, PR_WRITE_EXCLUSIVE_REG_ONLY },
>>> + { NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY, PR_EXCLUSIVE_ACCESS_REG_ONLY },
>>> + { NVME_PR_WRITE_EXCLUSIVE_ALL_REGS, PR_WRITE_EXCLUSIVE_ALL_REGS },
>>> + { NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, PR_EXCLUSIVE_ACCESS_ALL_REGS },
>>> +};
>>
>> Wouldn't it be easier to use the block type as the array index to avoid
>> the whole looped lookup?
>>
>> enum nvme_pr_type types[] = {
>> .PR_WRITE_EXCLUSIVE = NVME_PR_WRITE_EXCLUSIVE,
>> .PR_EXCLUSIVE_ACCESS = NVME_PR_EXCLUSIVE_ACCESS,
>> ...
>> };
>
> It would be. However,
>
> 1. I wasn't sure how future proof we wanted it and I might have
> misinterpreted Chaitanya's original review comment. The part of
> the comment about handling "every new nvme_type" made me think that
> we were worried there would be new types in the future. So I thought
> we wanted it to be really generic and be able to handle cases where
> the values could be funky like -1 in the future.
>
> 2. I also need to go from NVME_PR type to PR type, so we need a
> second array. So we can either have 2 arrays or 1 array and 2
> loops (the next patch in this set added the second loop).
> If we don't care about #1 then I can I see 2 arrays is nicer.
Oh wait there was also a
3. The pr_types come from userspace so if it passes us 10
and we just do:
types[pr_type]
then we would crash due an out of bounds error.
Similarly I thought there could be a bad target that does the
same thing.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
