[PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Alun]

Hi,

I've recently been looking into a problem on a heavily loaded fileserver.
Running fsfreeze on the filesystem causes the load average to go sky high.
Looking at the kernel sources, I discovered that the FIFREEZE ioctl suspends
writes, then syncs the filesystem. With a large amount of dirty data, this
can leave writes suspended for a significant time, even if your intent is to
have the filesystem frozen for a short time. I raised this on the kernel
list, suggesting an extra call to sync the filesystem prior to suspending
writes, but it was pointed out to me that this would change the semantics
of the ioctl.

Adding a call to syncfs() prior to freezing the filesystem will achieve
the same result, so I'm proposing the following patch which adds a "-s"
option to fsfreeze which, when specified, will cause it to call syncfs
on the filesystem. You can use it by itself to cause a filesystem sync,
or in combination with -f to sync, then freeze the filesystem.

Signed-off-by: Alun Jones <alun.linux@ty-penguin.org.uk>

diff --git a/sys-utils/fsfreeze.8 b/sys-utils/fsfreeze.8
index 7693861..0d73afe 100644
--- a/sys-utils/fsfreeze.8
+++ b/sys-utils/fsfreeze.8
@@ -1,5 +1,5 @@
 .\" -*- nroff -*-
-.TH FSFREEZE 8 "May 2010" "util-linux" "System Administration"
+.TH FSFREEZE 8 "December 2012" "util-linux" "System Administration"
 .SH NAME
 fsfreeze \- suspend access to a filesystem (Linux Ext3/4, ReiserFS, JFS, XFS)
 .SH SYNOPSIS
@@ -9,6 +9,9 @@ fsfreeze \- suspend access to a filesystem (Linux Ext3/4, ReiserFS, JFS, XFS)
 .B fsfreeze \-u
 .I mountpoint
 
+.B fsfreeze \-s
+.I mountpoint
+
 .SH DESCRIPTION
 .B fsfreeze
 suspends and resumes access to an filesystem
@@ -53,6 +56,14 @@ or a clean mount of the snapshot is complete.
 This option is used to un-freeze the filesystem and allow operations to
 continue.  Any filesystem modifications that were blocked by the freeze are
 unblocked and allowed to complete.
+.IP "\fB\-s, \-\-sync\fP
+This option, if specified, will cause fsfreeze to call syncfs() on the 
+filesystem. It can be combined with -f to force dirty data to be written prior
+to the freeze operation. On a filesystem with heavy write load, adding this
+option will very likely reduce the time during which writes are suspended,
+at the cost of a delay before suspending writes. Therefore, if it's critical
+to reduce the time for which the filesystem is frozen, use this option; if 
+it's critical to freeze the filesystem at a specific moment, don't use it.
 .SH AUTHOR
 .PP
 Written by Hajime Taira.
diff --git a/sys-utils/fsfreeze.c b/sys-utils/fsfreeze.c
index 7161442..5a473fd 100644
--- a/sys-utils/fsfreeze.c
+++ b/sys-utils/fsfreeze.c
@@ -44,6 +44,7 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 
 	fputs(_("\nOptions:\n"), out);
 	fputs(_(" -h, --help        this help\n"
+		" -s, --sync        sync the filesystem\n"
 		" -f, --freeze      freeze the filesystem\n"
 		" -u, --unfreeze    unfreeze the filesystem\n"), out);
 
@@ -56,6 +57,7 @@ int main(int argc, char **argv)
 {
 	int fd = -1, c;
 	int freeze = -1, rc = EXIT_FAILURE;
+	int dosync = -1;
 	char *path;
 	struct stat sb;
 
@@ -63,6 +65,7 @@ int main(int argc, char **argv)
 	    { "help",      0, 0, 'h' },
 	    { "freeze",    0, 0, 'f' },
 	    { "unfreeze",  0, 0, 'u' },
+	    { "sync",      0, 0, 's' },
 	    { NULL,        0, 0, 0 }
 	};
 
@@ -71,7 +74,7 @@ int main(int argc, char **argv)
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((c = getopt_long(argc, argv, "hfu", longopts, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "hfus", longopts, NULL)) != -1) {
 		switch(c) {
 		case 'h':
 			usage(stdout);
@@ -82,13 +85,16 @@ int main(int argc, char **argv)
 		case 'u':
 			freeze = FALSE;
 			break;
+		case 's':
+			dosync = TRUE;
+			break;
 		default:
 			usage(stderr);
 			break;
 		}
 	}
 
-	if (freeze == -1)
+	if (freeze == -1 && dosync == -1)
 		errx(EXIT_FAILURE, _("no action specified"));
 	if (optind == argc)
 		errx(EXIT_FAILURE, _("no filename specified"));
@@ -113,12 +119,19 @@ int main(int argc, char **argv)
 		goto done;
 	}
 
-	if (freeze) {
+	if (dosync) {
+		if (syncfs(fd)) {
+			warn(_("%s: syncfs failed"), path);
+			goto done;
+		}
+	}
+
+	if (freeze == TRUE) {
 		if (freeze_f(fd)) {
 			warn(_("%s: freeze failed"), path);
 			goto done;
 		}
-	} else {
+	} else if (freeze == FALSE) {
 		if (unfreeze_f(fd)) {
 			warn(_("%s: unfreeze failed"), path);
 			goto done;

Re: [PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Alun]

Dear all,

Very sorry - I made a small mistake in the previous patch, which
resulted in syncfs() always being called rather than only when -s
was specified.

Signed-off-by: Alun Jones <alun.linux@ty-penguin.org.uk>

diff --git a/sys-utils/fsfreeze.8 b/sys-utils/fsfreeze.8
index 7693861..0d73afe 100644
--- a/sys-utils/fsfreeze.8
+++ b/sys-utils/fsfreeze.8
@@ -1,5 +1,5 @@
 .\" -*- nroff -*-
-.TH FSFREEZE 8 "May 2010" "util-linux" "System Administration"
+.TH FSFREEZE 8 "December 2012" "util-linux" "System Administration"
 .SH NAME
 fsfreeze \- suspend access to a filesystem (Linux Ext3/4, ReiserFS, JFS, XFS)
 .SH SYNOPSIS
@@ -9,6 +9,9 @@ fsfreeze \- suspend access to a filesystem (Linux Ext3/4, ReiserFS, JFS, XFS)
 .B fsfreeze \-u
 .I mountpoint
 
+.B fsfreeze \-s
+.I mountpoint
+
 .SH DESCRIPTION
 .B fsfreeze
 suspends and resumes access to an filesystem
@@ -53,6 +56,14 @@ or a clean mount of the snapshot is complete.
 This option is used to un-freeze the filesystem and allow operations to
 continue.  Any filesystem modifications that were blocked by the freeze are
 unblocked and allowed to complete.
+.IP "\fB\-s, \-\-sync\fP
+This option, if specified, will cause fsfreeze to call syncfs() on the 
+filesystem. It can be combined with -f to force dirty data to be written prior
+to the freeze operation. On a filesystem with heavy write load, adding this
+option will very likely reduce the time during which writes are suspended,
+at the cost of a delay before suspending writes. Therefore, if it's critical
+to reduce the time for which the filesystem is frozen, use this option; if 
+it's critical to freeze the filesystem at a specific moment, don't use it.
 .SH AUTHOR
 .PP
 Written by Hajime Taira.
diff --git a/sys-utils/fsfreeze.c b/sys-utils/fsfreeze.c
index 7161442..6e8fc82 100644
--- a/sys-utils/fsfreeze.c
+++ b/sys-utils/fsfreeze.c
@@ -44,6 +44,7 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 
 	fputs(_("\nOptions:\n"), out);
 	fputs(_(" -h, --help        this help\n"
+		" -s, --sync        sync the filesystem\n"
 		" -f, --freeze      freeze the filesystem\n"
 		" -u, --unfreeze    unfreeze the filesystem\n"), out);
 
@@ -56,6 +57,7 @@ int main(int argc, char **argv)
 {
 	int fd = -1, c;
 	int freeze = -1, rc = EXIT_FAILURE;
+	int dosync = -1;
 	char *path;
 	struct stat sb;
 
@@ -63,6 +65,7 @@ int main(int argc, char **argv)
 	    { "help",      0, 0, 'h' },
 	    { "freeze",    0, 0, 'f' },
 	    { "unfreeze",  0, 0, 'u' },
+	    { "sync",      0, 0, 's' },
 	    { NULL,        0, 0, 0 }
 	};
 
@@ -71,7 +74,7 @@ int main(int argc, char **argv)
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((c = getopt_long(argc, argv, "hfu", longopts, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "hfus", longopts, NULL)) != -1) {
 		switch(c) {
 		case 'h':
 			usage(stdout);
@@ -82,13 +85,16 @@ int main(int argc, char **argv)
 		case 'u':
 			freeze = FALSE;
 			break;
+		case 's':
+			dosync = TRUE;
+			break;
 		default:
 			usage(stderr);
 			break;
 		}
 	}
 
-	if (freeze == -1)
+	if (freeze == -1 && dosync == -1)
 		errx(EXIT_FAILURE, _("no action specified"));
 	if (optind == argc)
 		errx(EXIT_FAILURE, _("no filename specified"));
@@ -113,12 +119,19 @@ int main(int argc, char **argv)
 		goto done;
 	}
 
-	if (freeze) {
+	if (dosync == TRUE) {
+		if (syncfs(fd)) {
+			warn(_("%s: syncfs failed"), path);
+			goto done;
+		}
+	}
+
+	if (freeze == TRUE) {
 		if (freeze_f(fd)) {
 			warn(_("%s: freeze failed"), path);
 			goto done;
 		}
-	} else {
+	} else if (freeze == FALSE) {
 		if (unfreeze_f(fd)) {
 			warn(_("%s: unfreeze failed"), path);
 			goto done;

Re: [PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Christoph Hellwig]

On Fri, Dec 07, 2012 at 10:14:00AM +0000, Alun wrote:
> Hi,
> 
> I've recently been looking into a problem on a heavily loaded fileserver.
> Running fsfreeze on the filesystem causes the load average to go sky high.
> Looking at the kernel sources, I discovered that the FIFREEZE ioctl suspends
> writes, then syncs the filesystem. With a large amount of dirty data, this
> can leave writes suspended for a significant time, even if your intent is to
> have the filesystem frozen for a short time. I raised this on the kernel
> list, suggesting an extra call to sync the filesystem prior to suspending
> writes, but it was pointed out to me that this would change the semantics
> of the ioctl.

It might be better to send a kernel patch to do a first async sync
attempt instead of band aiding this in one of the consumers.

Re: [PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Alun]

Christoph Hellwig <hch@infradead.org> said, in message
20121207124415.GA28504@infradead.org:
> 
> It might be better to send a kernel patch to do a first async sync
> attempt instead of band aiding this in one of the consumers.

That's what I did originally - see http://marc.info/?t=135474654500003&r=1&w=2

Cheers,
Alun.

-- 
Alun Jones,    auj@aber.ac.uk,    01970 622494
Gwasanaethau Gwybodaeth / Information Services  
Prifysgol Aberystwyth / Aberystwyth University

Re: [PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Christoph Hellwig]

On Fri, Dec 07, 2012 at 12:50:32PM +0000, Alun wrote:
> Christoph Hellwig <hch@infradead.org> said, in message
> 20121207124415.GA28504@infradead.org:
> > 
> > It might be better to send a kernel patch to do a first async sync
> > attempt instead of band aiding this in one of the consumers.
> 
> That's what I did originally - see http://marc.info/?t=135474654500003&r=1&w=2

So let's keep the discussion there, if you do exactly the same call from
userspace the same arguments still apply.

Re: [PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Alun]

On Sat, 8 Dec 2012 07:47:43 -0500
Christoph Hellwig <hch@infradead.org> wrote:

> On Fri, Dec 07, 2012 at 12:50:32PM +0000, Alun wrote:
> > Christoph Hellwig <hch@infradead.org> said, in message
> > 20121207124415.GA28504@infradead.org:
> > > 
> > > It might be better to send a kernel patch to do a first async sync
> > > attempt instead of band aiding this in one of the consumers.
> > 
> > That's what I did originally - see
> > http://marc.info/?t=135474654500003&r=1&w=2
> 
> So let's keep the discussion there, if you do exactly the same call
> from userspace the same arguments still apply.

I'm out of my depth when it comes to the politics of all this. So I
think I'm going to bow out now. I'd already got the solution to my
specific issue (write a tiny "syncfs" program and call it from my
script prior to taking a snapshot). While I'd like to try and help
others avoid the same pitfall as me, I'm not motivated enough to
blunder any further into this disagreement.

Cheers,
Alun.

Re: [PATCH] Add option to fsfreeze to call syncfs() prior to freezing.

[Karel Zak]

On Sat, Dec 08, 2012 at 07:46:41PM +0000, Alun wrote:
> On Sat, 8 Dec 2012 07:47:43 -0500
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Fri, Dec 07, 2012 at 12:50:32PM +0000, Alun wrote:
> > > Christoph Hellwig <hch@infradead.org> said, in message
> > > 20121207124415.GA28504@infradead.org:
> > > > 
> > > > It might be better to send a kernel patch to do a first async sync
> > > > attempt instead of band aiding this in one of the consumers.
> > > 
> > > That's what I did originally - see
> > > http://marc.info/?t=135474654500003&r=1&w=2
> > 
> > So let's keep the discussion there, if you do exactly the same call
> > from userspace the same arguments still apply.
> 
> I'm out of my depth when it comes to the politics of all this. So I
> think I'm going to bow out now. I'd already got the solution to my
> specific issue (write a tiny "syncfs" program and call it from my
> script prior to taking a snapshot). While I'd like to try and help
> others avoid the same pitfall as me, I'm not motivated enough to
> blunder any further into this disagreement.

 What is the current status of this issue?

 Do we really need a special option (fsfreeze -s) to call syncfs()?
 Cannot we call it always in fsfreeze(8)? 

 And if the thing will be implemented without the command line option
 then we can later remove the syncfs() call when kernel will be improved.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com