* [PATCH V2 1/2] fs: include device name in error messages about freezing @ 2014-05-13 22:04 Mateusz Guzik 2014-05-13 22:04 ` [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems Mateusz Guzik 2014-05-14 11:10 ` [PATCH V2 1/2] fs: include device name in error messages about freezing Jan Kara 0 siblings, 2 replies; 23+ messages in thread From: Mateusz Guzik @ 2014-05-13 22:04 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fsdevel, Josef Bacik, Jan Kara, Al Viro, Eric Sandeen While here use pr_err instead of printk(KERN_ERR...) Signed-off-by: Mateusz Guzik <mguzik@redhat.com> Cc: linux-fsdevel@vger.kernel.org Cc: Josef Bacik <jbacik@fb.com> Cc: Jan Kara <jack@suse.cz> Cc: Al Viro <viro@ZenIV.linux.org.uk> Cc: Eric Sandeen <esandeen@redhat.com> --- fs/super.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/super.c b/fs/super.c index 48377f7..017e10a 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1323,8 +1323,7 @@ int freeze_super(struct super_block *sb) if (sb->s_op->freeze_fs) { ret = sb->s_op->freeze_fs(sb); if (ret) { - printk(KERN_ERR - "VFS:Filesystem freeze failed\n"); + pr_err("VFS:Filesystem %s freeze failed\n", sb->s_id); sb->s_writers.frozen = SB_UNFROZEN; smp_wmb(); wake_up(&sb->s_writers.wait_unfrozen); @@ -1364,8 +1363,7 @@ int thaw_super(struct super_block *sb) if (sb->s_op->unfreeze_fs) { error = sb->s_op->unfreeze_fs(sb); if (error) { - printk(KERN_ERR - "VFS:Filesystem thaw failed\n"); + pr_err("VFS:Filesystem %s thaw failed\n", sb->s_id); up_write(&sb->s_umount); return error; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-13 22:04 [PATCH V2 1/2] fs: include device name in error messages about freezing Mateusz Guzik @ 2014-05-13 22:04 ` Mateusz Guzik 2014-05-14 11:14 ` Jan Kara 2014-05-14 11:10 ` [PATCH V2 1/2] fs: include device name in error messages about freezing Jan Kara 1 sibling, 1 reply; 23+ messages in thread From: Mateusz Guzik @ 2014-05-13 22:04 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Josef Bacik, Jan Kara, Al Viro, Eric Sandeen, Joe Perches This helps hang troubleshooting efforts when only dmesg is available. While here remove code duplication with MS_RDONLY case and fix a whitespace nit. Signed-off-by: Mateusz Guzik <mguzik@redhat.com> Cc: linux-fsdevel@vger.kernel.org Cc: Josef Bacik <jbacik@fb.com> Cc: Jan Kara <jack@suse.cz> Cc: Al Viro <viro@ZenIV.linux.org.uk> Cc: Eric Sandeen <esandeen@redhat.com> Cc: Joe Perches <joe@perches.com> --- since v1: fix copy-pasto which found its way into the patch restore curly brackets in MS_RDONLY case fs/super.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/super.c b/fs/super.c index 017e10a..4dd7356 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1291,9 +1291,7 @@ int freeze_super(struct super_block *sb) if (sb->s_flags & MS_RDONLY) { /* Nothing to do really... */ - sb->s_writers.frozen = SB_FREEZE_COMPLETE; - up_write(&sb->s_umount); - return 0; + goto out; } /* From now on, no new normal writers can start */ @@ -1335,8 +1333,10 @@ int freeze_super(struct super_block *sb) * This is just for debugging purposes so that fs can warn if it * sees write activity when frozen is set to SB_FREEZE_COMPLETE. */ +out: sb->s_writers.frozen = SB_FREEZE_COMPLETE; up_write(&sb->s_umount); + pr_info("VFS:Filesystem %s frozen\n", sb->s_id); return 0; } EXPORT_SYMBOL(freeze_super); @@ -1374,7 +1374,7 @@ out: smp_wmb(); wake_up(&sb->s_writers.wait_unfrozen); deactivate_locked_super(sb); - + pr_info("VFS:Filesystem %s thawed\n", sb->s_id); return 0; } EXPORT_SYMBOL(thaw_super); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-13 22:04 ` [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems Mateusz Guzik @ 2014-05-14 11:14 ` Jan Kara 2014-05-14 11:26 ` Mateusz Guzik 2014-05-14 11:58 ` Lukáš Czerner 0 siblings, 2 replies; 23+ messages in thread From: Jan Kara @ 2014-05-14 11:14 UTC (permalink / raw) To: Mateusz Guzik Cc: linux-kernel, linux-fsdevel, Josef Bacik, Jan Kara, Al Viro, Eric Sandeen, Joe Perches On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > This helps hang troubleshooting efforts when only dmesg is available. > > While here remove code duplication with MS_RDONLY case and fix a > whitespace nit. I'm somewhat undecided here I have to say. On one hand I don't like printing to kernel log when everything is fine and kernel is operating normally. On the other hand I've seen quite a few cases where people have shot themselves in the foot with filesystem freezing so having some trace of this in the log doesn't seem like a completely bad thing either. What do other people think? Honza > Signed-off-by: Mateusz Guzik <mguzik@redhat.com> > Cc: linux-fsdevel@vger.kernel.org > Cc: Josef Bacik <jbacik@fb.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Al Viro <viro@ZenIV.linux.org.uk> > Cc: Eric Sandeen <esandeen@redhat.com> > Cc: Joe Perches <joe@perches.com> > --- > since v1: > fix copy-pasto which found its way into the patch > restore curly brackets in MS_RDONLY case > fs/super.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/super.c b/fs/super.c > index 017e10a..4dd7356 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -1291,9 +1291,7 @@ int freeze_super(struct super_block *sb) > > if (sb->s_flags & MS_RDONLY) { > /* Nothing to do really... */ > - sb->s_writers.frozen = SB_FREEZE_COMPLETE; > - up_write(&sb->s_umount); > - return 0; > + goto out; > } > > /* From now on, no new normal writers can start */ > @@ -1335,8 +1333,10 @@ int freeze_super(struct super_block *sb) > * This is just for debugging purposes so that fs can warn if it > * sees write activity when frozen is set to SB_FREEZE_COMPLETE. > */ > +out: > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > up_write(&sb->s_umount); > + pr_info("VFS:Filesystem %s frozen\n", sb->s_id); > return 0; > } > EXPORT_SYMBOL(freeze_super); > @@ -1374,7 +1374,7 @@ out: > smp_wmb(); > wake_up(&sb->s_writers.wait_unfrozen); > deactivate_locked_super(sb); > - > + pr_info("VFS:Filesystem %s thawed\n", sb->s_id); > return 0; > } > EXPORT_SYMBOL(thaw_super); > -- > 1.8.3.1 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 11:14 ` Jan Kara @ 2014-05-14 11:26 ` Mateusz Guzik 2014-05-14 11:39 ` Jan Kara 2014-05-14 11:58 ` Lukáš Czerner 1 sibling, 1 reply; 23+ messages in thread From: Mateusz Guzik @ 2014-05-14 11:26 UTC (permalink / raw) To: Jan Kara Cc: linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > This helps hang troubleshooting efforts when only dmesg is available. > > > > While here remove code duplication with MS_RDONLY case and fix a > > whitespace nit. > I'm somewhat undecided here I have to say. On one hand I don't like > printing to kernel log when everything is fine and kernel is operating > normally. On the other hand I've seen quite a few cases where people have > shot themselves in the foot with filesystem freezing so having some trace > of this in the log doesn't seem like a completely bad thing either. What do > other people think? > I would like to note that the kernel already prints messages when e.g. filesystems get mounted. And yes, situations where I/O is frozen and hung task detector comes into play happen more often than you may think. I can hide these behind a sysctl if this helps. Finally, somewhat related to the first mail, my preferred message format (which I was somewhat reluctant to propose earlier) would be: VFS (%s): Filesystem frozen by %s:%d with device, process and pid respectively. -- Mateusz Guzik ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 11:26 ` Mateusz Guzik @ 2014-05-14 11:39 ` Jan Kara 2014-05-14 22:00 ` Dave Chinner 0 siblings, 1 reply; 23+ messages in thread From: Jan Kara @ 2014-05-14 11:39 UTC (permalink / raw) To: Mateusz Guzik Cc: Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > This helps hang troubleshooting efforts when only dmesg is available. > > > > > > While here remove code duplication with MS_RDONLY case and fix a > > > whitespace nit. > > I'm somewhat undecided here I have to say. On one hand I don't like > > printing to kernel log when everything is fine and kernel is operating > > normally. On the other hand I've seen quite a few cases where people have > > shot themselves in the foot with filesystem freezing so having some trace > > of this in the log doesn't seem like a completely bad thing either. What do > > other people think? > > > > I would like to note that the kernel already prints messages when e.g. > filesystems get mounted. Yeah, that's a fair point. > And yes, situations where I/O is frozen and hung task detector comes into > play happen more often than you may think. I can hide these behind a > sysctl if this helps. No, I don't think hiding behind sysctl is really needed. > Finally, somewhat related to the first mail, my preferred message format > (which I was somewhat reluctant to propose earlier) would be: > > VFS (%s): Filesystem frozen by %s:%d > > with device, process and pid respectively. More common seems to be "%s (%d)" for process & pid but whatever. I like the format change. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 11:39 ` Jan Kara @ 2014-05-14 22:00 ` Dave Chinner 2014-05-14 22:37 ` Dave Chinner 0 siblings, 1 reply; 23+ messages in thread From: Dave Chinner @ 2014-05-14 22:00 UTC (permalink / raw) To: Jan Kara Cc: Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > > This helps hang troubleshooting efforts when only dmesg is available. > > > > > > > > While here remove code duplication with MS_RDONLY case and fix a > > > > whitespace nit. > > > I'm somewhat undecided here I have to say. On one hand I don't like > > > printing to kernel log when everything is fine and kernel is operating > > > normally. On the other hand I've seen quite a few cases where people have > > > shot themselves in the foot with filesystem freezing so having some trace > > > of this in the log doesn't seem like a completely bad thing either. What do > > > other people think? > > > > > > > I would like to note that the kernel already prints messages when e.g. > > filesystems get mounted. > Yeah, that's a fair point. But filesystems choose to output that info, not the VFS. When you do a remount,ro there is no output in syslog, because filesystems don't need to dump any output - the state change is reflected in /proc/self/mounts. IMO frozen should state should be communicated the same way so that it is silent when it just works, and the state can easily be determined when something goes wrong. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 22:00 ` Dave Chinner @ 2014-05-14 22:37 ` Dave Chinner 2014-05-14 22:40 ` Eric Sandeen 2014-05-15 10:13 ` Jan Kara 0 siblings, 2 replies; 23+ messages in thread From: Dave Chinner @ 2014-05-14 22:37 UTC (permalink / raw) To: Jan Kara Cc: Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > > On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > > On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > > > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > > > This helps hang troubleshooting efforts when only dmesg is available. > > > > > > > > > > While here remove code duplication with MS_RDONLY case and fix a > > > > > whitespace nit. > > > > I'm somewhat undecided here I have to say. On one hand I don't like > > > > printing to kernel log when everything is fine and kernel is operating > > > > normally. On the other hand I've seen quite a few cases where people have > > > > shot themselves in the foot with filesystem freezing so having some trace > > > > of this in the log doesn't seem like a completely bad thing either. What do > > > > other people think? > > > > > > > > > > I would like to note that the kernel already prints messages when e.g. > > > filesystems get mounted. > > Yeah, that's a fair point. > > But filesystems choose to output that info, not the VFS. When you do > a remount,ro there is no output in syslog, because filesystems don't > need to dump any output - the state change is reflected in > /proc/self/mounts. IMO frozen should state should be communicated > the same way so that it is silent when it just works, and the state > can easily be determined when something goes wrong. Say, like this: $ grep /mnt/test /proc/mounts /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 $ sudo xfs_freeze -f /mnt/test $ grep /mnt/test /proc/mounts /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 $ sudo xfs_freeze -u /mnt/test $ grep /mnt/test /proc/mounts /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 $ Patch below does this. Cheers, Dave. -- Dave Chinner david@fromorbit.com fs: report frozen state in /proc/mounts From: Dave Chinner <dchinner@redhat.com> So people can tell if a filesystem is frozen easily, add the freezing state to the /proc/mount output for the given superblock. To help diagnose freezing hangs as opposed to frozen filesystems, differentiate between the states of "freezing" and "frozen" in the output. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/proc_namespace.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c index 1a81373..0ec4b56 100644 --- a/fs/proc_namespace.c +++ b/fs/proc_namespace.c @@ -53,6 +53,20 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb) seq_puts(m, fs_infop->str); } + switch (sb->s_writers.frozen) { + case SB_FREEZE_WRITE: + case SB_FREEZE_PAGEFAULT: + case SB_FREEZE_FS: + seq_puts(m, ",freezing"); + break; + case SB_FREEZE_COMPLETE: + seq_puts(m, ",frozen"); + break; + case SB_UNFROZEN: + default: + break; + } + return security_sb_show_options(m, sb); } ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 22:37 ` Dave Chinner @ 2014-05-14 22:40 ` Eric Sandeen 2014-05-15 10:40 ` Lukáš Czerner 2014-05-15 10:13 ` Jan Kara 1 sibling, 1 reply; 23+ messages in thread From: Eric Sandeen @ 2014-05-14 22:40 UTC (permalink / raw) To: Dave Chinner, Jan Kara Cc: Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On 5/14/14, 5:37 PM, Dave Chinner wrote: > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: >> On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: >>> On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: >>>> On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: >>>>> On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: >>>>>> This helps hang troubleshooting efforts when only dmesg is available. >>>>>> >>>>>> While here remove code duplication with MS_RDONLY case and fix a >>>>>> whitespace nit. >>>>> I'm somewhat undecided here I have to say. On one hand I don't like >>>>> printing to kernel log when everything is fine and kernel is operating >>>>> normally. On the other hand I've seen quite a few cases where people have >>>>> shot themselves in the foot with filesystem freezing so having some trace >>>>> of this in the log doesn't seem like a completely bad thing either. What do >>>>> other people think? >>>>> >>>> >>>> I would like to note that the kernel already prints messages when e.g. >>>> filesystems get mounted. >>> Yeah, that's a fair point. >> >> But filesystems choose to output that info, not the VFS. When you do >> a remount,ro there is no output in syslog, because filesystems don't >> need to dump any output - the state change is reflected in >> /proc/self/mounts. IMO frozen should state should be communicated >> the same way so that it is silent when it just works, and the state >> can easily be determined when something goes wrong. > > Say, like this: > > $ grep /mnt/test /proc/mounts > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > $ sudo xfs_freeze -f /mnt/test > $ grep /mnt/test /proc/mounts > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > $ sudo xfs_freeze -u /mnt/test > $ grep /mnt/test /proc/mounts > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > $ I'm not totally convinced that including a non-mount option in what has always (?) been a list of mount options is a great idea. (Granted, some options there are defaults, and weren't actually specified as a mount option, but if they had been, they'd have been accepted). Maybe add a "mount -o remount,frozen" handler ? ;) -Eric ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 22:40 ` Eric Sandeen @ 2014-05-15 10:40 ` Lukáš Czerner 2014-05-15 10:47 ` Mateusz Guzik 0 siblings, 1 reply; 23+ messages in thread From: Lukáš Czerner @ 2014-05-15 10:40 UTC (permalink / raw) To: sandeen Cc: Dave Chinner, Jan Kara, Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Wed, 14 May 2014, Eric Sandeen wrote: > Date: Wed, 14 May 2014 17:40:22 -0500 > From: Eric Sandeen <esandeen@redhat.com> > Reply-To: sandeen@redhat.com > To: Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.cz> > Cc: Mateusz Guzik <mguzik@redhat.com>, linux-kernel@vger.kernel.org, > linux-fsdevel@vger.kernel.org, Josef Bacik <jbacik@fb.com>, > Al Viro <viro@ZenIV.linux.org.uk>, Joe Perches <joe@perches.com> > Subject: Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing > filesystems > > On 5/14/14, 5:37 PM, Dave Chinner wrote: > > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > >> On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > >>> On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > >>>> On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > >>>>> On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > >>>>>> This helps hang troubleshooting efforts when only dmesg is available. > >>>>>> > >>>>>> While here remove code duplication with MS_RDONLY case and fix a > >>>>>> whitespace nit. > >>>>> I'm somewhat undecided here I have to say. On one hand I don't like > >>>>> printing to kernel log when everything is fine and kernel is operating > >>>>> normally. On the other hand I've seen quite a few cases where people have > >>>>> shot themselves in the foot with filesystem freezing so having some trace > >>>>> of this in the log doesn't seem like a completely bad thing either. What do > >>>>> other people think? > >>>>> > >>>> > >>>> I would like to note that the kernel already prints messages when e.g. > >>>> filesystems get mounted. > >>> Yeah, that's a fair point. > >> > >> But filesystems choose to output that info, not the VFS. When you do > >> a remount,ro there is no output in syslog, because filesystems don't > >> need to dump any output - the state change is reflected in > >> /proc/self/mounts. IMO frozen should state should be communicated > >> the same way so that it is silent when it just works, and the state > >> can easily be determined when something goes wrong. > > > > Say, like this: > > > > $ grep /mnt/test /proc/mounts > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > $ sudo xfs_freeze -f /mnt/test > > $ grep /mnt/test /proc/mounts > > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > > $ sudo xfs_freeze -u /mnt/test > > $ grep /mnt/test /proc/mounts > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > $ > > I'm not totally convinced that including a non-mount option in what > has always (?) been a list of mount options is a great idea. I do not like it either. Mixing this together with other mount options does not seem like a great idea, however we really need a way to report this and I guess we can not just change the /proc/self/mounts, or /proc/self/mountinfo format. So what about crating a new file /proc/self/frozen with the list of frozen file systems in the same format what mounts, or mountinfo has ? > > (Granted, some options there are defaults, and weren't actually specified > as a mount option, but if they had been, they'd have been accepted). > > Maybe add a "mount -o remount,frozen" handler ? ;) That's the neat way to work around that :), but I would prefer a new procfs file rather than this. -Lukas > > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 10:40 ` Lukáš Czerner @ 2014-05-15 10:47 ` Mateusz Guzik 2014-05-15 22:21 ` Dave Chinner 0 siblings, 1 reply; 23+ messages in thread From: Mateusz Guzik @ 2014-05-15 10:47 UTC (permalink / raw) To: Lukáš Czerner Cc: sandeen, Dave Chinner, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Thu, May 15, 2014 at 12:40:19PM +0200, Lukáš Czerner wrote: > On Wed, 14 May 2014, Eric Sandeen wrote: > > > Date: Wed, 14 May 2014 17:40:22 -0500 > > From: Eric Sandeen <esandeen@redhat.com> > > Reply-To: sandeen@redhat.com > > To: Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.cz> > > Cc: Mateusz Guzik <mguzik@redhat.com>, linux-kernel@vger.kernel.org, > > linux-fsdevel@vger.kernel.org, Josef Bacik <jbacik@fb.com>, > > Al Viro <viro@ZenIV.linux.org.uk>, Joe Perches <joe@perches.com> > > Subject: Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing > > filesystems > > > > On 5/14/14, 5:37 PM, Dave Chinner wrote: > > > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > > >> On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > > >>> On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > >>>> On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > >>>>> On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > >>>>>> This helps hang troubleshooting efforts when only dmesg is available. > > >>>>>> > > >>>>>> While here remove code duplication with MS_RDONLY case and fix a > > >>>>>> whitespace nit. > > >>>>> I'm somewhat undecided here I have to say. On one hand I don't like > > >>>>> printing to kernel log when everything is fine and kernel is operating > > >>>>> normally. On the other hand I've seen quite a few cases where people have > > >>>>> shot themselves in the foot with filesystem freezing so having some trace > > >>>>> of this in the log doesn't seem like a completely bad thing either. What do > > >>>>> other people think? > > >>>>> > > >>>> > > >>>> I would like to note that the kernel already prints messages when e.g. > > >>>> filesystems get mounted. > > >>> Yeah, that's a fair point. > > >> > > >> But filesystems choose to output that info, not the VFS. When you do > > >> a remount,ro there is no output in syslog, because filesystems don't > > >> need to dump any output - the state change is reflected in > > >> /proc/self/mounts. IMO frozen should state should be communicated > > >> the same way so that it is silent when it just works, and the state > > >> can easily be determined when something goes wrong. > > > > > > Say, like this: > > > > > > $ grep /mnt/test /proc/mounts > > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > > $ sudo xfs_freeze -f /mnt/test > > > $ grep /mnt/test /proc/mounts > > > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > > > $ sudo xfs_freeze -u /mnt/test > > > $ grep /mnt/test /proc/mounts > > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > > $ > > > > I'm not totally convinced that including a non-mount option in what > > has always (?) been a list of mount options is a great idea. > > I do not like it either. Mixing this together with other mount > options does not seem like a great idea, however we really need a > way to report this and I guess we can not just change the > /proc/self/mounts, or /proc/self/mountinfo format. > > So what about crating a new file /proc/self/frozen with the list of > frozen file systems in the same format what mounts, or mountinfo has > ? > As the time goes on there will be more data to present about given mountpoint. So either mountinfo should be extendable to support this (can't see why not) or new generic file with that property should be provided. I vote for the former. IOW, a new column in mountinfo. For frozen filesystems it would contain 'frozen_by=[%s]:[%d]' (escaped comm, pid). -- Mateusz Guzik ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 10:47 ` Mateusz Guzik @ 2014-05-15 22:21 ` Dave Chinner 2014-05-15 22:34 ` Mateusz Guzik 0 siblings, 1 reply; 23+ messages in thread From: Dave Chinner @ 2014-05-15 22:21 UTC (permalink / raw) To: Mateusz Guzik Cc: Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Thu, May 15, 2014 at 12:47:48PM +0200, Mateusz Guzik wrote: > On Thu, May 15, 2014 at 12:40:19PM +0200, Lukáš Czerner wrote: > > On Wed, 14 May 2014, Eric Sandeen wrote: > > > > > Date: Wed, 14 May 2014 17:40:22 -0500 > > > From: Eric Sandeen <esandeen@redhat.com> > > > Reply-To: sandeen@redhat.com > > > To: Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.cz> > > > Cc: Mateusz Guzik <mguzik@redhat.com>, linux-kernel@vger.kernel.org, > > > linux-fsdevel@vger.kernel.org, Josef Bacik <jbacik@fb.com>, > > > Al Viro <viro@ZenIV.linux.org.uk>, Joe Perches <joe@perches.com> > > > Subject: Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing > > > filesystems > > > > > > On 5/14/14, 5:37 PM, Dave Chinner wrote: > > > > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > > > >> On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > > > >>> On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > > >>>> On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > > >>>>> On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > >>>>>> This helps hang troubleshooting efforts when only dmesg is available. > > > >>>>>> > > > >>>>>> While here remove code duplication with MS_RDONLY case and fix a > > > >>>>>> whitespace nit. > > > >>>>> I'm somewhat undecided here I have to say. On one hand I don't like > > > >>>>> printing to kernel log when everything is fine and kernel is operating > > > >>>>> normally. On the other hand I've seen quite a few cases where people have > > > >>>>> shot themselves in the foot with filesystem freezing so having some trace > > > >>>>> of this in the log doesn't seem like a completely bad thing either. What do > > > >>>>> other people think? > > > >>>>> > > > >>>> > > > >>>> I would like to note that the kernel already prints messages when e.g. > > > >>>> filesystems get mounted. > > > >>> Yeah, that's a fair point. > > > >> > > > >> But filesystems choose to output that info, not the VFS. When you do > > > >> a remount,ro there is no output in syslog, because filesystems don't > > > >> need to dump any output - the state change is reflected in > > > >> /proc/self/mounts. IMO frozen should state should be communicated > > > >> the same way so that it is silent when it just works, and the state > > > >> can easily be determined when something goes wrong. > > > > > > > > Say, like this: > > > > > > > > $ grep /mnt/test /proc/mounts > > > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > > > $ sudo xfs_freeze -f /mnt/test > > > > $ grep /mnt/test /proc/mounts > > > > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > > > > $ sudo xfs_freeze -u /mnt/test > > > > $ grep /mnt/test /proc/mounts > > > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > > > $ > > > > > > I'm not totally convinced that including a non-mount option in what > > > has always (?) been a list of mount options is a great idea. > > > > I do not like it either. Mixing this together with other mount > > options does not seem like a great idea, however we really need a > > way to report this and I guess we can not just change the > > /proc/self/mounts, or /proc/self/mountinfo format. > > > > So what about crating a new file /proc/self/frozen with the list of > > frozen file systems in the same format what mounts, or mountinfo has > > ? > > > > As the time goes on there will be more data to present about given > mountpoint. So either mountinfo should be extendable to support this > (can't see why not) or new generic file with that property should be > provided. I vote for the former. > > IOW, a new column in mountinfo. For frozen filesystems it would contain > 'frozen_by=[%s]:[%d]' (escaped comm, pid). I really don't see that the process that froze the filesystem is particularly useful - it many cases that process is long gone (e.g. fsfreeze is being used to allow a HW array to take a snapshot). Just the fact it is in the process of freezing (if stuck, stack trace in sysrq-w should be present) or frozen (freezing process may be long gone, and is mostly irrelevant because you're now tracking down why a thaw hasn't happened)... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 22:21 ` Dave Chinner @ 2014-05-15 22:34 ` Mateusz Guzik 2014-05-15 22:51 ` Dave Chinner 0 siblings, 1 reply; 23+ messages in thread From: Mateusz Guzik @ 2014-05-15 22:34 UTC (permalink / raw) To: Dave Chinner Cc: Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Fri, May 16, 2014 at 08:21:35AM +1000, Dave Chinner wrote: > On Thu, May 15, 2014 at 12:47:48PM +0200, Mateusz Guzik wrote: > > On Thu, May 15, 2014 at 12:40:19PM +0200, Lukáš Czerner wrote: > > > On Wed, 14 May 2014, Eric Sandeen wrote: > > > > > > > Date: Wed, 14 May 2014 17:40:22 -0500 > > > > From: Eric Sandeen <esandeen@redhat.com> > > > > Reply-To: sandeen@redhat.com > > > > To: Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.cz> > > > > Cc: Mateusz Guzik <mguzik@redhat.com>, linux-kernel@vger.kernel.org, > > > > linux-fsdevel@vger.kernel.org, Josef Bacik <jbacik@fb.com>, > > > > Al Viro <viro@ZenIV.linux.org.uk>, Joe Perches <joe@perches.com> > > > > Subject: Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing > > > > filesystems > > > > > > > > On 5/14/14, 5:37 PM, Dave Chinner wrote: > > > > > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > > > > >> On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > > > > >>> On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > > > >>>> On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > > > >>>>> On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > > >>>>>> This helps hang troubleshooting efforts when only dmesg is available. > > > > >>>>>> > > > > >>>>>> While here remove code duplication with MS_RDONLY case and fix a > > > > >>>>>> whitespace nit. > > > > >>>>> I'm somewhat undecided here I have to say. On one hand I don't like > > > > >>>>> printing to kernel log when everything is fine and kernel is operating > > > > >>>>> normally. On the other hand I've seen quite a few cases where people have > > > > >>>>> shot themselves in the foot with filesystem freezing so having some trace > > > > >>>>> of this in the log doesn't seem like a completely bad thing either. What do > > > > >>>>> other people think? > > > > >>>>> > > > > >>>> > > > > >>>> I would like to note that the kernel already prints messages when e.g. > > > > >>>> filesystems get mounted. > > > > >>> Yeah, that's a fair point. > > > > >> > > > > >> But filesystems choose to output that info, not the VFS. When you do > > > > >> a remount,ro there is no output in syslog, because filesystems don't > > > > >> need to dump any output - the state change is reflected in > > > > >> /proc/self/mounts. IMO frozen should state should be communicated > > > > >> the same way so that it is silent when it just works, and the state > > > > >> can easily be determined when something goes wrong. > > > > > > > > > > Say, like this: > > > > > > > > > > $ grep /mnt/test /proc/mounts > > > > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > > > > $ sudo xfs_freeze -f /mnt/test > > > > > $ grep /mnt/test /proc/mounts > > > > > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > > > > > $ sudo xfs_freeze -u /mnt/test > > > > > $ grep /mnt/test /proc/mounts > > > > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > > > > $ > > > > > > > > I'm not totally convinced that including a non-mount option in what > > > > has always (?) been a list of mount options is a great idea. > > > > > > I do not like it either. Mixing this together with other mount > > > options does not seem like a great idea, however we really need a > > > way to report this and I guess we can not just change the > > > /proc/self/mounts, or /proc/self/mountinfo format. > > > > > > So what about crating a new file /proc/self/frozen with the list of > > > frozen file systems in the same format what mounts, or mountinfo has > > > ? > > > > > > > As the time goes on there will be more data to present about given > > mountpoint. So either mountinfo should be extendable to support this > > (can't see why not) or new generic file with that property should be > > provided. I vote for the former. > > > > IOW, a new column in mountinfo. For frozen filesystems it would contain > > 'frozen_by=[%s]:[%d]' (escaped comm, pid). > > I really don't see that the process that froze the filesystem is > particularly useful - it many cases that process is long gone (e.g. > fsfreeze is being used to allow a HW array to take a snapshot). Just > the fact it is in the process of freezing (if stuck, stack trace in > sysrq-w should be present) or frozen (freezing process may be long > gone, and is mostly irrelevant because you're now tracking down why > a thaw hasn't happened)... > There are deamons which perform freezing and unfreezing on their own. Thus storing the name along with pid helps to determine whether someone went behind such daemon's back, or maybe it's the daemon which "forgot" to unfreeze after all. -- Mateusz Guzik ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 22:34 ` Mateusz Guzik @ 2014-05-15 22:51 ` Dave Chinner 2014-05-15 23:19 ` Mateusz Guzik 0 siblings, 1 reply; 23+ messages in thread From: Dave Chinner @ 2014-05-15 22:51 UTC (permalink / raw) To: Mateusz Guzik Cc: Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Fri, May 16, 2014 at 12:34:40AM +0200, Mateusz Guzik wrote: > On Fri, May 16, 2014 at 08:21:35AM +1000, Dave Chinner wrote: > > > IOW, a new column in mountinfo. For frozen filesystems it would contain > > > 'frozen_by=[%s]:[%d]' (escaped comm, pid). > > > > I really don't see that the process that froze the filesystem is > > particularly useful - it many cases that process is long gone (e.g. > > fsfreeze is being used to allow a HW array to take a snapshot). Just > > the fact it is in the process of freezing (if stuck, stack trace in > > sysrq-w should be present) or frozen (freezing process may be long > > gone, and is mostly irrelevant because you're now tracking down why > > a thaw hasn't happened)... > > There are deamons which perform freezing and unfreezing on their own. > Thus storing the name along with pid helps to determine whether someone > went behind such daemon's back, or maybe it's the daemon which "forgot" to > unfreeze after all. Such a daemon should be logging the fact that it's freezing and thawing the filesystem. The kernel is not the place to track what buggy userspace applications are doing wrong. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 22:51 ` Dave Chinner @ 2014-05-15 23:19 ` Mateusz Guzik 2014-05-16 0:11 ` Dave Chinner 0 siblings, 1 reply; 23+ messages in thread From: Mateusz Guzik @ 2014-05-15 23:19 UTC (permalink / raw) To: Dave Chinner Cc: Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Fri, May 16, 2014 at 08:51:41AM +1000, Dave Chinner wrote: > On Fri, May 16, 2014 at 12:34:40AM +0200, Mateusz Guzik wrote: > > On Fri, May 16, 2014 at 08:21:35AM +1000, Dave Chinner wrote: > > > > IOW, a new column in mountinfo. For frozen filesystems it would contain > > > > 'frozen_by=[%s]:[%d]' (escaped comm, pid). > > > > > > I really don't see that the process that froze the filesystem is > > > particularly useful - it many cases that process is long gone (e.g. > > > fsfreeze is being used to allow a HW array to take a snapshot). Just > > > the fact it is in the process of freezing (if stuck, stack trace in > > > sysrq-w should be present) or frozen (freezing process may be long > > > gone, and is mostly irrelevant because you're now tracking down why > > > a thaw hasn't happened)... > > > > There are deamons which perform freezing and unfreezing on their own. > > Thus storing the name along with pid helps to determine whether someone > > went behind such daemon's back, or maybe it's the daemon which "forgot" to > > unfreeze after all. > > Such a daemon should be logging the fact that it's freezing and > thawing the filesystem. The kernel is not the place to track what > buggy userspace applications are doing wrong. > Except there is no log entry if /var got frozen (and this is not an imaginary example). Grabbig a debugger to inspect daemon's state is not exactly what your typical support associate can or should do. But this was a side request, I'm not going to argue about including this since turns out there is a better way. Somewhere in the thread an idea to log long-standing freezes was mentioned which would provide sufficient information as far as troubleshooting this stuff is concerned. Thanks and sorry :-> -- Mateusz Guzik ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 23:19 ` Mateusz Guzik @ 2014-05-16 0:11 ` Dave Chinner 2014-05-16 0:39 ` Mateusz Guzik 2014-05-19 9:43 ` Jan Kara 0 siblings, 2 replies; 23+ messages in thread From: Dave Chinner @ 2014-05-16 0:11 UTC (permalink / raw) To: Mateusz Guzik Cc: Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Fri, May 16, 2014 at 01:19:09AM +0200, Mateusz Guzik wrote: > On Fri, May 16, 2014 at 08:51:41AM +1000, Dave Chinner wrote: > > On Fri, May 16, 2014 at 12:34:40AM +0200, Mateusz Guzik wrote: > > > On Fri, May 16, 2014 at 08:21:35AM +1000, Dave Chinner wrote: > > > > > IOW, a new column in mountinfo. For frozen filesystems it would contain > > > > > 'frozen_by=[%s]:[%d]' (escaped comm, pid). > > > > > > > > I really don't see that the process that froze the filesystem is > > > > particularly useful - it many cases that process is long gone (e.g. > > > > fsfreeze is being used to allow a HW array to take a snapshot). Just > > > > the fact it is in the process of freezing (if stuck, stack trace in > > > > sysrq-w should be present) or frozen (freezing process may be long > > > > gone, and is mostly irrelevant because you're now tracking down why > > > > a thaw hasn't happened)... > > > > > > There are deamons which perform freezing and unfreezing on their own. > > > Thus storing the name along with pid helps to determine whether someone > > > went behind such daemon's back, or maybe it's the daemon which "forgot" to > > > unfreeze after all. > > > > Such a daemon should be logging the fact that it's freezing and > > thawing the filesystem. The kernel is not the place to track what > > buggy userspace applications are doing wrong. > > > > Except there is no log entry if /var got frozen (and this is not an > imaginary example). Freezing the filesystem that the freezing daemon logs to is, well, a major application architecture fail. Sorry, catering for the lowest common denominator (i.e. stupidity) is not an valid argument for adding stuff to the kernel.... > Grabbig a debugger to inspect daemon's state is not > exactly what your typical support associate can or should do. No, but they can read /proc/self/mountinfo, and grab sysrq-w output. And they should be able to read that and tell that there is a freeze hang from that info. This "filesystem hang triage 101" stuff.... > But this was a side request, I'm not going to argue about including > this since turns out there is a better way. > > Somewhere in the thread an idea to log long-standing freezes was > mentioned which would provide sufficient information as far as You've already got the hung task timer firing when a fs is frozen for too long. You'll see processes hung in sb_write_wait(), and that tells you the filesystem is frozen. Then look at /proc/self/mountinfo to find which fs is frozen.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-16 0:11 ` Dave Chinner @ 2014-05-16 0:39 ` Mateusz Guzik 2014-05-19 9:43 ` Jan Kara 1 sibling, 0 replies; 23+ messages in thread From: Mateusz Guzik @ 2014-05-16 0:39 UTC (permalink / raw) To: Dave Chinner Cc: Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Fri, May 16, 2014 at 10:11:56AM +1000, Dave Chinner wrote: > On Fri, May 16, 2014 at 01:19:09AM +0200, Mateusz Guzik wrote: > > Except there is no log entry if /var got frozen (and this is not an > > imaginary example). > > Freezing the filesystem that the freezing daemon logs to is, well, a > major application architecture fail. Sorry, catering for the lowest > common denominator (i.e. stupidity) is not an valid argument for > adding stuff to the kernel.... > I'm only saying what you can encounter in varous companies. If aiding this problem the way I proposed is not a good idea (and it turns out there is a much better way), I'm not insisting. > > Grabbig a debugger to inspect daemon's state is not > > exactly what your typical support associate can or should do. > > No, but they can read /proc/self/mountinfo, and grab sysrq-w output. > And they should be able to read that and tell that there is a freeze > hang from that info. This "filesystem hang triage 101" stuff.... > > > But this was a side request, I'm not going to argue about including > > this since turns out there is a better way. > > > > Somewhere in the thread an idea to log long-standing freezes was > > mentioned which would provide sufficient information as far as > > You've already got the hung task timer firing when a fs is frozen > for too long. You'll see processes hung in sb_write_wait(), and that > tells you the filesystem is frozen. Then look at > /proc/self/mountinfo to find which fs is frozen.... > But additional question was what initiated the freeze and it is not answered by this. Hopefully a warning for long-standing freezes will be implemented and that will answer the question. Once more, I'm fine with mere 'frozen' in mountinfo, so I suggest we drop this now side subject. If you really want to continue we can discuss this in private. :-> -- Mateusz Guzik ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-16 0:11 ` Dave Chinner 2014-05-16 0:39 ` Mateusz Guzik @ 2014-05-19 9:43 ` Jan Kara 2014-05-19 23:37 ` Dave Chinner 1 sibling, 1 reply; 23+ messages in thread From: Jan Kara @ 2014-05-19 9:43 UTC (permalink / raw) To: Dave Chinner Cc: Mateusz Guzik, Lukáš Czerner, sandeen, Jan Kara, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Fri 16-05-14 10:11:56, Dave Chinner wrote: > On Fri, May 16, 2014 at 01:19:09AM +0200, Mateusz Guzik wrote: > > On Fri, May 16, 2014 at 08:51:41AM +1000, Dave Chinner wrote: > > > On Fri, May 16, 2014 at 12:34:40AM +0200, Mateusz Guzik wrote: > > > > On Fri, May 16, 2014 at 08:21:35AM +1000, Dave Chinner wrote: > > > > > > IOW, a new column in mountinfo. For frozen filesystems it would contain > > > > > > 'frozen_by=[%s]:[%d]' (escaped comm, pid). > > > > > > > > > > I really don't see that the process that froze the filesystem is > > > > > particularly useful - it many cases that process is long gone (e.g. > > > > > fsfreeze is being used to allow a HW array to take a snapshot). Just > > > > > the fact it is in the process of freezing (if stuck, stack trace in > > > > > sysrq-w should be present) or frozen (freezing process may be long > > > > > gone, and is mostly irrelevant because you're now tracking down why > > > > > a thaw hasn't happened)... > > > > > > > > There are deamons which perform freezing and unfreezing on their own. > > > > Thus storing the name along with pid helps to determine whether someone > > > > went behind such daemon's back, or maybe it's the daemon which "forgot" to > > > > unfreeze after all. > > > > > > Such a daemon should be logging the fact that it's freezing and > > > thawing the filesystem. The kernel is not the place to track what > > > buggy userspace applications are doing wrong. > > > > > > > Except there is no log entry if /var got frozen (and this is not an > > imaginary example). > > Freezing the filesystem that the freezing daemon logs to is, well, a > major application architecture fail. Sorry, catering for the lowest > common denominator (i.e. stupidity) is not an valid argument for > adding stuff to the kernel.... Sure it's not a good architecture but it happens either because of a bug or a wrong architecture. So you need to debug it and traces from sysrq-w don't tell you who froze the filesystem. Currently you have to use tracepoints or similar stuff to find that out (e.g. in one case I was debugging it was rpm running a post-install script that froze the fs, believe me that was really unexpected :)). But tracepoints aren't useful after the fact so sometimes it would be useful to be able to find out after the fact who froze the fs (PID and command name to help with situations when the process isn't running anymore). Since this is mostly debug stuff I'd be OK with dumping this information on sysrq request or as Ted suggested from some fs-freeze hang check timer... Hmm? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-19 9:43 ` Jan Kara @ 2014-05-19 23:37 ` Dave Chinner 0 siblings, 0 replies; 23+ messages in thread From: Dave Chinner @ 2014-05-19 23:37 UTC (permalink / raw) To: Jan Kara Cc: Mateusz Guzik, Lukáš Czerner, sandeen, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Joe Perches On Mon, May 19, 2014 at 11:43:13AM +0200, Jan Kara wrote: > On Fri 16-05-14 10:11:56, Dave Chinner wrote: > > On Fri, May 16, 2014 at 01:19:09AM +0200, Mateusz Guzik wrote: > > > On Fri, May 16, 2014 at 08:51:41AM +1000, Dave Chinner wrote: > > > > On Fri, May 16, 2014 at 12:34:40AM +0200, Mateusz Guzik wrote: > > > > > On Fri, May 16, 2014 at 08:21:35AM +1000, Dave Chinner wrote: > > > > > > > IOW, a new column in mountinfo. For frozen filesystems it would contain > > > > > > > 'frozen_by=[%s]:[%d]' (escaped comm, pid). > > > > > > > > > > > > I really don't see that the process that froze the filesystem is > > > > > > particularly useful - it many cases that process is long gone (e.g. > > > > > > fsfreeze is being used to allow a HW array to take a snapshot). Just > > > > > > the fact it is in the process of freezing (if stuck, stack trace in > > > > > > sysrq-w should be present) or frozen (freezing process may be long > > > > > > gone, and is mostly irrelevant because you're now tracking down why > > > > > > a thaw hasn't happened)... > > > > > > > > > > There are deamons which perform freezing and unfreezing on their own. > > > > > Thus storing the name along with pid helps to determine whether someone > > > > > went behind such daemon's back, or maybe it's the daemon which "forgot" to > > > > > unfreeze after all. > > > > > > > > Such a daemon should be logging the fact that it's freezing and > > > > thawing the filesystem. The kernel is not the place to track what > > > > buggy userspace applications are doing wrong. > > > > > > > > > > Except there is no log entry if /var got frozen (and this is not an > > > imaginary example). > > > > Freezing the filesystem that the freezing daemon logs to is, well, a > > major application architecture fail. Sorry, catering for the lowest > > common denominator (i.e. stupidity) is not an valid argument for > > adding stuff to the kernel.... > Sure it's not a good architecture but it happens either because of a bug > or a wrong architecture. So you need to debug it and traces from sysrq-w > don't tell you who froze the filesystem. Currently you have to use > tracepoints or similar stuff to find that out (e.g. in one case I was > debugging it was rpm running a post-install script that froze the fs, > believe me that was really unexpected :)). But tracepoints aren't useful > after the fact so sometimes it would be useful to be able to find out > after the fact who froze the fs (PID and command name to help with > situations when the process isn't running anymore). Since this is mostly > debug stuff I'd be OK with dumping this information on sysrq request or as > Ted suggested from some fs-freeze hang check timer... Hmm? Sure, along with all the memory allocation, copying and freeing stuff that isn't guaranteed to be around if a hang occurs. And it has to be per-sb, because you can indepenently freeze multiple filesystems at the same time, and so on. I just don't see the complexity as being worthwhile given how rare freeze problems are... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 22:37 ` Dave Chinner 2014-05-14 22:40 ` Eric Sandeen @ 2014-05-15 10:13 ` Jan Kara 2014-05-15 22:16 ` Dave Chinner 1 sibling, 1 reply; 23+ messages in thread From: Jan Kara @ 2014-05-15 10:13 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Thu 15-05-14 08:37:45, Dave Chinner wrote: > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > > On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > > > On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > > > On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > > > > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > > > > This helps hang troubleshooting efforts when only dmesg is available. > > > > > > > > > > > > While here remove code duplication with MS_RDONLY case and fix a > > > > > > whitespace nit. > > > > > I'm somewhat undecided here I have to say. On one hand I don't like > > > > > printing to kernel log when everything is fine and kernel is operating > > > > > normally. On the other hand I've seen quite a few cases where people have > > > > > shot themselves in the foot with filesystem freezing so having some trace > > > > > of this in the log doesn't seem like a completely bad thing either. What do > > > > > other people think? > > > > > > > > > > > > > I would like to note that the kernel already prints messages when e.g. > > > > filesystems get mounted. > > > Yeah, that's a fair point. > > > > But filesystems choose to output that info, not the VFS. When you do > > a remount,ro there is no output in syslog, because filesystems don't > > need to dump any output - the state change is reflected in > > /proc/self/mounts. IMO frozen should state should be communicated > > the same way so that it is silent when it just works, and the state > > can easily be determined when something goes wrong. > > Say, like this: > > $ grep /mnt/test /proc/mounts > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > $ sudo xfs_freeze -f /mnt/test > $ grep /mnt/test /proc/mounts > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > $ sudo xfs_freeze -u /mnt/test > $ grep /mnt/test /proc/mounts > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > $ > > Patch below does this. Hum, same as Eric I'm not very enthusiastic about a fake mount option in /proc/mounts. Maybe we could stuff some extra field in /proc/self/mountinfo? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-15 10:13 ` Jan Kara @ 2014-05-15 22:16 ` Dave Chinner 0 siblings, 0 replies; 23+ messages in thread From: Dave Chinner @ 2014-05-15 22:16 UTC (permalink / raw) To: Jan Kara Cc: Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Thu, May 15, 2014 at 12:13:56PM +0200, Jan Kara wrote: > On Thu 15-05-14 08:37:45, Dave Chinner wrote: > > On Thu, May 15, 2014 at 08:00:52AM +1000, Dave Chinner wrote: > > > On Wed, May 14, 2014 at 01:39:45PM +0200, Jan Kara wrote: > > > > On Wed 14-05-14 13:26:21, Mateusz Guzik wrote: > > > > > On Wed, May 14, 2014 at 01:14:49PM +0200, Jan Kara wrote: > > > > > > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > > > > > > This helps hang troubleshooting efforts when only dmesg is available. > > > > > > > > > > > > > > While here remove code duplication with MS_RDONLY case and fix a > > > > > > > whitespace nit. > > > > > > I'm somewhat undecided here I have to say. On one hand I don't like > > > > > > printing to kernel log when everything is fine and kernel is operating > > > > > > normally. On the other hand I've seen quite a few cases where people have > > > > > > shot themselves in the foot with filesystem freezing so having some trace > > > > > > of this in the log doesn't seem like a completely bad thing either. What do > > > > > > other people think? > > > > > > > > > > > > > > > > I would like to note that the kernel already prints messages when e.g. > > > > > filesystems get mounted. > > > > Yeah, that's a fair point. > > > > > > But filesystems choose to output that info, not the VFS. When you do > > > a remount,ro there is no output in syslog, because filesystems don't > > > need to dump any output - the state change is reflected in > > > /proc/self/mounts. IMO frozen should state should be communicated > > > the same way so that it is silent when it just works, and the state > > > can easily be determined when something goes wrong. > > > > Say, like this: > > > > $ grep /mnt/test /proc/mounts > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > $ sudo xfs_freeze -f /mnt/test > > $ grep /mnt/test /proc/mounts > > /dev/vda /mnt/test xfs rw,frozen,relatime,attr2,inode64,noquota 0 0 > > $ sudo xfs_freeze -u /mnt/test > > $ grep /mnt/test /proc/mounts > > /dev/vda /mnt/test xfs rw,relatime,attr2,inode64,noquota 0 0 > > $ > > > > Patch below does this. > Hum, same as Eric I'm not very enthusiastic about a fake mount option in > /proc/mounts. Maybe we could stuff some extra field in /proc/self/mountinfo? sure - I'd forgotten about /proc/self/mountinfo, but the way i wrote the patch will also dump the information into it. It's just a case of moving the code to show_mountinfo() rather than show_sb_opts(). If that's acceptible for everyone, then I'll update the patch to do that.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems 2014-05-14 11:14 ` Jan Kara 2014-05-14 11:26 ` Mateusz Guzik @ 2014-05-14 11:58 ` Lukáš Czerner 1 sibling, 0 replies; 23+ messages in thread From: Lukáš Czerner @ 2014-05-14 11:58 UTC (permalink / raw) To: Jan Kara Cc: Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen, Joe Perches On Wed, 14 May 2014, Jan Kara wrote: > Date: Wed, 14 May 2014 13:14:49 +0200 > From: Jan Kara <jack@suse.cz> > To: Mateusz Guzik <mguzik@redhat.com> > Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, > Josef Bacik <jbacik@fb.com>, Jan Kara <jack@suse.cz>, > Al Viro <viro@ZenIV.linux.org.uk>, Eric Sandeen <esandeen@redhat.com>, > Joe Perches <joe@perches.com> > Subject: Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing > filesystems > > On Wed 14-05-14 00:04:43, Mateusz Guzik wrote: > > This helps hang troubleshooting efforts when only dmesg is available. > > > > While here remove code duplication with MS_RDONLY case and fix a > > whitespace nit. > I'm somewhat undecided here I have to say. On one hand I don't like > printing to kernel log when everything is fine and kernel is operating > normally. On the other hand I've seen quite a few cases where people have > shot themselves in the foot with filesystem freezing so having some trace > of this in the log doesn't seem like a completely bad thing either. What do > other people think? I think that this message is definitely useful to have. As Mateusz already mentioned I think that this is of the same importance as the "file system mounted/remounted" message. -Lukas > > Honza > > Signed-off-by: Mateusz Guzik <mguzik@redhat.com> > > Cc: linux-fsdevel@vger.kernel.org > > Cc: Josef Bacik <jbacik@fb.com> > > Cc: Jan Kara <jack@suse.cz> > > Cc: Al Viro <viro@ZenIV.linux.org.uk> > > Cc: Eric Sandeen <esandeen@redhat.com> > > Cc: Joe Perches <joe@perches.com> > > --- > > since v1: > > fix copy-pasto which found its way into the patch > > restore curly brackets in MS_RDONLY case > > fs/super.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/fs/super.c b/fs/super.c > > index 017e10a..4dd7356 100644 > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -1291,9 +1291,7 @@ int freeze_super(struct super_block *sb) > > > > if (sb->s_flags & MS_RDONLY) { > > /* Nothing to do really... */ > > - sb->s_writers.frozen = SB_FREEZE_COMPLETE; > > - up_write(&sb->s_umount); > > - return 0; > > + goto out; > > } > > > > /* From now on, no new normal writers can start */ > > @@ -1335,8 +1333,10 @@ int freeze_super(struct super_block *sb) > > * This is just for debugging purposes so that fs can warn if it > > * sees write activity when frozen is set to SB_FREEZE_COMPLETE. > > */ > > +out: > > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > > up_write(&sb->s_umount); > > + pr_info("VFS:Filesystem %s frozen\n", sb->s_id); > > return 0; > > } > > EXPORT_SYMBOL(freeze_super); > > @@ -1374,7 +1374,7 @@ out: > > smp_wmb(); > > wake_up(&sb->s_writers.wait_unfrozen); > > deactivate_locked_super(sb); > > - > > + pr_info("VFS:Filesystem %s thawed\n", sb->s_id); > > return 0; > > } > > EXPORT_SYMBOL(thaw_super); > > -- > > 1.8.3.1 > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 1/2] fs: include device name in error messages about freezing 2014-05-13 22:04 [PATCH V2 1/2] fs: include device name in error messages about freezing Mateusz Guzik 2014-05-13 22:04 ` [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems Mateusz Guzik @ 2014-05-14 11:10 ` Jan Kara 2014-05-14 22:07 ` Dave Chinner 1 sibling, 1 reply; 23+ messages in thread From: Jan Kara @ 2014-05-14 11:10 UTC (permalink / raw) To: Mateusz Guzik Cc: linux-kernel, linux-fsdevel, Josef Bacik, Jan Kara, Al Viro, Eric Sandeen On Wed 14-05-14 00:04:42, Mateusz Guzik wrote: > While here use pr_err instead of printk(KERN_ERR...) The patch looks good. Just one nit below: > Signed-off-by: Mateusz Guzik <mguzik@redhat.com> > Cc: linux-fsdevel@vger.kernel.org > Cc: Josef Bacik <jbacik@fb.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Al Viro <viro@ZenIV.linux.org.uk> > Cc: Eric Sandeen <esandeen@redhat.com> > --- > fs/super.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/super.c b/fs/super.c > index 48377f7..017e10a 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -1323,8 +1323,7 @@ int freeze_super(struct super_block *sb) > if (sb->s_op->freeze_fs) { > ret = sb->s_op->freeze_fs(sb); > if (ret) { > - printk(KERN_ERR > - "VFS:Filesystem freeze failed\n"); > + pr_err("VFS:Filesystem %s freeze failed\n", sb->s_id); I'd somewhat prefer to have the format like: VFS (device %s): Filesystem freeze failed\n That should easy to programatically parse if necessary and also more consistent with how filesystems report errors. Now VFS itself doesn't have any unified style but still... Honza > sb->s_writers.frozen = SB_UNFROZEN; > smp_wmb(); > wake_up(&sb->s_writers.wait_unfrozen); > @@ -1364,8 +1363,7 @@ int thaw_super(struct super_block *sb) > if (sb->s_op->unfreeze_fs) { > error = sb->s_op->unfreeze_fs(sb); > if (error) { > - printk(KERN_ERR > - "VFS:Filesystem thaw failed\n"); > + pr_err("VFS:Filesystem %s thaw failed\n", sb->s_id); > up_write(&sb->s_umount); > return error; > } > -- > 1.8.3.1 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V2 1/2] fs: include device name in error messages about freezing 2014-05-14 11:10 ` [PATCH V2 1/2] fs: include device name in error messages about freezing Jan Kara @ 2014-05-14 22:07 ` Dave Chinner 0 siblings, 0 replies; 23+ messages in thread From: Dave Chinner @ 2014-05-14 22:07 UTC (permalink / raw) To: Jan Kara Cc: Mateusz Guzik, linux-kernel, linux-fsdevel, Josef Bacik, Al Viro, Eric Sandeen On Wed, May 14, 2014 at 01:10:48PM +0200, Jan Kara wrote: > On Wed 14-05-14 00:04:42, Mateusz Guzik wrote: > > While here use pr_err instead of printk(KERN_ERR...) > The patch looks good. Just one nit below: > > > Signed-off-by: Mateusz Guzik <mguzik@redhat.com> > > Cc: linux-fsdevel@vger.kernel.org > > Cc: Josef Bacik <jbacik@fb.com> > > Cc: Jan Kara <jack@suse.cz> > > Cc: Al Viro <viro@ZenIV.linux.org.uk> > > Cc: Eric Sandeen <esandeen@redhat.com> > > --- > > fs/super.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/fs/super.c b/fs/super.c > > index 48377f7..017e10a 100644 > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -1323,8 +1323,7 @@ int freeze_super(struct super_block *sb) > > if (sb->s_op->freeze_fs) { > > ret = sb->s_op->freeze_fs(sb); > > if (ret) { > > - printk(KERN_ERR > > - "VFS:Filesystem freeze failed\n"); > > + pr_err("VFS:Filesystem %s freeze failed\n", sb->s_id); > I'd somewhat prefer to have the format like: > VFS (device %s): Filesystem freeze failed\n > > That should easy to programatically parse if necessary and also more > consistent with how filesystems report errors. Now VFS itself doesn't have > any unified style but still... I'd suggest that filesystems should be outputting a failure message if something goes wrong during a freeze. As you mention, they already have their own device tailored log messages, but they also have a much better idea of what the failure actually is. And some filesystems will have already outputted an error if something went wrong during the freeze.... Further, the freeze failure errno is is sent back to the caller (i.e. userspace), so the user should already be getting a message saying the freeze failed (just like for remount,ro). So why do we need dmesg output for something that already has a working error reporting path? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-05-19 23:37 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-13 22:04 [PATCH V2 1/2] fs: include device name in error messages about freezing Mateusz Guzik 2014-05-13 22:04 ` [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems Mateusz Guzik 2014-05-14 11:14 ` Jan Kara 2014-05-14 11:26 ` Mateusz Guzik 2014-05-14 11:39 ` Jan Kara 2014-05-14 22:00 ` Dave Chinner 2014-05-14 22:37 ` Dave Chinner 2014-05-14 22:40 ` Eric Sandeen 2014-05-15 10:40 ` Lukáš Czerner 2014-05-15 10:47 ` Mateusz Guzik 2014-05-15 22:21 ` Dave Chinner 2014-05-15 22:34 ` Mateusz Guzik 2014-05-15 22:51 ` Dave Chinner 2014-05-15 23:19 ` Mateusz Guzik 2014-05-16 0:11 ` Dave Chinner 2014-05-16 0:39 ` Mateusz Guzik 2014-05-19 9:43 ` Jan Kara 2014-05-19 23:37 ` Dave Chinner 2014-05-15 10:13 ` Jan Kara 2014-05-15 22:16 ` Dave Chinner 2014-05-14 11:58 ` Lukáš Czerner 2014-05-14 11:10 ` [PATCH V2 1/2] fs: include device name in error messages about freezing Jan Kara 2014-05-14 22:07 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).