| Message ID | f23e8ec8-b4cc-79d2-95b5-df4821878f91@sandeen.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: Print XFS UUID on mount and umount events. | expand |
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); > > >
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>
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); > > >
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
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:
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.
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
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 >
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
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);