[PATCH V2 1/2] fs: include device name in error messages about freezing

[PATCH V2 1/2] fs: include device name in error messages about freezing

[Mateusz Guzik]

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

[PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Mateusz Guzik]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Jan Kara]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Mateusz Guzik]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Jan Kara]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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);
 }
 

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Eric Sandeen]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Lukáš Czerner]

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
> 

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Mateusz Guzik]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Mateusz Guzik]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Mateusz Guzik]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Mateusz Guzik]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Jan Kara]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Jan Kara]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Dave Chinner]

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

Re: [PATCH V2 2/2] fs: print a message when freezing/unfreezing filesystems

[Lukáš Czerner]

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

Re: [PATCH V2 1/2] fs: include device name in error messages about freezing

[Jan Kara]

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

Re: [PATCH V2 1/2] fs: include device name in error messages about freezing

[Dave Chinner]

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