Make task in balance_dirty_pages() killable

There is no reason why task in balance_dirty_pages() shouldn't be killable and it helps in recovering from some error conditions (like when filesystem goes in error state and cannot accept writeback anymore but we still want to kill processes using it to be able to unmount it).

Reported-by Kazuya Mio <[email protected]>

Tested-by Kazuya Mio <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 mm/page-writeback.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

mm/page-writeback.c +4 -1

index 0360d1b..39f5805 100644
@@ -1133,7 +1133,7 @@ pause:
 					  pages_dirtied,
 					  pause,
 					  start_time);
-		__set_current_state(TASK_UNINTERRUPTIBLE);
+		__set_current_state(TASK_KILLABLE);
 		io_schedule_timeout(pause);
 
 		dirty_thresh = hard_dirty_limit(dirty_thresh);

@@ -1145,6 +1145,9 @@ pause:
 		 */
 		if (nr_dirty < dirty_thresh)
 			break;
+
+		if (fatal_signal_pending(current))
+			break;
 	}
 
 	if (!dirty_exceeded && bdi->dirty_exceeded)

Signature · 1 line

1.7.1

Quoted 4 lines · Jan Kara

There is no reason why task in balance_dirty_pages() shouldn't be killable and it helps in recovering from some error conditions (like when filesystem goes in error state and cannot accept writeback anymore but we still want to kill processes using it to be able to unmount it).

Here I'd like to add a paragraph from my original patch:

The follow up patches will further abort the generic_perform_write() and other filesystem write loops, to avoid large write + SIGKILL combination exceeding the dirty limit and possibly strange OOM.

Quoted 32 lines

Reported-by Kazuya Mio <[email protected]>

Tested-by Kazuya Mio <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 mm/page-writeback.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

mm/page-writeback.c +4 -1

index 0360d1b..39f5805 100644
@@ -1133,7 +1133,7 @@ pause:
 					  pages_dirtied,
 					  pause,
 					  start_time);
-		__set_current_state(TASK_UNINTERRUPTIBLE);
+		__set_current_state(TASK_KILLABLE);
 		io_schedule_timeout(pause);
 
 		dirty_thresh = hard_dirty_limit(dirty_thresh);

@@ -1145,6 +1145,9 @@ pause:
 		 */
 		if (nr_dirty < dirty_thresh)
 			break;
+
+		if (fatal_signal_pending(current))
+			break;
 	}
 
 	if (!dirty_exceeded && bdi->dirty_exceeded)

Signature · 1 line

1.7.1

Currently write(2) to a file is not interruptible by a signal. Sometimes this is desirable (e.g. when you want to quickly kill a process hogging your disk or when some process gets blocked in balance_dirty_pages() indefinitely due to a filesystem being in an error condition).

Reported-by Kazuya Mio <[email protected]>

Tested-by Kazuya Mio <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 mm/filemap.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

mm/filemap.c +4 -2

index c0018f2..c106d3b 100644
@@ -2407,7 +2407,6 @@ static ssize_t generic_perform_write(struct file *file,
 						iov_iter_count(i));
 
 again:
-
 		/*
 		 * Bring in the user page that we will copy from _first_.
 		 * Otherwise there's a nasty deadlock on copying from the

@@ -2463,7 +2462,10 @@ again:
 		written += copied;
 
 		balance_dirty_pages_ratelimited(mapping);
-
+		if (fatal_signal_pending(current)) {
+			status = -EINTR;
+			break;
+		}
 	} while (iov_iter_count(i));
 
 	return written ? written : status;

Signature · 1 line

1.7.1

Jan,

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window, would you agree?

Pushing the first patch will address the main problem, anyway.

Quoted 38 lines

Currently write(2) to a file is not interruptible by a signal. Sometimes this is desirable (e.g. when you want to quickly kill a process hogging your disk or when some process gets blocked in balance_dirty_pages() indefinitely due to a filesystem being in an error condition).

Reported-by Kazuya Mio <[email protected]>

Tested-by Kazuya Mio <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 mm/filemap.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

mm/filemap.c +4 -2

index c0018f2..c106d3b 100644
@@ -2407,7 +2407,6 @@ static ssize_t generic_perform_write(struct file *file,
 						iov_iter_count(i));
 
 again:
-
 		/*
 		 * Bring in the user page that we will copy from _first_.
 		 * Otherwise there's a nasty deadlock on copying from the

@@ -2463,7 +2462,10 @@ again:
 		written += copied;
 
 		balance_dirty_pages_ratelimited(mapping);
-
+		if (fatal_signal_pending(current)) {
+			status = -EINTR;
+			break;
+		}
 	} while (iov_iter_count(i));
 
 	return written ? written : status;

Signature · 1 line

1.7.1

Quoted 5 lines · Wu Fengguang

Jan,

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window, would you agree?

Fine with me. Thanks.

Quoted 1 line

Pushing the first patch will address the main problem, anyway.

Hopefully, yes.

Quoted 39 lines · Jan Kara

Quoted 38 lines

Currently write(2) to a file is not interruptible by a signal. Sometimes this is desirable (e.g. when you want to quickly kill a process hogging your disk or when some process gets blocked in balance_dirty_pages() indefinitely due to a filesystem being in an error condition).

Reported-by Kazuya Mio <[email protected]>

Tested-by Kazuya Mio <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 mm/filemap.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

mm/filemap.c +4 -2

index c0018f2..c106d3b 100644
@@ -2407,7 +2407,6 @@ static ssize_t generic_perform_write(struct file *file,
 						iov_iter_count(i));
 
 again:
-
 		/*
 		 * Bring in the user page that we will copy from _first_.
 		 * Otherwise there's a nasty deadlock on copying from the

@@ -2463,7 +2462,10 @@ again:
 		written += copied;
 
 		balance_dirty_pages_ratelimited(mapping);
-
+		if (fatal_signal_pending(current)) {
+			status = -EINTR;
+			break;
+		}
 	} while (iov_iter_count(i));
 
 	return written ? written : status;

Signature · 1 line

1.7.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window,

Quoted 2 lines · On Wed, 16 Nov 2011 19:44:21 +0800 Wu Fengguang <[email protected]> wrote:

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window,

Any such bugs will not be discovered in linux-next testing.

The only way to find these things in a reasonable period of time is to go in and find them. For example, intensive fsx-linux testing with concurrent heavy memory pressure on various filesystems with various block sizes. And of course concurrent signalling. If you're talking about O_DIRECT then iirc I hacked support for that into fsx-linux. I think.

Anyway, what _are_ the scenarios in which we think data can be lost?

Quoted 7 lines · Andrew Morton

Quoted 2 lines · On Wed, 16 Nov 2011 19:44:21 +0800 Wu Fengguang <[email protected]> wrote:

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window,

Any such bugs will not be discovered in linux-next testing.

Yup, I'm afraid.

Quoted 6 lines

The only way to find these things in a reasonable period of time is to go in and find them. For example, intensive fsx-linux testing with concurrent heavy memory pressure on various filesystems with various block sizes. And of course concurrent signalling. If you're talking about O_DIRECT then iirc I hacked support for that into fsx-linux. I think.

How are we going to measure the success/failure? Check if it eventually resulted in filesystem corruption or whatever?

When received SIGKILL, fsx-linux itself will just die.

Quoted 1 line

Anyway, what _are_ the scenarios in which we think data can be lost?

It's the vision that there may be partial writes on SIGKILL. Before patch, the write will either succeed as a whole or not started at all, depending on the timing of write/SIGKILL. This is kind of atomic operation. However now the write can be half done.

If the application really cares about atomic behavior, it can do create-and-rename. However there are always the possibility of broken applications.

Maybe this is not that big problem as SIGKILL is considered be to destructive already.

Quoted 4 lines

If the application really cares about atomic behavior, it can do create-and-rename. However there are always the possibility of broken applications.

Quoted 3 lines

Maybe this is not that big problem as SIGKILL is considered be to destructive already.

Yeah, I have dim dark memories that there are subtle problems with interrupting write(). Linus might remember.

Quoted 6 lines

Quoted 3 lines

Maybe this is not that big problem as SIGKILL is considered be to destructive already.

Yeah, I have dim dark memories that there are subtle problems with interrupting write(). Linus might remember.

The big one is that you're lucky if application programmers check the return values of write(2), and if they do, it's likely they will only check for error returns and not necessarily for partial writes --- since no other Unix-like or Linux-like system has ever returned partial reads or writes for files except in error conditions. We've barely gotten them trained to check for partial writes and reads with TCP connections and character devices, but I wouldn't bet on application programmers getting things right for files.

Still, if it's ***only*** for SIGKILL, we'll probably be OK, since for that one case there's no chance userspace can intercept the signal, so it can't do any recovery anyway. (I could imagine some HPC program doing a massive 2GB write, and some user of that program depending on the fact that he can kill it at a predefined place by sending a SIGKILL and knowing that the file would be written up to that 2GB chunk --- but that's clearly an edge situation, as opposed to something that would effect most GNOME and KDE apps.) We just need to make sure we never try to do this for any other signal that could be caught, such as SIGINT or SIGQUIT or (worse yet) SIGTSTP.

Quoted 10 lines

Still, if it's ***only*** for SIGKILL, we'll probably be OK, since for that one case there's no chance userspace can intercept the signal, so it can't do any recovery anyway. (I could imagine some HPC program doing a massive 2GB write, and some user of that program depending on the fact that he can kill it at a predefined place by sending a SIGKILL and knowing that the file would be written up to that 2GB chunk --- but that's clearly an edge situation, as opposed to something that would effect most GNOME and KDE apps.) We just need to make sure we never try to do this for any other signal that could be caught, such as SIGINT or SIGQUIT or (worse yet) SIGTSTP.

That it is a fatal SIGKILL means that the *current* application doesn't care. But other processes will sometimes notice this change. Previously if an app did write(file, 128k) and was hit with SIGKILL, it would write either 0 bytes or 128k bytes. Now, it can write 36k bytes, yes? If the target file consisted of a stream of 128k records then the user will claim, with some justification, that Linux corrupted it.

Dunno. People do lots of weird and flakey things. I have a suspicion that we'll be hearing back from them about this change.

Quoted 11 lines · Andrew Morton

Quoted 10 lines

Still, if it's ***only*** for SIGKILL, we'll probably be OK, since for that one case there's no chance userspace can intercept the signal, so it can't do any recovery anyway. (I could imagine some HPC program doing a massive 2GB write, and some user of that program depending on the fact that he can kill it at a predefined place by sending a SIGKILL and knowing that the file would be written up to that 2GB chunk --- but that's clearly an edge situation, as opposed to something that would effect most GNOME and KDE apps.) We just need to make sure we never try to do this for any other signal that could be caught, such as SIGINT or SIGQUIT or (worse yet) SIGTSTP.

Quoted 6 lines

That it is a fatal SIGKILL means that the *current* application doesn't care. But other processes will sometimes notice this change. Previously if an app did write(file, 128k) and was hit with SIGKILL, it would write either 0 bytes or 128k bytes. Now, it can write 36k bytes, yes? If the target file consisted of a stream of 128k records then the user will claim, with some justification, that Linux corrupted it.

On the other hand, if there was a crash mid-write, they might also get a 36k write that actually hit media (right? Or do we guarantee that on reboot you see a multiple of 128k?)

We could put in some nice code that rewinds i_size to where it used to be if the write was interrupted. Or do we need to? Presumably write_end would not have been called, so i_size would not have been updated.

On the other hand, if there was a crash mid-write, they might also get a 36k write that actually hit media (right? Or do we guarantee that on reboot you see a multiple of 128k?)

Quoted 2 lines

Dunno. People do lots of weird and flakey things. I have a suspicion that we'll be hearing back from them about this change.

The problem is that we may not hear from them for 6 years ... or whenever they decide to move off RHEL 3.

Quoted 3 lines · Matthew Wilcox

On the other hand, if there was a crash mid-write, they might also get a 36k write that actually hit media (right? Or do we guarantee that on reboot you see a multiple of 128k?)

Sure, but in the case the crash we expect things to be in a wonky state. The problem is if people assume atomic writes to files in a non-crash case, which has been a traditional Unix/Linux "feature". It's guaranteed by the standards as much a "close() implies fsync()", but once application programmers start coding to such assumptions, they refuse to admit they were wrong, and blame the kernel programmers.

Quoted 7 lines · Andrew Morton

Quoted 2 lines · On Wed, 16 Nov 2011 19:44:21 +0800 Wu Fengguang <[email protected]> wrote:

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window,

Any such bugs will not be discovered in linux-next testing.

Yup, I'm afraid.

Quoted 6 lines

The only way to find these things in a reasonable period of time is to go in and find them. For example, intensive fsx-linux testing with concurrent heavy memory pressure on various filesystems with various block sizes. And of course concurrent signalling. If you're talking about O_DIRECT then iirc I hacked support for that into fsx-linux. I think.

How are we going to measure the success/failure? Check if it eventually resulted in filesystem corruption or whatever?

Quoted 18 lines · Wu Fengguang

Quoted 7 lines · Andrew Morton

Quoted 2 lines · On Wed, 16 Nov 2011 19:44:21 +0800 Wu Fengguang <[email protected]> wrote:

Due to the (very low) possibility of data loss by partial writes, IMHO it would safer to test this patch in linux-next until next merge window,

Any such bugs will not be discovered in linux-next testing.

Yup, I'm afraid.

Quoted 6 lines

The only way to find these things in a reasonable period of time is to go in and find them. For example, intensive fsx-linux testing with concurrent heavy memory pressure on various filesystems with various block sizes. And of course concurrent signalling. If you're talking about O_DIRECT then iirc I hacked support for that into fsx-linux. I think.

Quoted 2 lines

How are we going to measure the success/failure? Check if it eventually resulted in filesystem corruption or whatever?

There are a few different questions:
1) Checking for filesystem corruption via fsck - I find such corruption
caused by stopping write early extremely unlikely.

2) Checking that we do not expose uninitialized data after a partial
(possibly DIRECT_IO) write - I did not find a place where that could happen but this would be worth testing. I think I can write a test for this if people are afraid of data exposure problems.

3) Is it acceptable for write(2) to be interrupted by SIGKILL in the
middle? That obviously does happen with my patches so there's no reason to test that. The question is whether someone cares or not and that can be tested only by reality check :). Since the signal is SIGKILL, the process itself cannot notice the interrupted write but someone else can. But as I already said earlier, partial writes can already be observed when the

machine crashes, filesystem is close to ENOSPC or so. Arguably these are more severe error conditions than application catching SIGKILL so my patch lowers the bar for observing partial writes. But I wouldn't like to throw away a sensible thing - allow SIGKILL to interrupt a system call - just because of fear of possibility some broken app could rely on this. Sure if the reality check shows there are such broken apps and users who care enough to report, then I have nothing against biting the bullet and reverting the change... Opinions?

Reading Ted's information feed, I tend to disregard the partial write
issue: since the "broken" applications will already fail and get punished in various other cases, I don't care adding one more penalty case to them :-P

Quoted 1 line · Wu Fengguang

Quoted 4 lines

Reading Ted's information feed, I tend to disregard the partial write
issue: since the "broken" applications will already fail and get punished in various other cases, I don't care adding one more penalty case to them :-P

Just wait until you have a bunch of rabid application programmers, questioning your judgement, your morality, and even your paternity. :-)

Quoted 4 lines · Theodore Ts'o

Quoted 1 line · Wu Fengguang

Quoted 8 lines

Quoted 4 lines

Reading Ted's information feed, I tend to disregard the partial write
issue: since the "broken" applications will already fail and get punished in various other cases, I don't care adding one more penalty case to them :-P

Just wait until you have a bunch of rabid application programmers, questioning your judgement, your morality, and even your paternity. :-)

Ah OK, that sounds frightening. Hmm, till now every one have acknowledged the possibility of data corruption, only that people have different perceptions of the severeness.

Let's rethink things this way: "Is it a _worthwhile_ risk at all?" I'm afraid not. Considering the origin of this patch

[BUG] aborted ext4 leads to inifinity loop in balance_dirty_pages
http://www.spinics.net/lists/linux-ext4/msg28464.html

I *think* Jan's first patch is already enough for fixing the bug. IWO the patch we worried/discussed so much is really an optional one. I would imagine the easy and safe solution is to just drop it. Any objections?

Quoted 21 lines · Wu Fengguang

Quoted 4 lines · Theodore Ts'o

Quoted 1 line · Wu Fengguang

Quoted 8 lines

Quoted 4 lines

Reading Ted's information feed, I tend to disregard the partial write
issue: since the "broken" applications will already fail and get punished in various other cases, I don't care adding one more penalty case to them :-P

Just wait until you have a bunch of rabid application programmers, questioning your judgement, your morality, and even your paternity. :-)

Ah OK, that sounds frightening. Hmm, till now every one have acknowledged the possibility of data corruption, only that people have different perceptions of the severeness.

Let's rethink things this way: "Is it a _worthwhile_ risk at all?" I'm afraid not. Considering the origin of this patch

Quoted 7 lines

[BUG] aborted ext4 leads to inifinity loop in balance_dirty_pages
http://www.spinics.net/lists/linux-ext4/msg28464.html

I *think* Jan's first patch is already enough for fixing the bug. IWO the patch we worried/discussed so much is really an optional one. I would imagine the easy and safe solution is to just drop it. Any objections?

Here is the replacement patch.

---
Subject: writeback: add a safety limit to the SIGKILL abort
Date: Mon Nov 28 11:16:54 CST 2011

This adds a safety limit to the SIGKILL abort in commit 499d05ecf990 ("mm: Make task in balance_dirty_pages() killable"). This will avoid dirty pages rushing arbitrarily high in the case some task receives SIGKILL and hence become *unthrottled* when doing a huge sized write.

The alternative way is to check SIGKILL and return partial write in generic_perform_write(). However it will lead to data corruption as put by Andrew Morton:

Previously if an app did write(file, 128k) and was hit with SIGKILL, it would write either 0 bytes or 128k bytes. Now, it can write 36k bytes, yes? If the target file consisted of a stream of 128k records then the user will claim, with some justification, that Linux corrupted it.

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/page-writeback.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

mm/page-writeback.c +2 -2

@@ -1136,7 +1136,8 @@ pause:
 		if (task_ratelimit)
 			break;
 
-		if (fatal_signal_pending(current))
+		if (fatal_signal_pending(current) &&
+		    nr_dirty <= dirty_thresh + dirty_thresh / 2)
 			break;
 	}
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Theodore Ts'o

Quoted 1 line · Wu Fengguang

Quoted 8 lines

Quoted 4 lines

Reading Ted's information feed, I tend to disregard the partial write
issue: since the "broken" applications will already fail and get punished in various other cases, I don't care adding one more penalty case to them :-P

Just wait until you have a bunch of rabid application programmers, questioning your judgement, your morality, and even your paternity. :-)

Ah OK, that sounds frightening. Hmm, till now every one have acknowledged the possibility of data corruption, only that people have different perceptions of the severeness.

Let's rethink things this way: "Is it a _worthwhile_ risk at all?" I'm afraid not. Considering the origin of this patch

[BUG] aborted ext4 leads to inifinity loop in balance_dirty_pages
http://www.spinics.net/lists/linux-ext4/msg28464.html

I *think* Jan's first patch is already enough for fixing the bug. IWO the patch we worried/discussed so much is really an optional one. I would imagine the easy and safe solution is to just drop it. Any objections?

Hello,

since the discussion has calmed down, here is a third iteration of the patches.
Fengguang, can you put them into your tree and merge them with Linus? Thanks.

Changes since v2:
* removed return value of balance_dirty_pages() since it didn't seem that useful after all.
* kept fatal_signal_pending() check in generic_perform_write() since I don't think possible partial writes when app receives SIGKILL are a problem.

Changes since v1:
* slightly moved the check in balance_dirty_pages() as Fengguang requested
* made balance_dirty_pages() return EINTR if fatal signal was detected
* changed check for signal to check for fatal signal in generic_perform_write() to avoid unexpected results for userspace applications.

Honza