Skip to contentSkip to conversation

open(2) says O_DIRECT works on 512 byte boundries?

DISCUSSION Greg KH 9 participants 26 messages Jan 28, 2009 – Feb 7, 2009 1 message not in the archive

Subsystems (7)
Memory management MM core Scheduler exec and binfmt +3 more subsystems
lore
Share & tools
lore

Greg KH

author
#0 lore raw

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.

However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)

Is this just a mistake in the documentation? Or am I reading it incorrectly?

I have a test program that shows this if anyone wants it.

thanks,

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

#1 lore raw
Quoted 7 lines · Greg KH

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.

Quoted 8 lines

However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)

Is this just a mistake in the documentation? Or am I reading it incorrectly?

I have a test program that shows this if anyone wants it.

Well, it sounds like a bug to me.. even if it's not supported, if you do such an access, surely the kernel should detect that and return EINVAL or something rather than reading corrupted data..

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw
Quoted 16 lines · Robert Hancock
Quoted 4 lines · Greg KH

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical

Quoted 8 lines

block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.
However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)
Is this just a mistake in the documentation? Or am I reading it incorrectly?
I have a test program that shows this if anyone wants it.

Quoted 3 lines

Well, it sounds like a bug to me.. even if it's not supported, if you do such an access, surely the kernel should detect that and return EINVAL or something rather than reading corrupted data..

It doesn't. It says the read is successful, yet the data is not really read into the buffer. Portions of it is, but not the amount we asked for.

thanks,

greg k-h

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

#4 lore raw
Quoted 20 lines · Greg KH
Quoted 16 lines · Robert Hancock
Quoted 12 lines · Greg KH

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.
However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)
Is this just a mistake in the documentation? Or am I reading it incorrectly?
I have a test program that shows this if anyone wants it.

Quoted 7 lines
Quoted 3 lines

Well, it sounds like a bug to me.. even if it's not supported, if you do such an access, surely the kernel should detect that and return EINVAL or something rather than reading corrupted data..

It doesn't. It says the read is successful, yet the data is not really read into the buffer. Portions of it is, but not the amount we asked for.

Greg,

Can you post your test program?

Cheers,

Michael

Signature · 11 lines
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html


--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#5 lore raw
Quoted 25 lines · Michael Kerrisk
Quoted 24 lines · Greg KH
Quoted 19 lines · Robert Hancock
Quoted 4 lines · Greg KH

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical

Quoted 8 lines

block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.
However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)
Is this just a mistake in the documentation? Or am I reading it incorrectly?
I have a test program that shows this if anyone wants it.

Well, it sounds like a bug to me.. even if it's not supported, if you do such an access, surely the kernel should detect that and return EINVAL or something rather than reading corrupted data..

Quoted 7 lines
Quoted 3 lines

It doesn't. It says the read is successful, yet the data is not really read into the buffer. Portions of it is, but not the amount we asked for.

Greg,

Can you post your test program?

Sure, here it is. I'm still not quite sure it is valid, but at first glance it seems to be.

Run it once with no arguments and all of the files will be created. Then run it again with no offset being asked for:

	./dma_thread -a 0
then with an offset:
	./dma_thread -a 512

The second one breaks.

thanks,

greg k-h

/* compile with 'gcc -g -o dma_thread dma_thread.c -lpthread' */

#define _GNU_SOURCE 1

#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <memory.h>
#include <pthread.h>
#include <getopt.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/wait.h>

#define FILESIZE (12*1024*1024)
#define READSIZE (1024*1024)

#define FILENAME "test_%.04d.tmp"
#define FILECOUNT 100
#define MIN_WORKERS 2
#define MAX_WORKERS 256
#define PAGE_SIZE 4096

#define true 1
#define false 0

typedef int bool;

bool done = false;
int workers = 2;

#define PATTERN (0xfa)

static void
usage (void)
{
fprintf(stderr, "\nUsage: dma_thread [-h | -a <alignment> [ -w <workers>]\n"
"\nWith no arguments, generate test files and exit.\n" "-h Display this help and exit.\n"
"-a align read buffer to offset <alignment>.\n" "-w number of worker threads, 2 (default) to 256,\n"
" defaults to number of cores.\n\n"

"Run first with no arguments to generate files.\n" "Then run with -a <alignment> = 512 or 0. \n");
}

typedef struct {
    pthread_t	    tid;
    int		    worker_number;
    int		    fd;
    int		    offset;
    int		    length;
    int		    pattern;
    unsigned char  *buffer;
} worker_t;

void *worker_thread(void * arg)

{
    int		    bytes_read;
    int		    i,k;
    worker_t	   *worker  = (worker_t *) arg;
    int		    offset  = worker->offset;
    int		    fd	    = worker->fd;
    unsigned char  *buffer  = worker->buffer;
    int		    pattern = worker->pattern;
    int		    length  = worker->length;
    
    if (lseek(fd, offset, SEEK_SET) < 0) {
	fprintf(stderr, "Failed to lseek to %d on fd %d: %s.\n", 
			offset, fd, strerror(errno));
	exit(1);
    }

    bytes_read = read(fd, buffer, length);
    if (bytes_read != length) {
	fprintf(stderr, "read failed on fd %d: bytes_read %d, %s\n", 
			fd, bytes_read, strerror(errno));
	exit(1);
    }

    /* Corruption check */
    for (i = 0; i < length; i++) {
	if (buffer[i] != pattern) {
	    printf("Bad data at 0x%.06x: %p, \n", i, buffer + i);
	    printf("Data dump starting at 0x%.06x:\n", i - 8);
	    printf("Expect 0x%x followed by 0x%x:\n",
		    pattern, PATTERN);

	    for (k = 0; k < 16; k++) {
		printf("%02x ", buffer[i - 8 + k]);
		if (k == 7) {
		    printf("\n");
		}       
	    }

	    printf("\n");
	    abort();
	}
    }

    return 0;
}

void *fork_thread (void *arg)

{
    pid_t pid;

    while (!done) {
	pid = fork();
	if (pid == 0) {
	    exit(0);
	} else if (pid < 0) {
	    fprintf(stderr, "Failed to fork child.\n");
	    exit(1);
	} 
	waitpid(pid, NULL, 0 );
	usleep(100);
    }

    return NULL;

}

int main(int argc, char *argv[])

{
    unsigned char  *buffer = NULL;
    char	    filename[1024];
    int		    fd;
    bool	    dowrite = true;
    pthread_t	    fork_tid;
    int		    c, n, j;
    worker_t	   *worker;
    int		    align = 0;
    int		    offset, rc;

    workers = sysconf(_SC_NPROCESSORS_ONLN);

    while ((c = getopt(argc, argv, "a:hw:")) != -1) {
	switch (c) {
	case 'a':
	    align = atoi(optarg);
	    if (align < 0 || align > PAGE_SIZE) {
		printf("Bad alignment %d.\n", align);
		exit(1);
	    }
	    dowrite = false;
	    break;

	case 'h':
	    usage();
	    exit(0);
	    break;

	case 'w':
	    workers = atoi(optarg);
	    if (workers < MIN_WORKERS || workers > MAX_WORKERS) {
		fprintf(stderr, "Worker count %d not between "
				"%d and %d, inclusive.\n",
				workers, MIN_WORKERS, MAX_WORKERS);
		usage();
		exit(1);
	    }
	    dowrite = false;
	    break;

	default:
	    usage();
	    exit(1);
	}
    }

    if (argc > 1 && (optind < argc)) {
	fprintf(stderr, "Bad command line.\n");
	usage();
	exit(1);
    }

    if (dowrite) {

	buffer = malloc(FILESIZE);
	if (buffer == NULL) {
	    fprintf(stderr, "Failed to malloc write buffer.\n");
	    exit(1);
	}

	for (n = 1; n <= FILECOUNT; n++) {
	    sprintf(filename, FILENAME, n);
	    fd = open(filename, O_RDWR|O_CREAT|O_TRUNC, 0666);
	    if (fd < 0) {
		printf("create failed(%s): %s.\n", filename, strerror(errno));
		exit(1);
	    }
	    memset(buffer, n, FILESIZE);
	    printf("Writing file %s.\n", filename);
	    if (write(fd, buffer, FILESIZE) != FILESIZE) {
		printf("write failed (%s)\n", filename);
	    }

	    close(fd);
	    fd = -1;
	}

	free(buffer);
	buffer = NULL;

	printf("done\n");
	exit(0);
    }

    printf("Using %d workers.\n", workers);

    worker = malloc(workers * sizeof(worker_t));
    if (worker == NULL) {
	fprintf(stderr, "Failed to malloc worker array.\n");
	exit(1);
    }

    for (j = 0; j < workers; j++) {
	worker[j].worker_number = j;
    }

    printf("Using alignment %d.\n", align);
    
    posix_memalign((void *)&buffer, PAGE_SIZE, READSIZE+ align);
    printf("Read buffer: %p.\n", buffer);
    for (n = 1; n <= FILECOUNT; n++) {

	sprintf(filename, FILENAME, n);
	for (j = 0; j < workers; j++) {
	    if ((worker[j].fd = open(filename,  O_RDONLY|O_DIRECT)) < 0) {
		fprintf(stderr, "Failed to open %s: %s.\n",
				filename, strerror(errno));
		exit(1);
	    }

	    worker[j].pattern = n;
	}

	printf("Reading file %d.\n", n);

	for (offset = 0; offset < FILESIZE; offset += READSIZE) {
	    memset(buffer, PATTERN, READSIZE + align);
	    for (j = 0; j < workers; j++) {
		worker[j].offset = offset + j * PAGE_SIZE;
		worker[j].buffer = buffer + align + j * PAGE_SIZE;
		worker[j].length = PAGE_SIZE;
	    }
	    /* The final worker reads whatever is left over. */
	    worker[workers - 1].length = READSIZE - PAGE_SIZE * (workers - 1);

	    done = 0;

	    rc = pthread_create(&fork_tid, NULL, fork_thread, NULL);
	    if (rc != 0) {
		fprintf(stderr, "Can't create fork thread: %s.\n", 
				strerror(rc));
		exit(1);
	    }

	    for (j = 0; j < workers; j++) {
		rc = pthread_create(&worker[j].tid, 
				    NULL, 
				    worker_thread, 
				    worker + j);
		if (rc != 0) {
		    fprintf(stderr, "Can't create worker thread %d: %s.\n", 
				    j, strerror(rc));
		    exit(1);
		}
	    }

	    for (j = 0; j < workers; j++) {
		rc = pthread_join(worker[j].tid, NULL);
		if (rc != 0) {
		    fprintf(stderr, "Failed to join worker thread %d: %s.\n",
				    j, strerror(rc));
		    exit(1);
		}
	    }

	    /* Let the fork thread know it's ok to exit */
	    done = 1;

	    rc = pthread_join(fork_tid, NULL);
	    if (rc != 0) {
		fprintf(stderr, "Failed to join fork thread: %s.\n",
				strerror(rc));
		exit(1);
	    }
	}

	/* Close the fd's for the next file. */
	for (j = 0; j < workers; j++) {
	    close(worker[j].fd);
	}
    }

    return 0;
}
#6 lore raw
Quoted 37 lines · Greg KH
Quoted 25 lines · Michael Kerrisk
Quoted 24 lines · Greg KH
Quoted 19 lines · Robert Hancock
Quoted 4 lines · Greg KH

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical

Quoted 8 lines

block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.
However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)
Is this just a mistake in the documentation? Or am I reading it incorrectly?
I have a test program that shows this if anyone wants it.

Well, it sounds like a bug to me.. even if it's not supported, if you do such an access, surely the kernel should detect that and return EINVAL or something rather than reading corrupted data..

Quoted 7 lines
Quoted 3 lines

It doesn't. It says the read is successful, yet the data is not really read into the buffer. Portions of it is, but not the amount we asked for.

Greg,

Can you post your test program?

Sure, here it is. I'm still not quite sure it is valid, but at first glance it seems to be.

Quoted 7 lines

Run it once with no arguments and all of the files will be created. Then run it again with no offset being asked for:

	./dma_thread -a 0
then with an offset:
	./dma_thread -a 512

The second one breaks.

There are several folks working on this. See "Corruption with O_DIRECT and unaligned user buffers" on the linux-fsdevel list. There is also a Red Hat bugzilla for this (471613) that several folks have been working through.

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

author
#7 lore raw
Quoted 45 lines · Jeff Moyer
Quoted 43 lines · Greg KH
Quoted 25 lines · Michael Kerrisk
Quoted 24 lines · Greg KH
Quoted 19 lines · Robert Hancock
Quoted 4 lines · Greg KH

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical

Quoted 8 lines

block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.
However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)
Is this just a mistake in the documentation? Or am I reading it incorrectly?
I have a test program that shows this if anyone wants it.

Well, it sounds like a bug to me.. even if it's not supported, if you do such an access, surely the kernel should detect that and return EINVAL or something rather than reading corrupted data..

Quoted 7 lines
Quoted 3 lines

It doesn't. It says the read is successful, yet the data is not really read into the buffer. Portions of it is, but not the amount we asked for.

Greg,

Can you post your test program?

Sure, here it is. I'm still not quite sure it is valid, but at first glance it seems to be.

Run it once with no arguments and all of the files will be created. Then run it again with no offset being asked for:

	./dma_thread -a 0
then with an offset:
	./dma_thread -a 512
Quoted 6 lines
Quoted 1 line

The second one breaks.

There are several folks working on this. See "Corruption with O_DIRECT and unaligned user buffers" on the linux-fsdevel list. There is also a Red Hat bugzilla for this (471613) that several folks have been working through.

Thanks for the pointers to that, I'll follow along with it.

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

#8 lore raw
In reply to Greg KH
Quoted 11 lines · On Wed, 28 Jan 2009 13:33:22 -0800 Greg KH…

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.

However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)

IIUC, it's not related to 512bytes boundary. Just a race between direct-io v.s. copy-on-write. Copy-on-Write while reading a page via DIO is a problem.

Maybe it's true that if buffer is aligned to page size, no copy-on-write will happen in usual program. But assuming HugeTLB page, which does Copy-on-Write, data corruption will happen again. HugeTLB aligned buffer is nonsense.

Thanks,
-Kame

Quoted 5 lines

Is this just a mistake in the documentation? Or am I reading it incorrectly?

I have a test program that shows this if anyone wants it.

Quoted 9 lines

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

#9 lore raw

(CC to andrea)

Quoted 10 lines
Quoted 7 lines · On Wed, 28 Jan 2009 13:33:22 -0800 Greg KH…

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.

Quoted 8 lines
Quoted 4 lines

However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)

IIUC, it's not related to 512bytes boundary. Just a race between direct-io v.s. copy-on-write. Copy-on-Write while reading a page via DIO is a problem.

Yes.
Greg's reproducer is a bit misleading.

Quoted 5 lines
	    for (j = 0; j < workers; j++) {
		worker[j].offset = offset + j * PAGE_SIZE;
		worker[j].buffer = buffer + align + j * PAGE_SIZE;
		worker[j].length = PAGE_SIZE;
	    }

this code mean,
- if align == 0, reader thread touch only one page. and the page is touched only one thread.
- if align != 0, reader thread touch two page. and the page is touched two thread.

then, race is happend if align != 0.
We discussed this issue with andrea last month. ("Corruption with O_DIRECT and unaligned user buffers" thread)

As far as I know, he is working on fixing this issue now.

Quoted 4 lines

Maybe it's true that if buffer is aligned to page size, no copy-on-write will happen in usual program. But assuming HugeTLB page, which does Copy-on-Write, data corruption will happen again. HugeTLB aligned buffer is nonsense.

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

author
#10 lore raw
Quoted 36 lines · KOSAKI Motohiro

(CC to andrea)

Quoted 10 lines
Quoted 7 lines · On Wed, 28 Jan 2009 13:33:22 -0800 Greg KH…

In looking at open(2), it says that O_DIRECT works on 512 byte boundries with the 2.6 kernel release:
Under Linux 2.4, transfer sizes, and the alignment of the user buffer and the file offset must all be multiples of the logical block size of the file system. Under Linux 2.6, alignment to 512-byte boundaries suffices.

Quoted 8 lines
Quoted 4 lines

However if you try to access an O_DIRECT opened file with a buffer that is PAGE_SIZE aligned + 512 bytes, it fails in a bad way (wrong data is read.)

IIUC, it's not related to 512bytes boundary. Just a race between direct-io v.s. copy-on-write. Copy-on-Write while reading a page via DIO is a problem.

Yes.
Greg's reproducer is a bit misleading.

Quoted 5 lines
	    for (j = 0; j < workers; j++) {
		worker[j].offset = offset + j * PAGE_SIZE;
		worker[j].buffer = buffer + align + j * PAGE_SIZE;
		worker[j].length = PAGE_SIZE;
	    }

this code mean,
- if align == 0, reader thread touch only one page. and the page is touched only one thread.
- if align != 0, reader thread touch two page. and the page is touched two thread.

Quoted 5 lines

then, race is happend if align != 0.
We discussed this issue with andrea last month. ("Corruption with O_DIRECT and unaligned user buffers" thread)

As far as I know, he is working on fixing this issue now.

Thanks for the pointers, I'll go read the thread and follow up there.

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

#11 lore raw

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

From: Andrea Arcangeli <[email protected]>
Subject: fork-o_direct-race

Think a thread writing constantly to the last 512bytes of a page, while another thread read and writes to/from the first 512bytes of the page. We can lose O_DIRECT reads (or any other get_user_pages write=1 I/O not just bio/O_DIRECT), the very moment we mark any pte wrprotected because a third unrelated thread forks off a child.

This fixes it by never wprotecting anon ptes if there can be any direct I/O in flight to the page, and by instantiating a readonly pte and triggering a COW in the child. The only trouble here are O_DIRECT reads (writes to memory, read from disk). Checking the page_count under the PT lock guarantees no get_user_pages could be running under us because if somebody wants to write to the page, it has to break any cow first and that requires taking the PT lock in follow_page before increasing the page count. We are guaranteed mapcount is 1 if fork is writeprotecting the pte so the PT lock is enough to serialize against get_user_pages->get_page.

The COW triggered inside fork will run while the parent pte is readonly to provide as usual the per-page atomic copy from parent to child during fork. However timings will be altered by having to copy the pages that might be under O_DIRECT.

The pagevec code calls get_page while the page is sitting in the pagevec (before it becomes PageLRU) and doing so it can generate false positives, so to avoid slowing down fork all the time even for pages that could never possibly be under O_DIRECT write=1, the PG_gup bitflag is added, this eliminates most overhead of the fix in fork.

Patch doesn't break kABI despite introducing a new page flag.

Fixed version of original patch from Nick Piggin.

Signed-off-by Andrea Arcangeli <[email protected]>

---

include/linux/page-flags.h +5 -0
diff -ur 2/include/linux/page-flags.h 1/include/linux/page-flags.h
@@ -86,6 +86,7 @@
 #define PG_reclaim		17	/* To be reclaimed asap */
 #define PG_nosave_free		18	/* Free, should not be written */
 #define PG_buddy		19	/* Page is free, on buddy lists */
+#define PG_gup			20	/* Page pin may be because of gup */
 
 /* PG_owner_priv_1 users should have descriptive aliases */
 #define PG_checked              PG_owner_priv_1 /* Used by some filesystems */

@@ -238,6 +239,10 @@
 #define __SetPageCompound(page)	__set_bit(PG_compound, &(page)->flags)
 #define __ClearPageCompound(page) __clear_bit(PG_compound, &(page)->flags)
 
+#define SetPageGUP(page)	set_bit(PG_gup, &(page)->flags)
+#define PageGUP(page)		test_bit(PG_gup, &(page)->flags)
+#define __ClearPageGUP(page)	__clear_bit(PG_gup, &(page)->flags)
+
 /*
  * PG_reclaim is used in combination with PG_compound to mark the
  * head and tail of a compound page
kernel/fork.c +1 -1
diff -ur 2/kernel/fork.c 1/kernel/fork.c
@@ -368,7 +368,7 @@
 		rb_parent = &tmp->vm_rb;
 
 		mm->map_count++;
-		retval = copy_page_range(mm, oldmm, mpnt);
+		retval = copy_page_range(mm, oldmm, tmp);
 
 		if (tmp->vm_ops && tmp->vm_ops->open)
 			tmp->vm_ops->open(tmp);

Only in 1: ldtest7557.out

mm/memory.c +91 -13

179 lines not on this page · raw message

mm/page_alloc.c +4 -0
diff -ur 2/mm/page_alloc.c 1/mm/page_alloc.c
@@ -154,6 +154,7 @@
 			1 << PG_slab    |
 			1 << PG_swapcache |
 			1 << PG_writeback |
+			1 << PG_gup |
 			1 << PG_buddy );
 	set_page_count(page, 0);
 	reset_page_mapcount(page);

@@ -400,6 +401,8 @@
 		bad_page(page);
 	if (PageDirty(page))
 		__ClearPageDirty(page);
+	if (PageGUP(page))
+		__ClearPageGUP(page);
 	/*
 	 * For now, we report if PG_reserved was found set, but do not
 	 * clear it, and do not free the page.  But we shall soon need

@@ -546,6 +549,7 @@
 			1 << PG_swapcache |
 			1 << PG_writeback |
 			1 << PG_reserved |
+			1 << PG_gup |
 			1 << PG_buddy ))))
 		bad_page(page);

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

#12 lore raw
Quoted 11 lines · On Mon, 2 Feb 2009 23:08:56 +0100 Andrea Arcangeli <[email protected]> wrote:

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

I commented in FJ-Redhat Path but not forwared from unknown reason ;) I comment again.

1. Why TestSetLockPage() is necessary ?
It seems not necesary.

2. This patch doesn't cover HugeTLB.

3. Why "follow_page() successfully finds a page" case only ? not necessary to insert SetPageGUP() in following path ?

- handle_mm_fault()

           => do_anonymos/swap/wp_page()
           or some.

BTW, when you write a patch against upstream, please CC me or linux-mm. I'll have to add a hook for memory-cgroup.

Thanks,
-Kame

Quoted 292 lines

From: Andrea Arcangeli <[email protected]>
Subject: fork-o_direct-race

Think a thread writing constantly to the last 512bytes of a page, while another thread read and writes to/from the first 512bytes of the page. We can lose O_DIRECT reads (or any other get_user_pages write=1 I/O not just bio/O_DIRECT), the very moment we mark any pte wrprotected because a third unrelated thread forks off a child.

This fixes it by never wprotecting anon ptes if there can be any direct I/O in flight to the page, and by instantiating a readonly pte and triggering a COW in the child. The only trouble here are O_DIRECT reads (writes to memory, read from disk). Checking the page_count under the PT lock guarantees no get_user_pages could be running under us because if somebody wants to write to the page, it has to break any cow first and that requires taking the PT lock in follow_page before increasing the page count. We are guaranteed mapcount is 1 if fork is writeprotecting the pte so the PT lock is enough to serialize against get_user_pages->get_page.

The COW triggered inside fork will run while the parent pte is readonly to provide as usual the per-page atomic copy from parent to child during fork. However timings will be altered by having to copy the pages that might be under O_DIRECT.

The pagevec code calls get_page while the page is sitting in the pagevec (before it becomes PageLRU) and doing so it can generate false positives, so to avoid slowing down fork all the time even for pages that could never possibly be under O_DIRECT write=1, the PG_gup bitflag is added, this eliminates most overhead of the fix in fork.

Patch doesn't break kABI despite introducing a new page flag.

Fixed version of original patch from Nick Piggin.

Signed-off-by Andrea Arcangeli <[email protected]>

---

diff -ur 2/include/linux/page-flags.h 1/include/linux/page-flags.h

include/linux/page-flags.h +5 -0
@@ -86,6 +86,7 @@
 #define PG_reclaim		17	/* To be reclaimed asap */
 #define PG_nosave_free		18	/* Free, should not be written */
 #define PG_buddy		19	/* Page is free, on buddy lists */
+#define PG_gup			20	/* Page pin may be because of gup */
 
 /* PG_owner_priv_1 users should have descriptive aliases */
 #define PG_checked              PG_owner_priv_1 /* Used by some filesystems */

@@ -238,6 +239,10 @@
 #define __SetPageCompound(page)	__set_bit(PG_compound, &(page)->flags)
 #define __ClearPageCompound(page) __clear_bit(PG_compound, &(page)->flags)
 
+#define SetPageGUP(page)	set_bit(PG_gup, &(page)->flags)
+#define PageGUP(page)		test_bit(PG_gup, &(page)->flags)
+#define __ClearPageGUP(page)	__clear_bit(PG_gup, &(page)->flags)
+
 /*
  * PG_reclaim is used in combination with PG_compound to mark the
  * head and tail of a compound page

diff -ur 2/kernel/fork.c 1/kernel/fork.c

kernel/fork.c +1 -1
@@ -368,7 +368,7 @@
 		rb_parent = &tmp->vm_rb;
 
 		mm->map_count++;
-		retval = copy_page_range(mm, oldmm, mpnt);
+		retval = copy_page_range(mm, oldmm, tmp);
 
 		if (tmp->vm_ops && tmp->vm_ops->open)
 			tmp->vm_ops->open(tmp);

Only in 1: ldtest7557.out
diff -ur 2/mm/memory.c 1/mm/memory.c

mm/memory.c +91 -13
@@ -426,7 +426,7 @@
  * covered by this vma.
  */
 
-static inline void
+static inline int
 copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		pte_t *dst_pte, pte_t *src_pte, struct vm_area_struct *vma,
 		unsigned long addr, int *rss)

@@ -434,6 +434,7 @@
 	unsigned long vm_flags = vma->vm_flags;
 	pte_t pte = *src_pte;
 	struct page *page;
+	int forcecow = 0;
 
 	/* pte contains position in swap or file, so copy. */
 	if (unlikely(!pte_present(pte))) {

@@ -464,15 +465,6 @@
 	}
 
 	/*
-	 * If it's a COW mapping, write protect it both
-	 * in the parent and the child
-	 */
-	if (is_cow_mapping(vm_flags)) {
-		ptep_set_wrprotect(src_mm, addr, src_pte);
-		pte = *src_pte;
-	}
-
-	/*
 	 * If it's a shared mapping, mark it clean in
 	 * the child
 	 */

@@ -484,13 +476,41 @@
 	if (page) {
 		get_page(page);
 		page_dup_rmap(page);
+		if (is_cow_mapping(vm_flags) && PageAnon(page) &&
+		    PageGUP(page)) {
+			if (unlikely(TestSetPageLocked(page)))
+				forcecow = 1;
+			else {
+				if (unlikely(page_count(page) !=
+					     page_mapcount(page)
+					     + !!PageSwapCache(page)))
+					forcecow = 1;
+				unlock_page(page);
+			}
+		}
 		rss[!!PageAnon(page)]++;
 	}
 
+	/*
+	 * If it's a COW mapping, write protect it both
+	 * in the parent and the child.
+	 */
+	if (is_cow_mapping(vm_flags)) {
+		ptep_set_wrprotect(src_mm, addr, src_pte);
+		pte = pte_wrprotect(pte);
+	}
+
 out_set_pte:
 	set_pte_at(dst_mm, addr, dst_pte, pte);
+
+	return forcecow;
 }
 
+static void fork_pre_cow(struct mm_struct *mm, struct vm_area_struct *vma,
+			 unsigned long address,
+			 pte_t *src_pte, pte_t *dst_pte,
+			 struct page *pre_cow_page);
+
 static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
 		unsigned long addr, unsigned long end)

@@ -499,17 +519,30 @@
 	spinlock_t *src_ptl, *dst_ptl;
 	int progress = 0;
 	int rss[2];
+	int forcecow;
+	struct page *pre_cow_page = NULL;
 
 again:
+	if (!pre_cow_page) {
+		pre_cow_page = alloc_page_vma(GFP_HIGHUSER, vma, addr);
+		if (!pre_cow_page)
+			return -ENOMEM;
+	}
+	forcecow = 0;
 	rss[1] = rss[0] = 0;
 	dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
-	if (!dst_pte)
+	if (!dst_pte) {
+		page_cache_release(pre_cow_page);
 		return -ENOMEM;
+	}
 	src_pte = pte_offset_map_nested(src_pmd, addr);
 	src_ptl = pte_lockptr(src_mm, src_pmd);
 	spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
 
 	do {
+		if (forcecow)
+			break;
+
 		/*
 		 * We are holding two locks at this point - either of them
 		 * could generate latencies in another task on another CPU.

@@ -525,10 +558,26 @@
 			progress++;
 			continue;
 		}
-		copy_one_pte(dst_mm, src_mm, dst_pte, src_pte, vma, addr, rss);
+		forcecow = copy_one_pte(dst_mm, src_mm, dst_pte, src_pte, vma, addr, rss);
 		progress += 8;
 	} while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
 
+	if (unlikely(forcecow)) {
+		/*
+		 * Try to COW the child page as direct I/O is working
+		 * on the parent page, and so we've to mark the parent
+		 * pte read-write before dropping the PT lock to avoid
+		 * the page to be cowed in the parent and any direct
+		 * I/O to get lost.
+		 */
+		fork_pre_cow(dst_mm, vma, addr-PAGE_SIZE,
+			     src_pte-1, dst_pte-1, pre_cow_page);
+		/* after the page copy set the parent pte writeable */
+		set_pte_at(src_mm, addr-PAGE_SIZE, src_pte-1,
+			   pte_mkwrite(*(src_pte-1)));
+		pre_cow_page = NULL;
+	}
+
 	spin_unlock(src_ptl);
 	pte_unmap_nested(src_pte - 1);
 	add_mm_rss(dst_mm, rss[0], rss[1]);

@@ -536,6 +585,8 @@
 	cond_resched();
 	if (addr != end)
 		goto again;
+	if (pre_cow_page)
+		page_cache_release(pre_cow_page);
 	return 0;
 }
 

@@ -956,8 +1007,11 @@
 	if (unlikely(!page))
 		goto unlock;
 
-	if (flags & FOLL_GET)
+	if (flags & FOLL_GET) {
 		get_page(page);
+		if ((flags & FOLL_WRITE) && PageAnon(page) && !PageGUP(page))
+			SetPageGUP(page);
+	}
 	if (flags & FOLL_TOUCH) {
 		if ((flags & FOLL_WRITE) &&
 		    !pte_dirty(pte) && !PageDirty(page))

@@ -1607,6 +1661,30 @@
 	copy_user_highpage(dst, src, va);
 }
 
+static void fork_pre_cow(struct mm_struct *mm, struct vm_area_struct *vma,
+			 unsigned long address,
+			 pte_t *src_pte, pte_t *dst_pte,
+			 struct page *pre_cow_page)
+{
+	pte_t entry;
+	struct page *old_page;
+
+	old_page = vm_normal_page(vma, address, *src_pte);
+	BUG_ON(!old_page);
+	cow_user_page(pre_cow_page, old_page, address);
+	page_remove_rmap(old_page);
+	page_cache_release(old_page);
+
+	flush_cache_page(vma, address, pte_pfn(*src_pte));
+	entry = mk_pte(pre_cow_page, vma->vm_page_prot);
+	entry = maybe_mkwrite(pte_mkdirty(entry), vma);
+	page_add_new_anon_rmap(pre_cow_page, vma, address);
+	lru_cache_add_active(pre_cow_page);
+	set_pte_at(mm, address, dst_pte, entry);
+	update_mmu_cache(vma, address, entry);
+	lazy_mmu_prot_update(entry);
+}
+
 /*
  * This routine handles present pages, when users try to write
  * to a shared page. It is done by copying the page to a new address

diff -ur 2/mm/page_alloc.c 1/mm/page_alloc.c

mm/page_alloc.c +4 -0
@@ -154,6 +154,7 @@
 			1 << PG_slab    |
 			1 << PG_swapcache |
 			1 << PG_writeback |
+			1 << PG_gup |
 			1 << PG_buddy );
 	set_page_count(page, 0);
 	reset_page_mapcount(page);

@@ -400,6 +401,8 @@
 		bad_page(page);
 	if (PageDirty(page))
 		__ClearPageDirty(page);
+	if (PageGUP(page))
+		__ClearPageGUP(page);
 	/*
 	 * For now, we report if PG_reserved was found set, but do not
 	 * clear it, and do not free the page.  But we shall soon need

@@ -546,6 +549,7 @@
 			1 << PG_swapcache |
 			1 << PG_writeback |
 			1 << PG_reserved |
+			1 << PG_gup |
 			1 << PG_buddy ))))
 		bad_page(page);
#13 lore raw
Quoted 12 lines · KAMEZAWA Hiroyuki
Quoted 9 lines · On Mon, 2 Feb 2009 23:08:56 +0100 Andrea Arcangeli…

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Quoted 7 lines
Quoted 2 lines

Please help testing the below on pre-gup-fast kernels, thanks!

I commented in FJ-Redhat Path but not forwared from unknown reason ;) I comment again.

1. Why TestSetLockPage() is necessary ?
It seems not necesary.

To avoid the VM to remove or add the page from/to swapcache and change page_count/mapcount from under us. This most certainly wasn't the reason of the slowdown (the slowdown were the false positives generated by pagevec pinning) and removing it was more intrusive than I wanted.

Quoted 1 line

2. This patch doesn't cover HugeTLB.

There's no need to change hugetlb with my approach. I'm not touching the cow path, I'm addressing the real source of the problem (i.e. when fork pretends to mark the child pte readonly and pointing to the shared parent page, same as ksm: while the pte wrprotect + tlb flush stops the _cpu_ it can't stop any get_user_pages(write=1) user, hence we need to pre-cow the child page in fork instead of marking the child pte readonly to avoid the parent to lose writes if post-fork the parent cows and the child doesn't cow).

Quoted 6 lines

3. Why "follow_page() successfully finds a page" case only ? not necessary to insert SetPageGUP() in following path ?

- handle_mm_fault()

           => do_anonymos/swap/wp_page()
           or some.

No need to change that either, all we need to know are the pages whose count vs mapcount has a discrepancy that could have been caused by get_user_pages. So only follow_page has to set it. More precisely FOLL_GET|FOLL_WRITE is the only path we care about there.

Quoted 2 lines

BTW, when you write a patch against upstream, please CC me or linux-mm. I'll have to add a hook for memory-cgroup.

Sure.

BTW, despite I didn't reproduce the problem here while leaving the ./dma_thread -a 512 -w 40 workload run half a day, others reported me trouble but it was on a different kernel codebase, but at this time I'm unsure if any remaining trouble is caused by some imperfection in this patch or something else. Test results would be interesting basically. Patch is against rhel-5.2 but should be trivial to apply to anything pre-get_user_pages_fast.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw
Quoted 18 lines · On Tue, 3 Feb 2009 03:31:47 +0100 Andrea Arcangeli…
Quoted 17 lines · KAMEZAWA Hiroyuki
Quoted 11 lines · On Mon, 2 Feb 2009 23:08:56 +0100 Andrea Arcangeli…

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

I commented in FJ-Redhat Path but not forwared from unknown reason ;) I comment again.

Quoted 8 lines
Quoted 2 lines

1. Why TestSetLockPage() is necessary ?
It seems not necesary.

To avoid the VM to remove or add the page from/to swapcache and change page_count/mapcount from under us. This most certainly wasn't the reason of the slowdown (the slowdown were the false positives generated by pagevec pinning) and removing it was more intrusive than I wanted.

My point is.
- If TestSetLockPage() failes, force_cow=1.
- If count/mapcount check fails, force_cow=1.

So, lock_page() here seems meaningless. If you consider lock_page() is important, just use lock_page() seems better.

Quoted 12 lines
Quoted 2 lines

2. This patch doesn't cover HugeTLB.

There's no need to change hugetlb with my approach. I'm not touching the cow path, I'm addressing the real source of the problem (i.e. when fork pretends to mark the child pte readonly and pointing to the shared parent page, same as ksm: while the pte wrprotect + tlb flush stops the _cpu_ it can't stop any get_user_pages(write=1) user, hence we need to pre-cow the child page in fork instead of marking the child pte readonly to avoid the parent to lose writes if post-fork the parent cows and the child doesn't cow).

No need to make a patch for copy_hugetlb_page_range() ? IMHO, HugeTLB can be write-protected at fork().

Quoted 12 lines
Quoted 6 lines

3. Why "follow_page() successfully finds a page" case only ? not necessary to insert SetPageGUP() in following path ?

- handle_mm_fault()

           => do_anonymos/swap/wp_page()
           or some.

No need to change that either, all we need to know are the pages whose count vs mapcount has a discrepancy that could have been caused by get_user_pages. So only follow_page has to set it. More precisely FOLL_GET|FOLL_WRITE is the only path we care about there.

Assume 3 threads in a process.
==
Thread1 (DIO-Read) Thread2 Thread3
get_user_page()
=> handle_mm_fault().
=> map a page with no-write-protect.
fork()
(write-protect here)
Copy-On-Write
endio.

pre-cow-at-fork will never happen becasue PageGUP is not set. After the end of READ, this process will see a broken page.

Thanks,
-Kame

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

#15 lore raw
Quoted 30 lines · On Tue, 3 Feb 2009 11:55:40 +0900 KAMEZAWA Hiroyuki…
Quoted 18 lines · On Tue, 3 Feb 2009 03:31:47 +0100 Andrea Arcangeli…
Quoted 17 lines · KAMEZAWA Hiroyuki
Quoted 11 lines · On Mon, 2 Feb 2009 23:08:56 +0100 Andrea Arcangeli…

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

I commented in FJ-Redhat Path but not forwared from unknown reason ;) I comment again.

Quoted 8 lines
Quoted 2 lines

1. Why TestSetLockPage() is necessary ?
It seems not necesary.

To avoid the VM to remove or add the page from/to swapcache and change page_count/mapcount from under us. This most certainly wasn't the reason of the slowdown (the slowdown were the false positives generated by pagevec pinning) and removing it was more intrusive than I wanted.

Quoted 7 lines

My point is.
- If TestSetLockPage() failes, force_cow=1.
- If count/mapcount check fails, force_cow=1.

So, lock_page() here seems meaningless. If you consider lock_page() is important, just use lock_page() seems better.

I changed my thinking. I understood *lock* is necessary here. But I'm not sure this is enough.

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

3. Why "follow_page() successfully finds a page" case only ? not necessary to insert SetPageGUP() in following path ?

- handle_mm_fault()

           => do_anonymos/swap/wp_page()
           or some.
Quoted 7 lines
Quoted 5 lines

No need to change that either, all we need to know are the pages whose count vs mapcount has a discrepancy that could have been caused by get_user_pages. So only follow_page has to set it. More precisely FOLL_GET|FOLL_WRITE is the only path we care about there.

Assume 3 threads in a process.

please ignore this, I'm shame ;(

-Kame

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

#16 lore raw
In reply to KAMEZAWA Hiroyuki
Quoted 2 lines · KAMEZAWA Hiroyuki

No need to make a patch for copy_hugetlb_page_range() ? IMHO, HugeTLB can be write-protected at fork().

Yeah that was entirely correct, thanks Kame! I reproduced it with libhugetlbfs and verified it fixed it now.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#17 lore raw
In reply to Andrea Arcangeli
Quoted 10 lines · Andrea Arcangeli

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

Thanks a lot for the patch, I'll try this out tomorrow.

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

#18 lore raw
Quoted 5 lines · Greg KH
Quoted 4 lines · Andrea Arcangeli

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

Quoted 8 lines
Quoted 6 lines

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

Thanks a lot for the patch, I'll try this out tomorrow.

before testing I recommend to make this change:

- if ((flags & FOLL_WRITE) && PageAnon(page) && !PageGUP(page))
+ if (PageAnon(page) && !PageGUP(page))

If gup triggers cow, gup clears FOLL_WRITE from foll_flags before calling follow_page again, I think that's why it didn't work right for everyone, it just couldn't work right if FOLL_WRITE was going away before the last follow_page run. I wanted to set PG_gup only for gup(write=1) and not for gup(write=0) but that optimization that clears FOLL_WRITE prevents the above to work right, so the simple solution is to always set PG_gup, it won't make a big difference.

I'm looking at the hugetlb cow before posting an update...
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#19 lore raw
In reply to Andrea Arcangeli
Quoted 41 lines · On Mon, 2 Feb 2009 23:08:56 +0100 Andrea Arcangeli…

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

From: Andrea Arcangeli <[email protected]>
Subject: fork-o_direct-race

Think a thread writing constantly to the last 512bytes of a page, while another thread read and writes to/from the first 512bytes of the page. We can lose O_DIRECT reads (or any other get_user_pages write=1 I/O not just bio/O_DIRECT), the very moment we mark any pte wrprotected because a third unrelated thread forks off a child.

This fixes it by never wprotecting anon ptes if there can be any direct I/O in flight to the page, and by instantiating a readonly pte and triggering a COW in the child. The only trouble here are O_DIRECT reads (writes to memory, read from disk). Checking the page_count under the PT lock guarantees no get_user_pages could be running under us because if somebody wants to write to the page, it has to break any cow first and that requires taking the PT lock in follow_page before increasing the page count. We are guaranteed mapcount is 1 if fork is writeprotecting the pte so the PT lock is enough to serialize against get_user_pages->get_page.

The COW triggered inside fork will run while the parent pte is readonly to provide as usual the per-page atomic copy from parent to child during fork. However timings will be altered by having to copy the pages that might be under O_DIRECT.

The pagevec code calls get_page while the page is sitting in the pagevec (before it becomes PageLRU) and doing so it can generate false positives, so to avoid slowing down fork all the time even for pages that could never possibly be under O_DIRECT write=1, the PG_gup bitflag is added, this eliminates most overhead of the fix in fork.

Quoted 7 lines

Patch doesn't break kABI despite introducing a new page flag.

Fixed version of original patch from Nick Piggin.

Signed-off-by Andrea Arcangeli <[email protected]>

---

Just curious, could you confirm following ?

(following is from RHEL5.3)

It seems page_cache_release() is called before page_cache_get(). Doesn't this break your assumption ?
(I mean following code is buggy.)
Do we have extra page_count() somewhere before page_cache_release() ?

=
667 submit_page_section(struct dio *dio, struct page *page,
668 unsigned offset, unsigned len, sector_t blocknr)
669 {
670 int ret = 0;
671
672 /*
673 * Can we just grow the current page's presence in the dio?
674 */

 675         if (    (dio->cur_page == page) &&
 676                 (dio->cur_page_offset + dio->cur_page_len == offset) &&
 677                 (dio->cur_page_block +
 678                         (dio->cur_page_len >> dio->blkbits) == blocknr)) {
 679                 dio->cur_page_len += len;
 680 
 681                 /*
 682                  * If dio->boundary then we want to schedule the IO now to
 683                  * avoid metadata seeks.
 684                  */
 685                 if (dio->boundary) {
 686                         ret = dio_send_cur_page(dio);
 687                         page_cache_release(dio->cur_page);
 688                         dio->cur_page = NULL;
 689                 }
 690                 goto out;
 691         }
 692 
 693         /*
 694          * If there's a deferred page already there then send it.
 695          */
 696         if (dio->cur_page) {
 697                 ret = dio_send_cur_page(dio);
 698                 page_cache_release(dio->cur_page);
 699                 dio->cur_page = NULL;
 700                 if (ret)
 701                         goto out;
 702         }
 703 
 704         page_cache_get(page);           /* It is in dio */
 705         dio->cur_page = page;
 706         dio->cur_page_offset = offset;
 707         dio->cur_page_len = len;
 708         dio->cur_page_block = blocknr;

709 out:
710 return ret;
711 }
==

Regards,
-Kame

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

#20 lore raw
In reply to Andrea Arcangeli
Quoted 41 lines · On Mon, 2 Feb 2009 23:08:56 +0100 Andrea Arcangeli…

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

From: Andrea Arcangeli <[email protected]>
Subject: fork-o_direct-race

Think a thread writing constantly to the last 512bytes of a page, while another thread read and writes to/from the first 512bytes of the page. We can lose O_DIRECT reads (or any other get_user_pages write=1 I/O not just bio/O_DIRECT), the very moment we mark any pte wrprotected because a third unrelated thread forks off a child.

This fixes it by never wprotecting anon ptes if there can be any direct I/O in flight to the page, and by instantiating a readonly pte and triggering a COW in the child. The only trouble here are O_DIRECT reads (writes to memory, read from disk). Checking the page_count under the PT lock guarantees no get_user_pages could be running under us because if somebody wants to write to the page, it has to break any cow first and that requires taking the PT lock in follow_page before increasing the page count. We are guaranteed mapcount is 1 if fork is writeprotecting the pte so the PT lock is enough to serialize against get_user_pages->get_page.

The COW triggered inside fork will run while the parent pte is readonly to provide as usual the per-page atomic copy from parent to child during fork. However timings will be altered by having to copy the pages that might be under O_DIRECT.

The pagevec code calls get_page while the page is sitting in the pagevec (before it becomes PageLRU) and doing so it can generate false positives, so to avoid slowing down fork all the time even for pages that could never possibly be under O_DIRECT write=1, the PG_gup bitflag is added, this eliminates most overhead of the fix in fork.

Quoted 6 lines

Patch doesn't break kABI despite introducing a new page flag.

Fixed version of original patch from Nick Piggin.

Signed-off-by Andrea Arcangeli <[email protected]>

---

Sorry, one more ;)
==
1117 cond_resched();
1118 while (!(page = follow_page(vma, start, foll_flags))) {
1119 int ret;
1120 ret = __handle_mm_fault(mm, vma, start,
1121 foll_flags & FOLL_WRITE);
1122 /*

1123                                  * The VM_FAULT_WRITE bit tells us that do_wp_page has
1124                                  * broken COW when necessary, even if maybe_mkwrite
1125                                  * decided not to set pte_write. We can thus safely do
1126                                  * subsequent page lookups as if they were reads.
1127                                  */

1128 if (ret & VM_FAULT_WRITE)
1129 foll_flags &= ~FOLL_WRITE;
==

Quoted 1 line

From above, FOLL_WRITE can be dropped and PageGUP() will not be set ?

Thanks,
-Kame

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

#21 lore raw
Quoted 1 line · KAMEZAWA Hiroyuki

From above, FOLL_WRITE can be dropped and PageGUP() will not be set ?

Yeah, this was an implementation trouble!! If cow triggered, PG_gup wasn't getting set as it should have.

The only remaining bit is to check hugetlb pages...
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#22 lore raw
In reply to Andrea Arcangeli
Quoted 10 lines · Andrea Arcangeli

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Please help testing the below on pre-gup-fast kernels, thanks!

What kernel version was this patch against? I can't seem to apply it to anything close to mainline :(

I tried 2.6.29-rc3, 2.6.28-stable and 2.6.27-stable.

thanks,

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

#23 lore raw
Quoted 10 lines · Greg KH
Quoted 9 lines · Andrea Arcangeli

Hi Greg!

Quoted 1 line

Thanks for the pointers, I'll go read the thread and follow up there.

If you also run into this final fix is attached below. Porting to mainline is a bit hard because of gup-fast... Perhaps we can use mmu notifiers to fix gup-fast... need to think more about it then I'll post something.

Quoted 6 lines
Quoted 1 line

Please help testing the below on pre-gup-fast kernels, thanks!

What kernel version was this patch against? I can't seem to apply it to anything close to mainline :(

I tried 2.6.29-rc3, 2.6.28-stable and 2.6.27-stable.

Ah patch is against 2.6.18, it's a "production" targeted patch as this race is hitting production systems. So this is the final update addressing hugetlb too. Kame was totally right hugetlb has the same bug and I reproduced it trivially with:

LD_PRELOAD=/usr/lib64/libhugetlbfs.so HUGETLB_MORECORE=yes HUGETLB_PATH=/mnt/huge/ ./dma_thread -a 512 -w 40 LD_PRELOAD=/usr/lib64/libhugetlbfs.so HUGETLB_MORECORE=yes HUGETLB_PATH=/mnt/huge/ ./forkscrew

I verfied hugetlb pages were allocated in grep Huge /proc/meminfo and as well through printk in the hugetlb 'forcecow' path to be sure it run and fixed the race. With new patch the bug goes away for both largepages and regular anon pages.

I also exercised the -EAGAIN path and verfied it rollbacks and restart on the prev pte just fine, it only happens during very heavy paging activity. I unlock/relock around the page-copy to be more lowlatency schedule friendly and to move the page allocation out of the fork fast path.

Here the latest patch. Now that I consider the 'production' trouble closed I'll be porting it to mainline while addressing gup-fast too which is an additional complication I didn't have to take care of yet. So expect a patch that works for you in the next few days, either that or complains about an unfixable gup-fast ;). But frankly I've been thinking it should be possible in a much simpler way that I ever thought before, by entirely relaying on the tlb flush.

In short if I simply do the page-count vs page-mapcount check _after_ ptep_set_wrprotect (which implies a tlb flush and after that any gup-fast running in other cpus should be forced through the slow path and block) I think I'm done. The code now does:

check mapcount
ptep_set_wrprotect

I think to make the thing working with gup-fast I've only to ptep_set_wrprotect before the mapcount check.

The reason why the normal pagetable walking of the cpu is ok with current code is that ptep_set_wrprotect done later will block any access to the page from the other cpus. Not so if it was gup-fast taking reference on the page. So we need to stop with ptep_set_wrprotect any subsequential gup-fast _before_ we check count vs mapcount and the fact the get_page is run inside the critical section with local irq disabled in gup-fast should close the race for gup-fast too. Will try that ASAP...

With Hugh I'm also reviewing the old 2.4 bug where a thread was doing cow on swapcache while another thread was starting O_DIRECT, as I recall to be 100% safe I wanted ptes pointing to pages under O_DIRECT not to be unmapped by rmap, did that in 2.4 with a PG_pinned to tell vmscan.c to bailout on the page. In 2.6 I wanted to do it with mapcount but I think some alternate fix went in, so given I'm at it I'll review the correctness of that too, just in case ;).

--------
From: Andrea Arcangeli <[email protected]>
Subject: fork-o_direct-race

Think a thread writing constantly to the last 512bytes of a page, while another thread read and writes to/from the first 512bytes of the page. We can lose O_DIRECT reads (or any other get_user_pages write=1 I/O not just bio/O_DIRECT), the very moment we mark any pte wrprotected because a third unrelated thread forks off a child.

This fixes it by never wprotecting anon ptes if there can be any direct I/O in flight to the page, and by instantiating a readonly pte and triggering a COW in the child. The only trouble here are O_DIRECT reads (writes to memory, read from disk). Checking the page_count under the PT lock guarantees no get_user_pages could be running under us because if somebody wants to write to the page, it has to break any cow first and that requires taking the PT lock in follow_page before increasing the page count. We are guaranteed mapcount is 1 if fork is writeprotecting the pte so the PT lock is enough to serialize against get_user_pages->get_page.

The COW triggered inside fork will run while the parent pte is readonly to provide as usual the per-page atomic copy from parent to child during fork. However timings will be altered by having to copy the pages that might be under O_DIRECT.

The pagevec code calls get_page while the page is sitting in the pagevec (before it becomes PageLRU) and doing so it can generate false positives, so to avoid slowing down fork all the time even for pages that could never possibly be under O_DIRECT write=1, the PG_gup bitflag is added, this eliminates most overhead of the fix in fork.

Patch doesn't break kABI despite introducing a new page flag.

Fixed version of original patch from Nick Piggin.

Signed-off-by Andrea Arcangeli <[email protected]>

---

include/linux/page-flags.h +5 -0
index 63c8e4c..6105cad 100644
@@ -86,6 +86,7 @@
 #define PG_reclaim		17	/* To be reclaimed asap */
 #define PG_nosave_free		18	/* Free, should not be written */
 #define PG_buddy		19	/* Page is free, on buddy lists */
+#define PG_gup			20	/* Page pin may be because of gup */
 #define PG_xpmem		27	/* Testing for xpmem. */
 
 /* PG_owner_priv_1 users should have descriptive aliases */

@@ -239,6 +240,10 @@
 #define __SetPageCompound(page)	__set_bit(PG_compound, &(page)->flags)
 #define __ClearPageCompound(page) __clear_bit(PG_compound, &(page)->flags)
 
+#define SetPageGUP(page)	set_bit(PG_gup, &(page)->flags)
+#define PageGUP(page)		test_bit(PG_gup, &(page)->flags)
+#define __ClearPageGUP(page)	__clear_bit(PG_gup, &(page)->flags)
+
 /*
  * PG_reclaim is used in combination with PG_compound to mark the
  * head and tail of a compound page
kernel/fork.c +1 -1
index f038aff..c29da4d 100644
@@ -384,7 +384,7 @@ static inline int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 		rb_parent = &tmp->vm_rb;
 
 		mm->map_count++;
-		retval = copy_page_range(mm, oldmm, mpnt);
+		retval = copy_page_range(mm, oldmm, tmp);
 
 		if (tmp->vm_ops && tmp->vm_ops->open)
 			tmp->vm_ops->open(tmp);
mm/hugetlb.c +46 -10

111 lines not on this page · raw message

mm/memory.c +126 -12

210 lines not on this page · raw message

mm/page_alloc.c +4 -0
index 3eed821..5bd4b7f 100644
@@ -154,6 +154,7 @@ static void bad_page(struct page *page)
 			1 << PG_slab    |
 			1 << PG_swapcache |
 			1 << PG_writeback |
+			1 << PG_gup |
 			1 << PG_buddy );
 	set_page_count(page, 0);
 	reset_page_mapcount(page);

@@ -400,6 +401,8 @@ static inline int free_pages_check(struct page *page)
 		bad_page(page);
 	if (PageDirty(page))
 		__ClearPageDirty(page);
+	if (PageGUP(page))
+		__ClearPageGUP(page);
 	/*
 	 * For now, we report if PG_reserved was found set, but do not
 	 * clear it, and do not free the page.  But we shall soon need

@@ -546,6 +549,7 @@ static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
 			1 << PG_swapcache |
 			1 << PG_writeback |
 			1 << PG_reserved |
+			1 << PG_gup |
 			1 << PG_buddy ))))
 		bad_page(page);

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

#24 lore raw

mm/memory.c copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 503

Quoted 5 lines · Andrea Arcangeli
mm/memory.c +5 -0
+	if (is_cow_mapping(vm_flags)) {
+		if (pte_write(pte)) {
+			ptep_set_wrprotect(src_mm, addr, src_pte);
+			pte = pte_wrprotect(pte);
+		}

While working on the mainline version that will definitely require a tlb flush here if forcecow/PageGUP is set, I just realized to provide an atomic per-page copy in the fork_pre_cow an explicit tlb flush is needed in the pre-gup-fast version too.

	if (is_cow_mapping(vm_flags)) {
		if (pte_write(pte)) {
			ptep_set_wrprotect(src_mm, addr, src_pte);
			pte = pte_wrprotect(pte);
			if (forcecow)
				flush_tlb_page(src_vma, addr);
		}
	}

However to flush the 'src_mm' I feel I can't pass the 'dst_vma' that fork is passing to copy_page_range. OTOH the dst_vma is needed to be passed to the fork_pre_cow which is why fork.c was changed in the patch to pass dst_vma instead of src_vma.

So I think I want to avoid all further confusion if the 'vma' belongs to the src_mm or the dst_mm by passing both src_vma, and dst_vma from fork to copy_page_tables. In the pre-gup-fast version the tlb flush is mostly a nitpick and it would never lead to any practical issue, but for mostly theoretical reasons it may be good idea to have the per-page atomic copy there too, comments?
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#25 lore raw
In reply to Andrea Arcangeli
Quoted 20 lines · Andrea Arcangeli

Here the latest patch. Now that I consider the 'production' trouble closed I'll be porting it to mainline while addressing gup-fast too which is an additional complication I didn't have to take care of yet. So expect a patch that works for you in the next few days, either that or complains about an unfixable gup-fast ;). But frankly I've been thinking it should be possible in a much simpler way that I ever thought before, by entirely relaying on the tlb flush.

In short if I simply do the page-count vs page-mapcount check _after_ ptep_set_wrprotect (which implies a tlb flush and after that any gup-fast running in other cpus should be forced through the slow path and block) I think I'm done. The code now does:

check mapcount
ptep_set_wrprotect

I think to make the thing working with gup-fast I've only to ptep_set_wrprotect before the mapcount check.

Quoted 10 lines

The reason why the normal pagetable walking of the cpu is ok with current code is that ptep_set_wrprotect done later will block any access to the page from the other cpus. Not so if it was gup-fast taking reference on the page. So we need to stop with ptep_set_wrprotect any subsequential gup-fast _before_ we check count vs mapcount and the fact the get_page is run inside the critical section with local irq disabled in gup-fast should close the race for gup-fast too. Will try that ASAP...

Hi Andrea,
Good approach, but I think that it isn't enough to just change the order of check mapcount and ptep_set_wrprotect(), the reason is that gup_fast do the get_page() AFTER it fetched the pte, so we are opening here a tiny race:

cpu#1 do get_user_pages_fast and fetch the pte (it think the pte is writeable) cpu#2 do ptep_set_wrprotect()
cpu#2 check the mapcount against pagecount (it think that everything is fine and continue) cpu#1 only now do get_page()

Anyway this is minor issue that can be probably solved by just:
rechecking if the pte isnt read_only in gup_fast after we do the get_page()

Anyway sound like a great idea to fix this issue!
good work.

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

#26 lore raw
Quoted 11 lines · Izik Eidus

we are opening here a tiny race:

cpu#1 do get_user_pages_fast and fetch the pte (it think the pte is writeable)
cpu#2 do ptep_set_wrprotect()
cpu#2 check the mapcount against pagecount (it think that everything is fine and continue)
cpu#1 only now do get_page()

Anyway this is minor issue that can be probably solved by just:
rechecking if the pte isnt read_only in gup_fast after we do the get_page()

Not needed, if I check page_count vs mapcount after marking the pte readonly and after sending smp-tlb-flush there is no race.

Quoted 1 line

Anyway sound like a great idea to fix this issue!

The only problem I'm thinking now is the IPI flood that would be generated if I send IPIs for every pte wrprotected in fork, that sounds overkill. So to use the IPI fix I could have gup-fast take the slow path first time around if PG_gup isn't set, and then only second time take the lockless fast path when PG_gup is already set (PG_gup gets set by follow_page under PT lock/mmap_sem read mode at least). And fork/ksm would only send IPIs for PG_gup pages. However that would make gup-fast slow the first time it runs on an anonymous/hugetlb page with pte marked writeable and I'm unsure if that's ok.

Otherwise we've to return to the plan of the slightly more complicated fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html