| Message ID | 20221026231945.6609-11-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use block pr_ops in LIO | expand |
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 <michael.christie@oracle.com> > --- > 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_type blk_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, ... }; ?
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 <michael.christie@oracle.com> >> --- >> 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_type blk_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.
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 <michael.christie@oracle.com> >>> --- >>> 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_type blk_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.
On Thu, Oct 27, 2022 at 12:13:06PM -0500, michael.christie@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.
On 10/27/22 12:16 PM, Keith Busch wrote: > On Thu, Oct 27, 2022 at 12:13:06PM -0500, michael.christie@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.
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_type blk_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 }, +}; + +static enum nvme_pr_type nvme_pr_type_from_blk(enum pr_type type) { - switch (type) { - case PR_WRITE_EXCLUSIVE: - return 1; - case PR_EXCLUSIVE_ACCESS: - return 2; - case PR_WRITE_EXCLUSIVE_REG_ONLY: - return 3; - case PR_EXCLUSIVE_ACCESS_REG_ONLY: - return 4; - case PR_WRITE_EXCLUSIVE_ALL_REGS: - return 5; - case PR_EXCLUSIVE_ACCESS_ALL_REGS: - return 6; - default: - return 0; + int i; + + for (i = 0; i < ARRAY_SIZE(nvme_pr_types); i++) { + if (nvme_pr_types[i].blk_type == type) + return nvme_pr_types[i].nvme_type; } + + return 0; } static int nvme_send_ns_head_pr_command(struct block_device *bdev, @@ -91,7 +95,7 @@ static int nvme_pr_reserve(struct block_device *bdev, u64 key, if (flags & ~PR_FL_IGNORE_KEY) return -EOPNOTSUPP; - cdw10 = nvme_pr_type(type) << 8; + cdw10 = nvme_pr_type_from_blk(type) << 8; cdw10 |= ((flags & PR_FL_IGNORE_KEY) ? 1 << 3 : 0); return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_acquire); } @@ -99,7 +103,7 @@ static int nvme_pr_reserve(struct block_device *bdev, u64 key, static int nvme_pr_preempt(struct block_device *bdev, u64 old, u64 new, enum pr_type type, bool abort) { - u32 cdw10 = nvme_pr_type(type) << 8 | (abort ? 2 : 1); + u32 cdw10 = nvme_pr_type_from_blk(type) << 8 | (abort ? 2 : 1); return nvme_pr_command(bdev, cdw10, old, new, nvme_cmd_resv_acquire); } @@ -113,7 +117,7 @@ static int nvme_pr_clear(struct block_device *bdev, u64 key) static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type) { - u32 cdw10 = nvme_pr_type(type) << 8 | (key ? 0 : 1 << 3); + u32 cdw10 = nvme_pr_type_from_blk(type) << 8 | (key ? 0 : 1 << 3); return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release); } diff --git a/include/linux/nvme.h b/include/linux/nvme.h index 5bc9c84dc216..d0bd15f527fc 100644 --- a/include/linux/nvme.h +++ b/include/linux/nvme.h @@ -757,6 +757,15 @@ enum { NVME_LBART_ATTRIB_HIDE = 1 << 1, }; +enum nvme_pr_type { + NVME_PR_WRITE_EXCLUSIVE = 1, + NVME_PR_EXCLUSIVE_ACCESS = 2, + NVME_PR_WRITE_EXCLUSIVE_REG_ONLY = 3, + NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY = 4, + NVME_PR_WRITE_EXCLUSIVE_ALL_REGS = 5, + NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS = 6, +}; + enum nvme_eds { NVME_EXTENDED_DATA_STRUCT = 0x1, };
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 <michael.christie@oracle.com> --- drivers/nvme/host/pr.c | 42 +++++++++++++++++++++++------------------- include/linux/nvme.h | 9 +++++++++ 2 files changed, 32 insertions(+), 19 deletions(-)