Tools: hv: vssdaemon: freeze/thaw logic improvement for the failure case

PATCH In review Vitaly Kuznetsov 3 participants 7 messages Nov 10, 2014 – Nov 19, 2014 Hyper-V/Azure core and drivers

Share & tools

Vitaly Kuznetsov

author

2014-11-10 16:37 UTC #0 lore raw

v2 cover Tools: hv: vssdaemon: freeze/thaw logic improvement for the failure case

This patch series addresses the following issues:
- Verbosely report errors during freeze (mountpoint, exact error);
- Skip all readonly-mounted filesystems instead of skipping iso9660 only.

Changes since v1:
- Rebase on top of 'char-misc-next';
- "Tools: hv: vssdaemon: thaw everything in case of freeze" was thrown away as Dexuan's "Tools: hv: vssdaemon: ignore the EBUSY on multiple freezing the same partition" contains the same change;
- "Tools: hv: vssdaemon: consult with errno in case of failure only" was replaced with "Tools: hv: vssdaemon: report freeze errors".

Vitaly Kuznetsov (2):
Tools: hv: vssdaemon: report freeze errors
Tools: hv: vssdaemon: skip all filesystems mounted readonly

 tools/hv/hv_vss_daemon.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Signature · 1 line

1.9.3

Vitaly Kuznetsov

author

2014-11-10 16:37 UTC #1 lore raw

PATCH v2 1/2 Tools: hv: vssdaemon: report freeze errors

When ioctl(fd, FIFREEZE, 0) results in an error we cannot report it to syslog instantly since that can cause write to a frozen disk. However, the name of the filesystem which caused the error and errno are valuable and we would like to get a nice human-readable message in the log. Save errno before calling vss_operate(VSS_OP_THAW) and report the error right after.

Unfortunately, FITHAW errors cannot be reported the same way as we need to finish thawing all filesystems before calling syslog().

We should also avoid calling endmntent() for the second time in case we encountered an error during freezing of '/' as it usually results in SEGSEGV.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
 tools/hv/hv_vss_daemon.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

tools/hv/hv_vss_daemon.c +12 -4

index b720d8f..ee44f0d 100644
@@ -82,7 +82,7 @@ static int vss_operate(int operation)
 	FILE *mounts;
 	struct mntent *ent;
 	unsigned int cmd;
-	int error = 0, root_seen = 0;
+	int error = 0, root_seen = 0, save_errno = 0;
 
 	switch (operation) {
 	case VSS_OP_FREEZE:

@@ -114,7 +114,6 @@ static int vss_operate(int operation)
 		if (error && operation == VSS_OP_FREEZE)
 			goto err;
 	}
-	endmntent(mounts);
 
 	if (root_seen) {
 		error |= vss_do_freeze("/", cmd);

@@ -122,10 +121,19 @@ static int vss_operate(int operation)
 			goto err;
 	}
 
-	return error;
+	goto out;
 err:
-	endmntent(mounts);
+	save_errno = errno;
 	vss_operate(VSS_OP_THAW);
+	/* Call syslog after we thaw all filesystems */
+	if (ent)
+		syslog(LOG_ERR, "FREEZE of %s failed; error:%d %s",
+		       ent->mnt_dir, save_errno, strerror(save_errno));
+	else
+		syslog(LOG_ERR, "FREEZE of / failed; error:%d %s", save_errno,
+		       strerror(save_errno));
+out:
+	endmntent(mounts);
 	return error;
 }

Signature · 1 line

1.9.3

Dexuan Cui

maintainer Acked-by on v2 1/2

2014-11-11 03:08 UTC #2 lore raw

RE: [PATCH v2 1/2] Tools: hv: vssdaemon: report freeze errors

Quoted 17 lines

-----Original Message-----
From: Vitaly Kuznetsov
Sent: Tuesday, November 11, 2014 0:37 AM
To: KY Srinivasan; Haiyang Zhang; Greg Kroah-Hartman
Cc: [email protected]; [email protected]; Dexuan Cui
Subject: [PATCH v2 1/2] Tools: hv: vssdaemon: report freeze errors
When ioctl(fd, FIFREEZE, 0) results in an error we cannot report it to syslog instantly since that can cause write to a frozen disk. However, the name of the filesystem which caused the error and errno are valuable and we would like to get a nice human-readable message in the log. Save errno before calling vss_operate(VSS_OP_THAW) and report the error right after.
Unfortunately, FITHAW errors cannot be reported the same way as we need to finish thawing all filesystems before calling syslog().

Quoted 8 lines

We should also avoid calling endmntent() for the second time in case we encountered an error during freezing of '/' as it usually results in SEGSEGV.
Signed-off-by Vitaly Kuznetsov <[email protected]>
---
 tools/hv/hv_vss_daemon.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Hi Vitaly,
Thanks for the patch -- it does improve the error handling!

Acked-by Dexuan Cui <[email protected]>

-- Dexuan

KY Srinivasan

2014-11-19 23:13 UTC #3 lore raw

RE: [PATCH v2 1/2] Tools: hv: vssdaemon: report freeze errors

In reply to Vitaly Kuznetsov

Quoted 16 lines

-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Monday, November 10, 2014 8:37 AM
To: KY Srinivasan; Haiyang Zhang; Greg Kroah-Hartman
Cc: [email protected]; [email protected]; Dexuan Cui
Subject: [PATCH v2 1/2] Tools: hv: vssdaemon: report freeze errors
When ioctl(fd, FIFREEZE, 0) results in an error we cannot report it to syslog instantly since that can cause write to a frozen disk.
However, the name of the filesystem which caused the error and errno are valuable and we would like to get a nice human-readable message in the log. Save errno before calling vss_operate(VSS_OP_THAW) and report the error right after.

Quoted 7 lines

Unfortunately, FITHAW errors cannot be reported the same way as we need to finish thawing all filesystems before calling syslog().
We should also avoid calling endmntent() for the second time in case we encountered an error during freezing of '/' as it usually results in SEGSEGV.
Signed-off-by Vitaly Kuznetsov <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

Quoted 50 lines

---
 tools/hv/hv_vss_daemon.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

tools/hv/hv_vss_daemon.c index renamed from tools/hv/hv_vss_daemon.c +0 -0

b720d8f..ee44f0d 100644

tools/hv/hv_vss_daemon.c +12 -5

@@ -82,7 +82,7 @@ static int vss_operate(int operation)
 	FILE *mounts;
 	struct mntent *ent;
 	unsigned int cmd;
-	int error = 0, root_seen = 0;
+	int error = 0, root_seen = 0, save_errno = 0;
 
 	switch (operation) {
 	case VSS_OP_FREEZE:

@@ -114,7 +114,6 @@ static int vss_operate(int operation)
 		if (error && operation == VSS_OP_FREEZE)
 			goto err;
 	}
-	endmntent(mounts);
 
 	if (root_seen) {
 		error |= vss_do_freeze("/", cmd);

@@ -122,10 +121,19 @@ static int vss_operate(int operation)
 			goto err;
 	}
 
-	return error;
+	goto out;
 err:
-	endmntent(mounts);
+	save_errno = errno;
 	vss_operate(VSS_OP_THAW);
+	/* Call syslog after we thaw all filesystems */
+	if (ent)
+		syslog(LOG_ERR, "FREEZE of %s failed; error:%d %s",
+		       ent->mnt_dir, save_errno, strerror(save_errno));
+	else
+		syslog(LOG_ERR, "FREEZE of / failed; error:%d %s",
 save_errno,
+		       strerror(save_errno));
+out:
+	endmntent(mounts);
 	return error;
 }
 
--

1.9.3

Vitaly Kuznetsov

author

2014-11-10 16:37 UTC #4 lore raw

PATCH v2 2/2 Tools: hv: vssdaemon: skip all filesystems mounted readonly

In reply to Vitaly Kuznetsov

Instead of making a list of exceptions for readonly filesystems in addition to iso9660 we already have it is better to skip freeze operation for all readonly-mounted filesystems.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
 tools/hv/hv_vss_daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/hv/hv_vss_daemon.c +1 -1

index ee44f0d..5e63f70 100644
@@ -102,7 +102,7 @@ static int vss_operate(int operation)
 	while ((ent = getmntent(mounts))) {
 		if (strncmp(ent->mnt_fsname, match, strlen(match)))
 			continue;
-		if (strcmp(ent->mnt_type, "iso9660") == 0)
+		if (hasmntopt(ent, MNTOPT_RO) != NULL)
 			continue;
 		if (strcmp(ent->mnt_type, "vfat") == 0)
 			continue;

Signature · 1 line

1.9.3

Dexuan Cui

maintainer Acked-by on v2 2/2

2014-11-11 03:23 UTC #5 lore raw

RE: [PATCH v2 2/2] Tools: hv: vssdaemon: skip all filesystems mounted readonly

Quoted 22 lines

-----Original Message-----
From: Vitaly Kuznetsov
Sent: Tuesday, November 11, 2014 0:37 AM
To: KY Srinivasan; Haiyang Zhang; Greg Kroah-Hartman
Cc: [email protected]; [email protected]; Dexuan Cui
Subject: [PATCH v2 2/2] Tools: hv: vssdaemon: skip all filesystems mounted readonly
Instead of making a list of exceptions for readonly filesystems in addition to iso9660 we already have it is better to skip freeze operation for all readonly-mounted filesystems.
Signed-off-by Vitaly Kuznetsov <[email protected]>
---
 tools/hv/hv_vss_daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/hv/hv_vss_daemon.c +0 -0
index ee44f0d..5e63f70 100644

tools/hv/hv_vss_daemon.c static int vss_operate(int operation)

Quoted 9 lines

tools/hv/hv_vss_daemon.c +1 -2

@@ -102,7 +102,7 @@ static int vss_operate(int operation)
 	while ((ent = getmntent(mounts))) {
 		if (strncmp(ent->mnt_fsname, match, strlen(match)))
 			continue;
-		if (strcmp(ent->mnt_type, "iso9660") == 0)
+		if (hasmntopt(ent, MNTOPT_RO) != NULL)
 			continue;
 		if (strcmp(ent->mnt_type, "vfat") == 0)
 			continue;
--

Acked-by Dexuan Cui <[email protected]>

KY Srinivasan

2014-11-19 23:01 UTC #6 lore raw

RE: [PATCH v2 2/2] Tools: hv: vssdaemon: skip all filesystems mounted readonly

In reply to Vitaly Kuznetsov

Quoted 10 lines

-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Monday, November 10, 2014 8:37 AM
To: KY Srinivasan; Haiyang Zhang; Greg Kroah-Hartman
Cc: [email protected]; [email protected]; Dexuan Cui
Subject: [PATCH v2 2/2] Tools: hv: vssdaemon: skip all filesystems mounted readonly

Quoted 5 lines

Instead of making a list of exceptions for readonly filesystems in addition to iso9660 we already have it is better to skip freeze operation for all readonly- mounted filesystems.
Signed-off-by Vitaly Kuznetsov <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

Quoted 19 lines

---
 tools/hv/hv_vss_daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/hv/hv_vss_daemon.c index renamed from tools/hv/hv_vss_daemon.c +0 -0

ee44f0d..5e63f70 100644

tools/hv/hv_vss_daemon.c +1 -2

@@ -102,7 +102,7 @@ static int vss_operate(int operation)
 	while ((ent = getmntent(mounts))) {
 		if (strncmp(ent->mnt_fsname, match, strlen(match)))
 			continue;
-		if (strcmp(ent->mnt_type, "iso9660") == 0)
+		if (hasmntopt(ent, MNTOPT_RO) != NULL)
 			continue;
 		if (strcmp(ent->mnt_type, "vfat") == 0)
 			continue;
--

1.9.3