[PATCH] xfs: Print XFS UUID on mount and umount events.

[PATCH] xfs: Print XFS UUID on mount and umount events.

[Eric Sandeen]

From: Lukas Herbolt <lukas@herbolt.com>

As of now only device names are printed out over __xfs_printk().
The device names are not persistent across reboots which in case
of searching for origin of corruption brings another task to properly
identify the devices. This patch add XFS UUID upon every mount/umount
event which will make the identification much easier.

Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
[sandeen: rebase onto current upstream kernel]
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

[resending this as it seems to have gotten lost, and looks to me like 
a trivial and useful enhancement to xfs logmessages. This was requested
(and authored!) by our support folks.]

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f02a0dd522b3..0141d9907d31 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -644,12 +644,14 @@ xfs_log_mount(
 	int		min_logfsbs;
 
 	if (!xfs_has_norecovery(mp)) {
-		xfs_notice(mp, "Mounting V%d Filesystem",
-			   XFS_SB_VERSION_NUM(&mp->m_sb));
+		xfs_notice(mp, "Mounting V%d Filesystem %pU",
+			   XFS_SB_VERSION_NUM(&mp->m_sb),
+			   &mp->m_sb.sb_uuid);
 	} else {
 		xfs_notice(mp,
-"Mounting V%d filesystem in no-recovery mode. Filesystem will be inconsistent.",
-			   XFS_SB_VERSION_NUM(&mp->m_sb));
+"Mounting V%d filesystem %pU in no-recovery mode. Filesystem will be inconsistent.",
+			   XFS_SB_VERSION_NUM(&mp->m_sb),
+			   &mp->m_sb.sb_uuid);
 		ASSERT(xfs_is_readonly(mp));
 	}
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f029c6702dda..0ed477df6480 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1110,7 +1110,7 @@ xfs_fs_put_super(
 	if (!sb->s_fs_info)
 		return;
 
-	xfs_notice(mp, "Unmounting Filesystem");
+	xfs_notice(mp, "Unmounting Filesystem %pU", &mp->m_sb.sb_uuid);
 	xfs_filestream_unmount(mp);
 	xfs_unmountfs(mp);
 

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Darrick J. Wong]

On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> From: Lukas Herbolt <lukas@herbolt.com>
> 
> As of now only device names are printed out over __xfs_printk().
> The device names are not persistent across reboots which in case
> of searching for origin of corruption brings another task to properly
> identify the devices. This patch add XFS UUID upon every mount/umount
> event which will make the identification much easier.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> [sandeen: rebase onto current upstream kernel]
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

LGTM
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> 
> [resending this as it seems to have gotten lost, and looks to me like 
> a trivial and useful enhancement to xfs logmessages. This was requested
> (and authored!) by our support folks.]
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index f02a0dd522b3..0141d9907d31 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -644,12 +644,14 @@ xfs_log_mount(
>  	int		min_logfsbs;
>  
>  	if (!xfs_has_norecovery(mp)) {
> -		xfs_notice(mp, "Mounting V%d Filesystem",
> -			   XFS_SB_VERSION_NUM(&mp->m_sb));
> +		xfs_notice(mp, "Mounting V%d Filesystem %pU",
> +			   XFS_SB_VERSION_NUM(&mp->m_sb),
> +			   &mp->m_sb.sb_uuid);
>  	} else {
>  		xfs_notice(mp,
> -"Mounting V%d filesystem in no-recovery mode. Filesystem will be inconsistent.",
> -			   XFS_SB_VERSION_NUM(&mp->m_sb));
> +"Mounting V%d filesystem %pU in no-recovery mode. Filesystem will be inconsistent.",
> +			   XFS_SB_VERSION_NUM(&mp->m_sb),
> +			   &mp->m_sb.sb_uuid);
>  		ASSERT(xfs_is_readonly(mp));
>  	}
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f029c6702dda..0ed477df6480 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1110,7 +1110,7 @@ xfs_fs_put_super(
>  	if (!sb->s_fs_info)
>  		return;
>  
> -	xfs_notice(mp, "Unmounting Filesystem");
> +	xfs_notice(mp, "Unmounting Filesystem %pU", &mp->m_sb.sb_uuid);
>  	xfs_filestream_unmount(mp);
>  	xfs_unmountfs(mp);
>  
> 
> 

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Dave Chinner]

On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> From: Lukas Herbolt <lukas@herbolt.com>
> 
> As of now only device names are printed out over __xfs_printk().
> The device names are not persistent across reboots which in case
> of searching for origin of corruption brings another task to properly
> identify the devices. This patch add XFS UUID upon every mount/umount
> event which will make the identification much easier.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> [sandeen: rebase onto current upstream kernel]
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Seems harmless.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Lukas Czerner]

On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> From: Lukas Herbolt <lukas@herbolt.com>
> 
> As of now only device names are printed out over __xfs_printk().
> The device names are not persistent across reboots which in case
> of searching for origin of corruption brings another task to properly
> identify the devices. This patch add XFS UUID upon every mount/umount
> event which will make the identification much easier.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> [sandeen: rebase onto current upstream kernel]
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Hi,

it is a simple enough, nonintrusive change so it may not really matter as
much, but I was wondering if there is a way to map the device name to
the fs UUID already and I think there may be.

I know that udev daemon is constantly scanning devices then they are
closed in order to be able to read the signatures. It should know
exactly what is on the device and I know it is able to track the history
of changes. What I am not sure about is whether it is already logged
somewhere?

If it's not already, maybe it can be done and then we can cross
reference kernel log with udev log when tracking down problems to see
exactly what is going on without needing to sprinkle UUIDs in kernel log ?

Any thoughts?

-Lukas

> ---
> 
> [resending this as it seems to have gotten lost, and looks to me like
> a trivial and useful enhancement to xfs logmessages. This was requested
> (and authored!) by our support folks.]
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index f02a0dd522b3..0141d9907d31 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -644,12 +644,14 @@ xfs_log_mount(
>  	int		min_logfsbs;
> 
>  	if (!xfs_has_norecovery(mp)) {
> -		xfs_notice(mp, "Mounting V%d Filesystem",
> -			   XFS_SB_VERSION_NUM(&mp->m_sb));
> +		xfs_notice(mp, "Mounting V%d Filesystem %pU",
> +			   XFS_SB_VERSION_NUM(&mp->m_sb),
> +			   &mp->m_sb.sb_uuid);
>  	} else {
>  		xfs_notice(mp,
> -"Mounting V%d filesystem in no-recovery mode. Filesystem will be inconsistent.",
> -			   XFS_SB_VERSION_NUM(&mp->m_sb));
> +"Mounting V%d filesystem %pU in no-recovery mode. Filesystem will be inconsistent.",
> +			   XFS_SB_VERSION_NUM(&mp->m_sb),
> +			   &mp->m_sb.sb_uuid);
>  		ASSERT(xfs_is_readonly(mp));
>  	}
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f029c6702dda..0ed477df6480 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1110,7 +1110,7 @@ xfs_fs_put_super(
>  	if (!sb->s_fs_info)
>  		return;
> 
> -	xfs_notice(mp, "Unmounting Filesystem");
> +	xfs_notice(mp, "Unmounting Filesystem %pU", &mp->m_sb.sb_uuid);
>  	xfs_filestream_unmount(mp);
>  	xfs_unmountfs(mp);
> 
> 
> 

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Eric Sandeen]

On 11/3/22 8:32 AM, Lukas Czerner wrote:
> On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
>> From: Lukas Herbolt <lukas@herbolt.com>
>>
>> As of now only device names are printed out over __xfs_printk().
>> The device names are not persistent across reboots which in case
>> of searching for origin of corruption brings another task to properly
>> identify the devices. This patch add XFS UUID upon every mount/umount
>> event which will make the identification much easier.
>>
>> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
>> [sandeen: rebase onto current upstream kernel]
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Hi,
> 
> it is a simple enough, nonintrusive change so it may not really matter as
> much, but I was wondering if there is a way to map the device name to
> the fs UUID already and I think there may be.
> 
> I know that udev daemon is constantly scanning devices then they are
> closed in order to be able to read the signatures. It should know
> exactly what is on the device and I know it is able to track the history
> of changes. What I am not sure about is whether it is already logged
> somewhere?
> 
> If it's not already, maybe it can be done and then we can cross
> reference kernel log with udev log when tracking down problems to see
> exactly what is going on without needing to sprinkle UUIDs in kernel log ?
> 
> Any thoughts?

Hm, I'm not that familiar with udev or what it logs, so I can't really say
offhand. If you know for sure that this mapping is possible to achieve in
other ways, that may be useful.

I guess I'm still of the opinion that having the device::uuid mapping clearly
stated at mount time in the kernel logs is useful, though; AFAIK there is no
real "cost" to this, and other subsystems already print UUIDs for various
reasons so it's not an unusual thing to do.

(I'd have hesitated to add yet another printk for this purpose, but extending
an existing printk seems completely harmless...)

-Eric

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Lukas Czerner]

On Thu, Nov 03, 2022 at 09:57:20AM -0500, Eric Sandeen wrote:
> On 11/3/22 8:32 AM, Lukas Czerner wrote:
> > On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> >> From: Lukas Herbolt <lukas@herbolt.com>
> >>
> >> As of now only device names are printed out over __xfs_printk().
> >> The device names are not persistent across reboots which in case
> >> of searching for origin of corruption brings another task to properly
> >> identify the devices. This patch add XFS UUID upon every mount/umount
> >> event which will make the identification much easier.
> >>
> >> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> >> [sandeen: rebase onto current upstream kernel]
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > Hi,
> > 
> > it is a simple enough, nonintrusive change so it may not really matter as
> > much, but I was wondering if there is a way to map the device name to
> > the fs UUID already and I think there may be.
> > 
> > I know that udev daemon is constantly scanning devices then they are
> > closed in order to be able to read the signatures. It should know
> > exactly what is on the device and I know it is able to track the history
> > of changes. What I am not sure about is whether it is already logged
> > somewhere?
> > 
> > If it's not already, maybe it can be done and then we can cross
> > reference kernel log with udev log when tracking down problems to see
> > exactly what is going on without needing to sprinkle UUIDs in kernel log ?
> > 
> > Any thoughts?
> 
> Hm, I'm not that familiar with udev or what it logs, so I can't really say
> offhand. If you know for sure that this mapping is possible to achieve in
> other ways, that may be useful.
> 
> I guess I'm still of the opinion that having the device::uuid mapping clearly
> stated at mount time in the kernel logs is useful, though; AFAIK there is no
> real "cost" to this, and other subsystems already print UUIDs for various
> reasons so it's not an unusual thing to do.
> 
> (I'd have hesitated to add yet another printk for this purpose, but extending
> an existing printk seems completely harmless...)
> 
> -Eric

I have no strong opinion either way, just trying to explore avenues we
may already have.

When I do mkfs.xfs -f /dev/vdb this is what I get from:

udevadm monitor --environment


monitor will print the received events for:
UDEV - the event which udev sends out after rule processing
KERNEL - the kernel uevent

KERNEL[187.052212] change   /devices/pci0000:00/0000:00:01.6/0000:07:00.0/virtio5/block/vdb (block)
ACTION=change
DEVPATH=/devices/pci0000:00/0000:00:01.6/0000:07:00.0/virtio5/block/vdb
SUBSYSTEM=block
SYNTH_UUID=0
DEVNAME=/dev/vdb
DEVTYPE=disk
DISKSEQ=2
SEQNUM=3494
MAJOR=252
MINOR=16

UDEV  [187.069279] change   /devices/pci0000:00/0000:00:01.6/0000:07:00.0/virtio5/block/vdb (block)
ACTION=change
DEVPATH=/devices/pci0000:00/0000:00:01.6/0000:07:00.0/virtio5/block/vdb
SUBSYSTEM=block
SYNTH_UUID=0
DEVNAME=/dev/vdb
DEVTYPE=disk
DISKSEQ=2
SEQNUM=3494
USEC_INITIALIZED=4425234
ID_PATH=pci-0000:07:00.0
ID_PATH_TAG=pci-0000_07_00_0
ID_FS_UUID=61c0b0c8-2f9e-4fbf-817d-eda65b0363d5
ID_FS_UUID_ENC=61c0b0c8-2f9e-4fbf-817d-eda65b0363d5
ID_FS_TYPE=xfs
ID_FS_USAGE=filesystem
.ID_FS_TYPE_NEW=xfs
MAJOR=252
MINOR=16
DEVLINKS=/dev/disk/by-path/pci-0000:07:00.0 /dev/disk/by-path/virtio-pci-0000:07:00.0 /dev/disk/by-uuid/61c0b0c8-2f9e-4fbf-817d-eda65b0363d5
TAGS=:systemd:
CURRENT_TAGS=:systemd:

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Dave Chinner]

On Thu, Nov 03, 2022 at 02:32:52PM +0100, Lukas Czerner wrote:
> On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> > From: Lukas Herbolt <lukas@herbolt.com>
> > 
> > As of now only device names are printed out over __xfs_printk().
> > The device names are not persistent across reboots which in case
> > of searching for origin of corruption brings another task to properly
> > identify the devices. This patch add XFS UUID upon every mount/umount
> > event which will make the identification much easier.
> > 
> > Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> > [sandeen: rebase onto current upstream kernel]
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Hi,
> 
> it is a simple enough, nonintrusive change so it may not really matter as
> much, but I was wondering if there is a way to map the device name to
> the fs UUID already and I think there may be.
> 
> I know that udev daemon is constantly scanning devices then they are
> closed in order to be able to read the signatures. It should know
> exactly what is on the device and I know it is able to track the history
> of changes. What I am not sure about is whether it is already logged
> somewhere?
> 
> If it's not already, maybe it can be done and then we can cross
> reference kernel log with udev log when tracking down problems to see
> exactly what is going on without needing to sprinkle UUIDs in kernel log ?
> 
> Any thoughts?

Don't like it. Emitting the UUID on the fs mount/unmount log message
is a trivial change that has zero impact on anything as well as
being really easy for log scrapers to deal with.

Screwing around with udev to manage and/or find this correlationi
is ... unnecssarily awful.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Darrick J. Wong]

On Fri, Nov 04, 2022 at 07:51:07AM +1100, Dave Chinner wrote:
> On Thu, Nov 03, 2022 at 02:32:52PM +0100, Lukas Czerner wrote:
> > On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> > > From: Lukas Herbolt <lukas@herbolt.com>
> > > 
> > > As of now only device names are printed out over __xfs_printk().
> > > The device names are not persistent across reboots which in case
> > > of searching for origin of corruption brings another task to properly
> > > identify the devices. This patch add XFS UUID upon every mount/umount
> > > event which will make the identification much easier.
> > > 
> > > Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> > > [sandeen: rebase onto current upstream kernel]
> > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > Hi,
> > 
> > it is a simple enough, nonintrusive change so it may not really matter as
> > much, but I was wondering if there is a way to map the device name to
> > the fs UUID already and I think there may be.
> > 
> > I know that udev daemon is constantly scanning devices then they are
> > closed in order to be able to read the signatures. It should know
> > exactly what is on the device and I know it is able to track the history
> > of changes. What I am not sure about is whether it is already logged
> > somewhere?
> > 
> > If it's not already, maybe it can be done and then we can cross
> > reference kernel log with udev log when tracking down problems to see
> > exactly what is going on without needing to sprinkle UUIDs in kernel log ?
> > 
> > Any thoughts?
> 
> Don't like it. Emitting the UUID on the fs mount/unmount log message
> is a trivial change that has zero impact on anything as well as
> being really easy for log scrapers to deal with.
> 
> Screwing around with udev to manage and/or find this correlationi
> is ... unnecssarily awful.

/me wonders what problem is needing to be solved here -- if support is
having difficulty mapping fs uuids to block devices for $purpose, then
why not capture the blkid output in the sosreport and go from there?

That said, I thought logging the super device name ("sda1") and the fs
uuid in dmesg was sufficient to accomplish that task?

/me shrugs, doesn't understand, wanders off to more code refactoring...

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Lukas Czerner]

On Fri, Nov 04, 2022 at 07:51:07AM +1100, Dave Chinner wrote:
> On Thu, Nov 03, 2022 at 02:32:52PM +0100, Lukas Czerner wrote:
> > On Tue, Nov 01, 2022 at 12:19:06PM -0500, Eric Sandeen wrote:
> > > From: Lukas Herbolt <lukas@herbolt.com>
> > > 
> > > As of now only device names are printed out over __xfs_printk().
> > > The device names are not persistent across reboots which in case
> > > of searching for origin of corruption brings another task to properly
> > > identify the devices. This patch add XFS UUID upon every mount/umount
> > > event which will make the identification much easier.
> > > 
> > > Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> > > [sandeen: rebase onto current upstream kernel]
> > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > Hi,
> > 
> > it is a simple enough, nonintrusive change so it may not really matter as
> > much, but I was wondering if there is a way to map the device name to
> > the fs UUID already and I think there may be.
> > 
> > I know that udev daemon is constantly scanning devices then they are
> > closed in order to be able to read the signatures. It should know
> > exactly what is on the device and I know it is able to track the history
> > of changes. What I am not sure about is whether it is already logged
> > somewhere?
> > 
> > If it's not already, maybe it can be done and then we can cross
> > reference kernel log with udev log when tracking down problems to see
> > exactly what is going on without needing to sprinkle UUIDs in kernel log ?
> > 
> > Any thoughts?
> 
> Don't like it. Emitting the UUID on the fs mount/unmount log message
> is a trivial change that has zero impact on anything as well as
> being really easy for log scrapers to deal with.
> 
> Screwing around with udev to manage and/or find this correlationi
> is ... unnecssarily awful.
> 
> -Dave.

I agree that it's easy to do. But the reason I wanted to at least have
the discussion about an alternate way of doing this is that I remember a
discussion at Plumbers, or LFS/MM where people were opposed to putting
more and more UUID and other ugly identifiers into kernel log. My memory
is hazy now, but either you or Christoph may have been one of those
people.

But if the consensus shifted since then, I am fine with that.

Thanks!
-Lukas

> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] xfs: Print XFS UUID on mount and umount events.

[Eric Sandeen]

On 11/3/22 5:42 PM, Darrick J. Wong wrote:

...

> /me wonders what problem is needing to be solved here -- if support is
> having difficulty mapping fs uuids to block devices for $purpose, then
> why not capture the blkid output in the sosreport and go from there?

Because the sosreport is frequently a post-mortem, and device name to uuid
mapping may have changed by that time.

> That said, I thought logging the super device name ("sda1") and the fs
> uuid in dmesg was sufficient to accomplish that task?

I think it is.

-Eric