[PATCH] dm: expose dm_copy_name_and_uuid()

[PATCH] dm: expose dm_copy_name_and_uuid()

[John Dorminy]

While dm_device_name() returns the MAJOR:MINOR numbers of a device,
some targets would like to know the pretty name of a device, and
some targets would like to know the uuid expected for the device.

The MAJOR:MINOR name is useful for logging, but printing the long
name of the device can make it easier for the user to correlate
messages with the device names they know in userspace.

The UUID can be useful for devices which store a UUID on disk, and
would like to verify the on-disk UUID matches the UUID known by DM.

dm_copy_name_and_uuid() appears to be the only way to get at the
pretty name and the UUID at present, and this change exports the
function for use by GPLd modules.

Signed-off-by: John Dorminy <jdorminy@redhat.com>
---
 drivers/md/dm-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 1e03bc89e20f..711a46015696 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -2018,7 +2018,7 @@ int dm_copy_name_and_uuid(struct mapped_device *md, char *name, char *uuid)
 
 	return r;
 }
-
+EXPORT_SYMBOL_GPL(dm_copy_name_and_uuid);
 
 /**
  * dm_early_create - create a mapped device in early boot.
-- 
2.20.1

Re: dm: expose dm_copy_name_and_uuid()

[Mike Snitzer]

On Fri, Jan 31 2020 at  7:55pm -0500,
John Dorminy <jdorminy@redhat.com> wrote:

> While dm_device_name() returns the MAJOR:MINOR numbers of a device,
> some targets would like to know the pretty name of a device, and
> some targets would like to know the uuid expected for the device.
> 
> The MAJOR:MINOR name is useful for logging, but printing the long
> name of the device can make it easier for the user to correlate
> messages with the device names they know in userspace.
> 
> The UUID can be useful for devices which store a UUID on disk, and
> would like to verify the on-disk UUID matches the UUID known by DM.
> 
> dm_copy_name_and_uuid() appears to be the only way to get at the
> pretty name and the UUID at present, and this change exports the
> function for use by GPLd modules.
> 
> Signed-off-by: John Dorminy <jdorminy@redhat.com>
> ---
>  drivers/md/dm-ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 1e03bc89e20f..711a46015696 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -2018,7 +2018,7 @@ int dm_copy_name_and_uuid(struct mapped_device *md, char *name, char *uuid)
>  
>  	return r;
>  }
> -
> +EXPORT_SYMBOL_GPL(dm_copy_name_and_uuid);
>  
>  /**
>   * dm_early_create - create a mapped device in early boot.
> -- 
> 2.20.1

These are already available to userspace via sysfs, e.g.:

# dmsetup ls
test-test        (253:0)
# cat /sys/block/dm-0/dm/name
test-test
# cat /sys/block/dm-0/dm/uuid
LVM-IFFqf0id2DX3IgGmu6izzJW1rBoWmsC61hwGobtrD60aCMh6zJwK3uiYUS6rbNoY

Verification of the UUID that a target happens to store in its ondisk
metadata seems like it'd best be done by userspace, _before_ activating
the DM device, no?  Or are you saying that the target would do this
verification as part of its .ctr method?

Anyway, I'm fine with exporting it.. just think the proposed usecases
are not a "slam-dunk" for why you need it.

Re: dm: expose dm_copy_name_and_uuid()

[John Dorminy]

On Mon, Feb 3, 2020 at 11:54 AM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Fri, Jan 31 2020 at  7:55pm -0500,
> John Dorminy <jdorminy@redhat.com> wrote:
>
> > While dm_device_name() returns the MAJOR:MINOR numbers of a device,
> > some targets would like to know the pretty name of a device, and
> > some targets would like to know the uuid expected for the device.
> >
> > The MAJOR:MINOR name is useful for logging, but printing the long
> > name of the device can make it easier for the user to correlate
> > messages with the device names they know in userspace.
> >
> > The UUID can be useful for devices which store a UUID on disk, and
> > would like to verify the on-disk UUID matches the UUID known by DM.
> >
> > dm_copy_name_and_uuid() appears to be the only way to get at the
> > pretty name and the UUID at present, and this change exports the
> > function for use by GPLd modules.
> >
> > Signed-off-by: John Dorminy <jdorminy@redhat.com>
> > ---
> >  drivers/md/dm-ioctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index 1e03bc89e20f..711a46015696 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -2018,7 +2018,7 @@ int dm_copy_name_and_uuid(struct mapped_device *md, char *name, char *uuid)
> >
> >       return r;
> >  }
> > -
> > +EXPORT_SYMBOL_GPL(dm_copy_name_and_uuid);
> >
> >  /**
> >   * dm_early_create - create a mapped device in early boot.
> > --
> > 2.20.1
>
> These are already available to userspace via sysfs, e.g.:
>
> # dmsetup ls
> test-test        (253:0)
> # cat /sys/block/dm-0/dm/name
> test-test
> # cat /sys/block/dm-0/dm/uuid
> LVM-IFFqf0id2DX3IgGmu6izzJW1rBoWmsC61hwGobtrD60aCMh6zJwK3uiYUS6rbNoY
>
> Verification of the UUID that a target happens to store in its ondisk
> metadata seems like it'd best be done by userspace, _before_ activating
> the DM device, no?  Or are you saying that the target would do this
> verification as part of its .ctr method?
>
> Anyway, I'm fine with exporting it.. just think the proposed usecases
> are not a "slam-dunk" for why you need it.
>

To be clear, I care more about getting the name in kernelspace than
about getting the UUID in kernelspace. (253:0) is a unique name for
test-test, and we can get that via dm_device_name() at the moment. But
test-test is often a better name -- it's the name the user provided
dmsetup in the first place, so using it in log messages can help the
user match them to the device that generated them, even after the
device has been shut down and the device number has been lost.

I would tweak dm_device_name() save that at least some of its existing
users do want the numerical name. Besides, dm_copy_name_and_uuid() is
already declared in device-mapper.h, it just isn't exported for use
yet.

With regard to the UUID, I do think that some targets would like to
check it in their .ctr() function. A userspace tool that checks the
UUID before starting the device might as well just read the UUID off
the device and then pass that into dmsetup, thus guaranteeing that
they match. But the target itself might also want to check its UUID in
kernelspace in its constructor, perhaps to warn if someone is using
the target outside of the userspace tool just mentioned and has failed
to specify the right UUID.

If it would be better to alter or tweak dm_device_name(), I'd be happy
to do that instead.

Thanks!

Re: dm: expose dm_copy_name_and_uuid()

[Alasdair G Kergon]

On Thu, Feb 06, 2020 at 07:28:04PM -0500, John Dorminy wrote:
> To be clear, I care more about getting the name in kernelspace than
> about getting the UUID in kernelspace. (253:0) is a unique name for

From the kernel's point of view, it is THE unique and definitive name
for the lifetime of the device.  It's the one thing userspace can rely
upon (without assuming additional 'good' userspace behaviour) to tie
together messages relating to the same device instance.

> test-test, and we can get that via dm_device_name() at the moment. But
> test-test is often a better name -- it's the name the user provided

It can be changed and so processing can't rely on it without trusting
both the userspace management code's self-imposed constraints and the
sysadmin not to have worked around them.

In other words, NEVER report name/uuid without ALSO still reporting
dm_device_name alongside it.

Also take a look at
  https://github.com/lvmteam/storage-logger
which tackles logging all the relevant device information.

Alasdair

Re: dm: expose dm_copy_name_and_uuid()

[Alasdair G Kergon]

On Fri, Feb 07, 2020 at 01:24:33AM +0000, Alasdair G Kergon wrote:
> In other words, NEVER report name/uuid without ALSO still reporting
> dm_device_name alongside it.

The reason we only log dm_device_name() is because it is the minimum 
necessary to uniquely identify the device and tie together all
the messages relating to it.

Adding name/uuid to every message would make log messages unduly long.
We could offer an in-kernel option to log all setting and changing
of device names and uuids in the dm core, though I might argue that that
would just be covering up inadaquate logging in the userspace tools
making the changes.

Storage-logger offers a compromise that records it all from the
generated uevents.

Alasdair

Re: dm: expose dm_copy_name_and_uuid()

[John Dorminy]

I agree that adding uuid to all messages would be gross bloat, and a
bad idea to apply everywhere.

I didn't actually realize that devices could be renamed with dmsetup.
Thanks for pointing that out...

On Thu, Feb 6, 2020 at 8:42 PM Alasdair G Kergon <agk@redhat.com> wrote:
>
> On Fri, Feb 07, 2020 at 01:24:33AM +0000, Alasdair G Kergon wrote:
> > In other words, NEVER report name/uuid without ALSO still reporting
> > dm_device_name alongside it.
>
> The reason we only log dm_device_name() is because it is the minimum
> necessary to uniquely identify the device and tie together all
> the messages relating to it.
>
> Adding name/uuid to every message would make log messages unduly long.
> We could offer an in-kernel option to log all setting and changing
> of device names and uuids in the dm core, though I might argue that that
> would just be covering up inadaquate logging in the userspace tools
> making the changes.
>
> Storage-logger offers a compromise that records it all from the
> generated uevents.
>
> Alasdair
>

Re: dm: expose dm_copy_name_and_uuid()

[Mike Snitzer]

On Thu, Feb 06 2020 at 10:19pm -0500,
John Dorminy <jdorminy@redhat.com> wrote:

> I agree that adding uuid to all messages would be gross bloat, and a
> bad idea to apply everywhere.
> 
> I didn't actually realize that devices could be renamed with dmsetup.
> Thanks for pointing that out...

Also, device names can also be ridiculously long.  I really just don't
think logging the name instead of major:minor is a great idea... but I'm
open to target authors electing to log it _in addition to_
dm_device_name() -- but I wouldn't want to go as far as imposing that
constraint with code...

Or would I? ;)

Create dm_pretty_device_name() that logs: "major:minor (name)" ?

Mike