hugetlbfs: add llseek method

PATCH In review Christoph Hellwig 2 participants 5 messages Sep 8, 2008 – Sep 25, 2008

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

Christoph Hellwig

author

2008-09-08 17:46 UTC #0 lore raw

PATCH 1/1 hugetlbfs: add llseek method

Hugetlbfs currently doesn't set a llseek method for regular files, which means it will fall back to default_llseek. This means no one can seek beyond 2 Gigabytes.

Signed-off-by Christoph Hellwig <[email protected]>

fs/hugetlbfs/inode.c +1 -0

Index: linux-2.6/fs/hugetlbfs/inode.c
===================================================================
@@ -717,6 +717,7 @@ const struct file_operations hugetlbfs_f
 	.mmap			= hugetlbfs_file_mmap,
 	.fsync			= simple_sync_file,
 	.get_unmapped_area	= hugetlb_get_unmapped_area,
+	.llseek			= generic_file_llseek,
 };
 
 static const struct inode_operations hugetlbfs_dir_inode_operations = {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2008-09-22 07:58 UTC #1 lore raw

On (08/09/08 19:46), Christoph Hellwig didst pronounce:

Quoted 5 lines

Hugetlbfs currently doesn't set a llseek method for regular files, which means it will fall back to default_llseek. This means no one can seek beyond 2 Gigabytes.
Signed-off-by Christoph Hellwig <[email protected]>

I am probably missing something embarassingly simple. My reading of this description made me assume lseek64() must be broken so why does the following test not fail?

/*
* This test program writes numbers throughout a large hugetlb-backed file
* and checks if lseek64 can be used to read beyond the 2GB file boundary.
* mmap() is used to write the file as the write method is not supported on
* hugetlbfs. To test the program
* 1. mount -t hugetlbfs none /mnt
* 2. echo NR_HUGE > /proc/sys/vm/nr_hugepages
* 3. gcc -g -Wall hugetlbfs-llseek-test.c -o hugetlbfs-llseek-test && ./hugetlbfs-llseek-test *
* where NR_HUGE would be 16 on POWER, 64 with 4M huge pagesize and 128 with
* 2MB huge pagesize
*/
#define _LARGEFILE64_SOURCE
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <fcntl.h>

#define FILE_NAME "/mnt/hugepagefile"
#define QGB (256UL*1024UL*1024UL)
#define NR_QGB 16
#define MAP_LENGTH (16UL*1024*1024)
#define PROTECTION (PROT_READ | PROT_WRITE)

/* Only ia64 requires this */
#ifdef __ia64__
#define ADDR (void *)(0x8000000000000000UL) #define FLAGS (MAP_SHARED | MAP_FIXED | MAP_NORESERVE) #else
#define ADDR (void *)(0x0UL)
#define FLAGS (MAP_SHARED)
#endif

int main(int argc, char **argv)

{
	char *addr;
	int fd, i;

	/*
	 * Open a hugetlbfs-backed file and then unlink it so we can exit
	 * and have the file automatically cleared up
	 */
	printf("Opening file " FILE_NAME "\n");
	fd = open(FILE_NAME, O_CREAT | O_RDWR, 0755);
	if (fd < 0) {
		perror("Open failed");
		exit(1);
	}
	unlink(FILE_NAME);

	/* Write a known value every quarter gigabyte of the file */
	printf("Attemping write mmap: ");
	for (i = 0; i < NR_QGB; i++) {
		off_t offset = i * QGB;

		addr = mmap(ADDR, MAP_LENGTH, PROTECTION, FLAGS, fd, offset);
		if (addr == MAP_FAILED) {
			printf("FAIL mmap %d\n", i);
			perror("mmap");
			exit(-1);
		}
		*addr = i;
		munmap(addr, MAP_LENGTH);
	}
	printf(" PASS\n");

	/* Attempt to read the values back using lseek64 */
	printf("Attemping read seek: ");
	for (i = 0; i < NR_QGB; i++) {
		char val = 0;
		off64_t offset = i * QGB;

		printf("%d ", i);
		if (lseek64(fd, offset, SEEK_SET) != offset) {
			printf("FAIL seek %llu\n", offset);
			perror("lseek");
			exit(-1);
		}

		if (read(fd, &val, 1) == -1 || val != i) {
			printf("FAIL read %d != %d\n", val, i);
			perror("read");
			exit(-1);
		}

	}
	printf("\nTest passed successfully\n");

	close(fd);
	return 0;
}

Mel Gorman

2008-09-22 18:56 UTC #2 lore raw

In reply to Christoph Hellwig

On (08/09/08 19:46), Christoph Hellwig didst pronounce:

Quoted 4 lines

Hugetlbfs currently doesn't set a llseek method for regular files, which means it will fall back to default_llseek. This means no one can seek beyond 2 Gigabytes.

I took another look at this as it was pointed out to me by apw that this might be a SEEK_CUR vs SEEK_SET thing and also whether lseek() was the key. To use lseek though, the large file defines had to be used or it failed whether your patch was applied or not. The error as you'd expect is lseek() complaining that the type was too small.

At the face of it, the patch seems sensible but it works whether it is set or not so clearly I'm still missing something. The second test I tried is below. In the unlikely event it makes a difference, I was testing on qemu for i386.

/*
* This test program writes numbers throughout a large hugetlb-backed file
* and checks if lseek64 can be used to read beyond the 2GB file boundary.
* mmap() is used to write the file as the write method is not supported on
* hugetlbfs. To test the program
* 1. mount -t hugetlbfs none /mnt
* 2. echo NR_HUGE > /proc/sys/vm/nr_hugepages
* 3. gcc -g -Wall hugetlbfs-llseek-test.c -o hugetlbfs-llseek-test && ./hugetlbfs-llseek-test *
* where NR_HUGE would be 16 on POWER, 64 with 4M huge pagesize and 128 with
* 2MB huge pagesize
*/
#define _FILE_OFFSET_BITS 64
#define _LARGEFILE64_SOURCE
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/mman.h>
#include <fcntl.h>

#define FILE_NAME "/mnt/hugepagefile"
#define QGB (256UL*1024UL*1024UL)
#define NR_QGB 15
#define MAP_LENGTH (16UL*1024*1024)
#define PROTECTION (PROT_READ | PROT_WRITE)

/* Only ia64 requires this */
#ifdef __ia64__
#define ADDR (void *)(0x8000000000000000UL)
#define FLAGS (MAP_SHARED | MAP_FIXED)
#else
#define ADDR (void *)(0x0UL)
#define FLAGS (MAP_SHARED)
#endif

int main(int argc, char **argv)

{
	char *addr;
	int fd, i;

	/*
	 * Open a hugetlbfs-backed file and then unlink it so we can exit
	 * and have the file cleared up
	 */
	printf("Opening file " FILE_NAME "\n");
	fd = open(FILE_NAME, O_CREAT | O_RDWR | O_LARGEFILE, 0755);
	if (fd < 0) {
		perror("Open failed");
		exit(1);
	}
	unlink(FILE_NAME);

	/* Write a known value every quarter gigabyte of the file */
	printf("Attemping write mmap: ");
	for (i = 0; i < NR_QGB; i++) {
		off_t offset = i * QGB;

		addr = mmap(ADDR, MAP_LENGTH, PROTECTION, FLAGS, fd, offset);
		if (addr == MAP_FAILED) {
			printf("FAIL mmap %d\n", i);
			perror("mmap");
			exit(-1);
		}
		*addr = i;
		munmap(addr, MAP_LENGTH);
	}
	printf(" PASS\n");

	/* Attempt to read the values back using lseek64 */
	printf("Attemping read seek: ");
	for (i = 0; i < NR_QGB; i++) {
		char val = 0;
		off_t offset = (i + 1) * QGB;
		off_t ret;

		printf("%d ", i);
		if (read(fd, &val, 1) == -1 || val != i) {
			printf("FAIL read %d != %d\n", val, i);
			perror("read");
			exit(-1);
		}

		ret = lseek(fd, QGB - 1, SEEK_CUR);
		if (ret != offset) {
			printf("FAIL seek %llu != %llu\n", offset, ret);
			perror("lseek");
			exit(-1);
		}

	}
	printf("\nTest passed successfully\n");

	close(fd);
	return 0;
}

Christoph Hellwig

author

2008-09-24 19:00 UTC #3 lore raw

Quoted 12 lines · Mel Gorman

On (08/09/08 19:46), Christoph Hellwig didst pronounce:
Quoted 4 lines
Hugetlbfs currently doesn't set a llseek method for regular files, which means it will fall back to default_llseek. This means no one can seek beyond 2 Gigabytes.
I took another look at this as it was pointed out to me by apw that this might be a SEEK_CUR vs SEEK_SET thing and also whether lseek() was the key. To use lseek though, the large file defines had to be used or it failed whether your patch was applied or not. The error as you'd expect is lseek() complaining that the type was too small.

Quoted 4 lines

At the face of it, the patch seems sensible but it works whether it is set or not so clearly I'm still missing something. The second test I tried is below. In the unlikely event it makes a difference, I was testing on qemu for i386.

Sorry, my original description was complete bullsh*t ;-) The problem is the inverse of what I wrote. With default_llseek you can seek everywhere even if that's outside of the fs limit. This should give you quite interesting results if you seek outside of what we can represent page->index on 32bit platforms.

Mel Gorman

2008-09-25 01:15 UTC #4 lore raw

On (24/09/08 21:00), Christoph Hellwig didst pronounce:

Quoted 18 lines · Mel Gorman

Quoted 12 lines
On (08/09/08 19:46), Christoph Hellwig didst pronounce:
Quoted 4 lines
Hugetlbfs currently doesn't set a llseek method for regular files, which means it will fall back to default_llseek. This means no one can seek beyond 2 Gigabytes.
I took another look at this as it was pointed out to me by apw that this might be a SEEK_CUR vs SEEK_SET thing and also whether lseek() was the key. To use lseek though, the large file defines had to be used or it failed whether your patch was applied or not. The error as you'd expect is lseek() complaining that the type was too small.
Quoted 4 lines
At the face of it, the patch seems sensible but it works whether it is set or not so clearly I'm still missing something. The second test I tried is below. In the unlikely event it makes a difference, I was testing on qemu for i386.

Quoted 6 lines

Sorry, my original description was complete bullsh*t ;-) The problem is the inverse of what I wrote. With default_llseek you can seek everywhere even if that's outside of the fs limit. This should give you quite interesting results if you seek outside of what we can represent page->index on 32bit platforms.

Ahh right. To be honest, I can't even tell what the effect is for sure. In ordinary circumstances, I expect it either wraps or zeros are returned when real data should be there.

ftruncate can create a file larger than 4GB of course. However, as hugetlbfs doesn't support write and mmap64 does not map beyond the 4GB boundary, I couldn't create a proper test file. It means I can't verify if the zeros returned after seek above the 4GB boundary are garbage zeros or real zeros (seek above 4GB is allowed with or without the patch, is that expected?). The patch may fix a problem in theory but I can't prove it.

With your filesystem hat on, I'm happy to accept this is a problem in theory and should be applied in case someone adds write() support in the future and gets an unexpected kick in the pants due to lseek(). I'll ack a patch with a fixed-up description and will run the libhugetlbfs regression tests with the patch appliued just to make sure there are no side-effects.