xfs_copy: don't hang if /all/ the targets hit write errors
PATCH 1/1 xfs_copy: don't hang if /all/ the targets hit write errors
From: Darrick J. Wong <[email protected]>
If xfs_copy is told to copy a filesystem and /all/ the writer threads hit an write error, there won't be any threads to unlock mainwait, which means that write_wbuf will deadlock with itself trying to lock mainwait. Therefore, if we discover that all the writer threads are dead, just bail out.
Discovered by running xfs/073 with a tiny test device.
--- copy/xfs_copy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
copy/xfs_copy.c +12 -0
@@ -476,6 +476,7 @@ voidwrite_wbuf (void) {int i; +int badness =0 ;/* verify target threads */ for (i =0 ; i < num_targets; i++)
@@ -486,6 +487,17 @@ write_wbuf(void)for (i =0 ; i < num_targets; i++)if (target[i].state !=INACTIVE )pthread_mutex_unlock (&targ[i].wait);/* wake up */ +else + badness++; + +/* +* If all the targets are inactive then there won't be any io +* threads left to release mainwait. We're screwed, so bail out. +*/ +if (badness == num_targets) { +check_errors (); +exit (1 ); + }signal_maskfunc (SIGCHLD ,SIG_UNBLOCK );pthread_mutex_lock (&mainwait);
Quoted 30 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
If
xfs_copyis told to copy a filesystem and /all/ the writer threads hit an write error, there won't be any threads to unlock mainwait, which means thatwrite_wbufwill deadlock with itself trying to lock mainwait. Therefore, if we discover that all the writer threads are dead, just bail out.Discovered by running xfs/073 with a tiny test device.
Signed-off-by Darrick J. Wong <[email protected]>--- copy/xfs_copy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)copy/xfs_copy.c +1
-0@@ -476,6 +476,7 @@ voidwrite_wbuf (void) {int i; +int badness =0 ;/* verify target threads */ for (i =0 ; i < num_targets; i++)@@ -486,6 +487,17 @@ write_wbuf(void)for (i =0 ; i < num_targets; i++)if (target[i].state !=INACTIVE )
copy/xfs_copy.c write_wbuf(void) · line 498
Quoted 10 lines
copy/xfs_copy.c +9
-0pthread_mutex_unlock (&targ[i].wait);/* wake up */ + else + badness++; + +/* +* If all the targets are inactive then there won't be any io +* threads left to release mainwait. We're screwed, so bail out. +*/ +if (badness == num_targets) { +check_errors ();
libxfs_umount(mp); ?
-Eric
Quoted 10 lines
copy/xfs_copy.c +2
-1+exit (1 ); + }signal_maskfunc (SIGCHLD ,SIG_UNBLOCK );pthread_mutex_lock (&mainwait); --To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 37 lines · Eric Sandeen
Quoted 34 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
If
xfs_copyis told to copy a filesystem and /all/ the writer threads hit an write error, there won't be any threads to unlock mainwait, which means thatwrite_wbufwill deadlock with itself trying to lock mainwait. Therefore, if we discover that all the writer threads are dead, just bail out.Discovered by running xfs/073 with a tiny test device.
Signed-off-by Darrick J. Wong <[email protected]>--- copy/xfs_copy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)copy/xfs_copy.c +4
-0@@ -476,6 +476,7 @@ voidwrite_wbuf (void) {int i; +int badness =0 ;/* verify target threads */ for (i =0 ; i < num_targets; i++)@@ -486,6 +487,17 @@ write_wbuf(void)for (i =0 ; i < num_targets; i++)if (target[i].state !=INACTIVE )pthread_mutex_unlock (&targ[i].wait);/* wake up */ +else + badness++; +
Quoted 8 lines
copy/xfs_copy.c write_wbuf(void) · line 498
Quoted 6 lines
copy/xfs_copy.c +6
-0+/* +* If all the targets are inactive then there won't be any io +* threads left to release mainwait. We're screwed, so bail out. +*/ +if (badness == num_targets) { +check_errors ();
libxfs_umount(mp); ?
Doh. v2 on its way
--D
Quoted 16 lines
-Eric
Quoted 10 lines
copy/xfs_copy.c +2
-1+exit (1 ); + }signal_maskfunc (SIGCHLD ,SIG_UNBLOCK );pthread_mutex_lock (&mainwait); --To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 47 lines · Darrick J. Wong
Quoted 37 lines · Eric Sandeen
Quoted 34 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
If
xfs_copyis told to copy a filesystem and /all/ the writer threads hit an write error, there won't be any threads to unlock mainwait, which means thatwrite_wbufwill deadlock with itself trying to lock mainwait. Therefore, if we discover that all the writer threads are dead, just bail out.Discovered by running xfs/073 with a tiny test device.
Signed-off-by Darrick J. Wong <[email protected]>--- copy/xfs_copy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)copy/xfs_copy.c +4
-0@@ -476,6 +476,7 @@ voidwrite_wbuf (void) {int i; +int badness =0 ;/* verify target threads */ for (i =0 ; i < num_targets; i++)@@ -486,6 +487,17 @@ write_wbuf(void)for (i =0 ; i < num_targets; i++)if (target[i].state !=INACTIVE )pthread_mutex_unlock (&targ[i].wait);/* wake up */ +else + badness++; +Quoted 8 lines
copy/xfs_copy.c write_wbuf(void) · line 498
Quoted 6 lines
copy/xfs_copy.c +6
-0+/* +* If all the targets are inactive then there won't be any io +* threads left to release mainwait. We're screwed, so bail out. +*/ +if (badness == num_targets) { +check_errors ();
libxfs_umount(mp); ?
Quoted 1 line
Doh. v2 on its way
Hmmm. The other error bailouts don't call libxfs_umount and it hardly matters since we're exiting anyway. The mp is a local variable to main so we'd have to convey abort status out of write_wbuf back to main. That's a bigger change; do you want me to pursue that instead?
--D
Quoted 22 lines
--D
Quoted 5 lines
-Eric
Quoted 3 lines
copy/xfs_copy.c +2
-0+exit (1 ); + }Quoted 11 lines
Quoted 7 lines
copy/xfs_copy.c +0
-1signal_maskfunc (SIGCHLD ,SIG_UNBLOCK );pthread_mutex_lock (&mainwait); --To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 48 lines · Darrick J. Wong
Quoted 47 lines · Darrick J. Wong
Quoted 45 lines · Eric Sandeen
Quoted 30 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
If
xfs_copyis told to copy a filesystem and /all/ the writer threads hit an write error, there won't be any threads to unlock mainwait, which means thatwrite_wbufwill deadlock with itself trying to lock mainwait. Therefore, if we discover that all the writer threads are dead, just bail out.Discovered by running xfs/073 with a tiny test device.
Signed-off-by Darrick J. Wong <[email protected]>--- copy/xfs_copy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)copy/xfs_copy.c +1
-0@@ -476,6 +476,7 @@ voidwrite_wbuf (void) {int i; +int badness =0 ;/* verify target threads */ for (i =0 ; i < num_targets; i++)@@ -486,6 +487,17 @@ write_wbuf(void)for (i =0 ; i < num_targets; i++)if (target[i].state !=INACTIVE )copy/xfs_copy.c write_wbuf(void) · line 498
Quoted 10 lines
copy/xfs_copy.c +9
-0pthread_mutex_unlock (&targ[i].wait);/* wake up */ + else + badness++; + +/* +* If all the targets are inactive then there won't be any io +* threads left to release mainwait. We're screwed, so bail out. +*/ +if (badness == num_targets) { +check_errors ();
libxfs_umount(mp); ?
Quoted 6 lines
Quoted 1 line
Doh. v2 on its way
Hmmm. The other error bailouts don't call
libxfs_umountand it hardly matters since we're exiting anyway. The mp is a local variable to main so we'd have to convey abort status out ofwrite_wbufback to main. That's a bigger change; do you want me to pursue that instead?
Eh if there's precedent for such sloppiness, I guess we can stick with V1. ;)
Thanks for checking.
-Eric