| Message ID | 20190110143051.52305-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] : Switch to use new generic UUID API | expand |
On Thu, Jan 10, 2019 at 04:30:51PM +0200, Andy Shevchenko wrote: > There are new types and helpers that are supposed to be used in new code. > > As a preparation to get rid of legacy types and API functions do > the conversion here. This seems to miss a "lightnvm" in the subject line. > static inline void pblk_setup_uuid(struct pblk *pblk) > { > + guid_gen((guid_t *)&pblk->instance_uuid); > } I think we can just kill this wrapper. But more importantly the instance_uuid fied, and the header.uuid one it is copied from should be turned into an actual guid_t, the memcpys and memcmps should also be replaced with the proper UUID API.
On Mon, Jan 21, 2019 at 09:47:32AM +0100, Christoph Hellwig wrote: > On Thu, Jan 10, 2019 at 04:30:51PM +0200, Andy Shevchenko wrote: > > There are new types and helpers that are supposed to be used in new code. > > > > As a preparation to get rid of legacy types and API functions do > > the conversion here. > > This seems to miss a "lightnvm" in the subject line. > > > static inline void pblk_setup_uuid(struct pblk *pblk) > > { > > + guid_gen((guid_t *)&pblk->instance_uuid); > > } > > I think we can just kill this wrapper. > > But more importantly the instance_uuid fied, and the header.uuid one > it is copied from should be turned into an actual guid_t, the memcpys > and memcmps should also be replaced with the proper UUID API. header.uuid is defined using __u8 type, I'm not sure we can use guid_t there.
> On 24 Jan 2019, at 13.16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Jan 21, 2019 at 09:47:32AM +0100, Christoph Hellwig wrote: >> On Thu, Jan 10, 2019 at 04:30:51PM +0200, Andy Shevchenko wrote: >>> There are new types and helpers that are supposed to be used in new code. >>> >>> As a preparation to get rid of legacy types and API functions do >>> the conversion here. >> >> This seems to miss a "lightnvm" in the subject line. >> >>> static inline void pblk_setup_uuid(struct pblk *pblk) >>> { >>> + guid_gen((guid_t *)&pblk->instance_uuid); >>> } >> >> I think we can just kill this wrapper. >> >> But more importantly the instance_uuid fied, and the header.uuid one >> it is copied from should be turned into an actual guid_t, the memcpys >> and memcmps should also be replaced with the proper UUID API. > > header.uuid is defined using __u8 type, I'm not sure we can use guid_t there. > We can turn it into a guid_t and bump the minor version. Javier
On Thu, Jan 24, 2019 at 3:19 PM Javier González <javier@javigon.com> wrote: > > On 24 Jan 2019, at 13.16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Jan 21, 2019 at 09:47:32AM +0100, Christoph Hellwig wrote: > >> On Thu, Jan 10, 2019 at 04:30:51PM +0200, Andy Shevchenko wrote: > >>> There are new types and helpers that are supposed to be used in new code. > >>> > >>> As a preparation to get rid of legacy types and API functions do > >>> the conversion here. > >> > >> This seems to miss a "lightnvm" in the subject line. > >> > >>> static inline void pblk_setup_uuid(struct pblk *pblk) > >>> { > >>> + guid_gen((guid_t *)&pblk->instance_uuid); > >>> } > >> > >> I think we can just kill this wrapper. > >> > >> But more importantly the instance_uuid fied, and the header.uuid one > >> it is copied from should be turned into an actual guid_t, the memcpys > >> and memcmps should also be replaced with the proper UUID API. > > > > header.uuid is defined using __u8 type, I'm not sure we can use guid_t there. > > > > We can turn it into a guid_t and bump the minor version. It's not so easy. __uXX types are dedicated for external APIs. guid_t is kernel internal type disregard of (still) presence some uapi bits. So, the question is those __uXX types in the driver definition is a simple mistake, (weird) style decision, or what?
> On 24 Jan 2019, at 14.36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jan 24, 2019 at 3:19 PM Javier González <javier@javigon.com> wrote: >>> On 24 Jan 2019, at 13.16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >>> On Mon, Jan 21, 2019 at 09:47:32AM +0100, Christoph Hellwig wrote: >>>> On Thu, Jan 10, 2019 at 04:30:51PM +0200, Andy Shevchenko wrote: >>>>> There are new types and helpers that are supposed to be used in new code. >>>>> >>>>> As a preparation to get rid of legacy types and API functions do >>>>> the conversion here. >>>> >>>> This seems to miss a "lightnvm" in the subject line. >>>> >>>>> static inline void pblk_setup_uuid(struct pblk *pblk) >>>>> { >>>>> + guid_gen((guid_t *)&pblk->instance_uuid); >>>>> } >>>> >>>> I think we can just kill this wrapper. >>>> >>>> But more importantly the instance_uuid fied, and the header.uuid one >>>> it is copied from should be turned into an actual guid_t, the memcpys >>>> and memcmps should also be replaced with the proper UUID API. >>> >>> header.uuid is defined using __u8 type, I'm not sure we can use guid_t there. >> >> We can turn it into a guid_t and bump the minor version. > > It's not so easy. __uXX types are dedicated for external APIs. guid_t > is kernel internal type disregard of (still) presence some uapi bits. > So, the question is those __uXX types in the driver definition is a > simple mistake, (weird) style decision, or what? > I would define it as a mistake and I think it is worth fixing it. At the moment we are only using this uuid for recovery purposes, to discard data from a different pblk instance, so there should not be a big impact outside of pblk itself. Am I missing something? Javier
On Thu, Jan 24, 2019 at 3:45 PM Javier González <javier@javigon.com> wrote: > > On 24 Jan 2019, at 14.36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jan 24, 2019 at 3:19 PM Javier González <javier@javigon.com> wrote: > >>> On 24 Jan 2019, at 13.16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >>> header.uuid is defined using __u8 type, I'm not sure we can use guid_t there. > >> > >> We can turn it into a guid_t and bump the minor version. > > > > It's not so easy. __uXX types are dedicated for external APIs. guid_t > > is kernel internal type disregard of (still) presence some uapi bits. > > So, the question is those __uXX types in the driver definition is a > > simple mistake, (weird) style decision, or what? > > > > I would define it as a mistake and I think it is worth fixing it. At the > moment we are only using this uuid for recovery purposes, to discard > data from a different pblk instance, Does this come from outside of the kernel in any mean (user space, data from device, etc)? It sounds to me like it does. In this case there is no mistake and we may not use guid_t there. > so there should not be a big impact > outside of pblk itself. Am I missing something?
> On 24 Jan 2019, at 15.13, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jan 24, 2019 at 3:45 PM Javier González <javier@javigon.com> wrote: >>> On 24 Jan 2019, at 14.36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Thu, Jan 24, 2019 at 3:19 PM Javier González <javier@javigon.com> wrote: >>>>> On 24 Jan 2019, at 13.16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >>>>> header.uuid is defined using __u8 type, I'm not sure we can use guid_t there. >>>> >>>> We can turn it into a guid_t and bump the minor version. >>> >>> It's not so easy. __uXX types are dedicated for external APIs. guid_t >>> is kernel internal type disregard of (still) presence some uapi bits. >>> So, the question is those __uXX types in the driver definition is a >>> simple mistake, (weird) style decision, or what? >> >> I would define it as a mistake and I think it is worth fixing it. At the >> moment we are only using this uuid for recovery purposes, to discard >> data from a different pblk instance, > > Does this come from outside of the kernel in any mean (user space, > data from device, etc)? > It sounds to me like it does. In this case there is no mistake and we > may not use guid_t there. pblk manages the metadata layout without involvement of the device or user space, so no, no dependency at this moment. It is not pushed anywhere yet, but I have been working on a tool to make a pblk recovery tool to enable FTL repairs if something fails in the kernel recovery path. Here, I use this uuid to identify the instance - is there a way to reconcile guid_t with user space, which currently uses the __u8? Javier
On Thu, Jan 24, 2019 at 4:36 PM Javier González <javier@javigon.com> wrote: > It is not pushed anywhere yet, but I have been working on a tool to make > a pblk recovery tool to enable FTL repairs if something fails in the > kernel recovery path. Here, I use this uuid to identify the > instance - is there a way to reconcile guid_t with user space, which > currently uses the __u8? For Linux there is util-linux which contains libuuid. There is uuid_t type. Unfortunately there is no so called LE (little endian) variant. Perhaps someone would need to extend the support for that.
> On 24 Jan 2019, at 17.38, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jan 24, 2019 at 4:36 PM Javier González <javier@javigon.com> wrote: > >> It is not pushed anywhere yet, but I have been working on a tool to make >> a pblk recovery tool to enable FTL repairs if something fails in the >> kernel recovery path. Here, I use this uuid to identify the >> instance - is there a way to reconcile guid_t with user space, which >> currently uses the __u8? > > For Linux there is util-linux which contains libuuid. There is uuid_t type. > Unfortunately there is no so called LE (little endian) variant. > Perhaps someone would need to extend the support for that. Ok. I can look into that when releasing pblk-tools. But for now, I am OK with applying with the changes Christoph suggested and aligning with guid_t if you also think helps maintaining common helpers. Javier
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h index 85e38ed62f85..0a0aeb6ef314 100644 --- a/drivers/lightnvm/pblk.h +++ b/drivers/lightnvm/pblk.h @@ -1362,10 +1362,7 @@ static inline unsigned int pblk_get_secs(struct bio *bio) static inline void pblk_setup_uuid(struct pblk *pblk) { - uuid_le uuid; - - uuid_le_gen(&uuid); - memcpy(pblk->instance_uuid, uuid.b, 16); + guid_gen((guid_t *)&pblk->instance_uuid); } static inline char *pblk_disk_name(struct pblk *pblk)
There are new types and helpers that are supposed to be used in new code. As a preparation to get rid of legacy types and API functions do the conversion here. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/lightnvm/pblk.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)