[PATCH v1] : Switch to use new generic UUID API

[PATCH v1] : Switch to use new generic UUID API

[Andy Shevchenko]

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(-)

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)
-- 
2.20.1

Re: [PATCH v1] : Switch to use new generic UUID API

[Christoph Hellwig]

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.

Re: [PATCH v1] : Switch to use new generic UUID API

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] : Switch to use new generic UUID API

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]


> 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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] : Switch to use new generic UUID API

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] : Switch to use new generic UUID API

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]


> 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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] : Switch to use new generic UUID API

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] : Switch to use new generic UUID API

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]


> 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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] : Switch to use new generic UUID API

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] : Switch to use new generic UUID API

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 914 bytes --]


> 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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]