mm: Make sendfile(2) killable

Currently a simple program below issues a sendfile(2) system call which takes about 62 days to complete in my test KVM instance.

        int fd;
        off_t off = 0;

        fd = open("file", O_RDWR | O_TRUNC | O_SYNC | O_CREAT, 0644);
        ftruncate(fd, 2);
        lseek(fd, 0, SEEK_END);
        sendfile(fd, fd, &off, 0xfffffff);

Now you should not ask kernel to do a stupid stuff like copying 256MB in 2-byte chunks and call fsync(2) after each chunk but if you do, sysadmin should have a way to stop you.

We actually do have a check for fatal_signal_pending() in generic_perform_write() which triggers in this path however because we always succeed in writing something before the check is done, we return value > 0 from generic_perform_write() and thus the information about signal gets lost.

Fix the problem by doing the signal check before writing anything. That way generic_perform_write() returns -EINTR, the error gets propagated up and the sendfile loop terminates early.

...

mm/filemap.c +5 -0

@@ -2488,6 +2488,11 @@ again:
 			break;
 		}
 
+		if (fatal_signal_pending(current)) {
+			status = -EINTR;
+			break;
+		}
+
 		status = a_ops->write_begin(file, mapping, pos, bytes, flags,
 						&page, &fsdata);
 		if (unlikely(status < 0))

Quoted 2 lines · On Mon, 12 Oct 2015 14:45:23 +0200 Jan Kara <[email protected]> wrote:

Currently a simple program below issues a sendfile(2) system call which takes about 62 days to complete in my test KVM instance.

Geeze some people are impatient.

Quoted 12 lines

        int fd;
        off_t off = 0;

        fd = open("file", O_RDWR | O_TRUNC | O_SYNC | O_CREAT, 0644);
        ftruncate(fd, 2);
        lseek(fd, 0, SEEK_END);
        sendfile(fd, fd, &off, 0xfffffff);

Now you should not ask kernel to do a stupid stuff like copying 256MB in 2-byte chunks and call fsync(2) after each chunk but if you do, sysadmin should have a way to stop you.

Quoted 5 lines

We actually do have a check for fatal_signal_pending() in generic_perform_write() which triggers in this path however because we always succeed in writing something before the check is done, we return value > 0 from generic_perform_write() and thus the information about signal gets lost.

ah.

Quoted 30 lines

Fix the problem by doing the signal check before writing anything. That way generic_perform_write() returns -EINTR, the error gets propagated up and the sendfile loop terminates early.

...

mm/filemap.c +5 -4

@@ -2488,6 +2488,11 @@ again:
 			break;
 		}
 
+		if (fatal_signal_pending(current)) {
+			status = -EINTR;
+			break;
+		}
+
 		status = a_ops->write_begin(file, mapping, pos, bytes, flags,
 						&page, &fsdata);
 		if (unlikely(status < 0))

@@ -2525,10 +2530,6 @@ again:
 		written += copied;
 
 		balance_dirty_pages_ratelimited(mapping);
-		if (fatal_signal_pending(current)) {
-			status = -EINTR;
-			break;
-		}
 	} while (iov_iter_count(i));
 

mm/filemap.c again: · line 2535

Quoted 1 line

mm/filemap.c +0 -0

 	return written ? written : status;

This won't work, will it? If user hits ^C after we've written a few pages, `written' is non-zero and the same thing happens?

Quoted 11 lines

mm/filemap.c again: · line 2535

Quoted 8 lines

mm/filemap.c +0 -4

 		balance_dirty_pages_ratelimited(mapping);
-		if (fatal_signal_pending(current)) {
-			status = -EINTR;
-			break;
-		}
 	} while (iov_iter_count(i));
 
 	return written ? written : status;

This won't work, will it? If user hits ^C after we've written a few pages, `written' is non-zero and the same thing happens?

It does work - I've tested it :). Sure, the generic_perform_write() call that is running when the signal is delivered will return with value > 0. But the interesting thing is what happens after that: Either we return to userspace (and then we are fine) or generic_perform_write() gets called again because there's more to write and *that* call will return -EINTR which ends up terminating the whole sendfile syscall.