This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1185

cdatapack: do not keep fd open
ClosedPublic

Authored by quark on Oct 18 2017, 7:19 PM.

Download Raw Diff

Details

Reviewers

singhsrb

Group Reviewers

Restricted Project

Commits

rFBHGX6a9de6c27e3a: cdatapack: do not keep fd open

Summary

Once mmap succeeded, it's no longer necessary to keep the underlying fd
open. Therefore just close them as an attempt to reduce file descriptors
opened by the process.

Thanks for spectral from Google for pointing out that fd could be closed.

Test Plan

make clean local
rt test-cstore.t test-*pack*.t

Diff Detail

Repository

rFBHGX Facebook Mercurial Extensions

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Oct 18 2017, 7:19 PM

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 18 2017, 7:19 PM

If the FD are closed, does the mmap still work if the file is deleted? Does that count against the open file limit?

quark edited the summary of this revision. (Show Details)Oct 18 2017, 7:22 PM

In D1185#19960, @durham wrote:

If the FD are closed, does the mmap still work if the file is deleted?

Yes. I think the only problematic case is truncating the file, which also breaks the old code.

Does that count against the open file limit?

I think so. Let me do an experiment.

I tried the following and it works without hitting fd limit.

#include <string>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

using namespace std;

char *dommap(const char *path) {
  int fd = open(path, O_RDONLY);
  char *p = (char *)mmap(NULL, 4096, PROT_READ, MAP_SHARED, fd, 0);
  close(fd);
  return p;
}

int main(int argc, char const *argv[]) {
  for (int i = 0; i < 40960; ++i) {
    printf("%d\n", i);
    auto dest = string("/tmp/true-") + to_string(i);
    system((string("cp /bin/true ") + dest).c_str());
    char *p = dommap(dest.c_str());
    if (strncmp(p + 1, "ELF", 3) != 0) {
      printf("p is not 'ELF' !\n");
      abort();
    }
  }
  return 0;
}

Cool!

This revision is now accepted and ready to land.Oct 18 2017, 7:50 PM

Closed by commit rFBHGX6a9de6c27e3a: cdatapack: do not keep fd open (authored by quark). · Explain WhyOct 18 2017, 7:54 PM

This revision was automatically updated to reflect the committed changes.

quark mentioned this in D1442: test-remotefilelog-datapack: use more packs for cdatapack code path.Nov 16 2017, 7:27 PM

quark mentioned this in rFBHGX27f1a89cd4b5: test-remotefilelog-datapack: use more packs for cdatapack code path.Nov 20 2017, 9:54 AM

Revision Contents
Changeset List

			Path	Packages
M			cdatapack/cdatapack.h (2 lines)
M			cdatapack/cdatapack.c (36 lines)

Diff 3022

cdatapack/cdatapack.h

	DATAPACK_HANDLE_MMAP_ERROR,			DATAPACK_HANDLE_MMAP_ERROR,
	DATAPACK_HANDLE_CORRUPT,			DATAPACK_HANDLE_CORRUPT,
	DATAPACK_HANDLE_VERSION_MISMATCH,			DATAPACK_HANDLE_VERSION_MISMATCH,
	} datapack_handle_status_t;			} datapack_handle_status_t;

	typedef struct _datapack_handle_t {			typedef struct _datapack_handle_t {
	datapack_handle_status_t status;			datapack_handle_status_t status;

	int indexfd;
	int datafd;
	void* index_mmap;			void* index_mmap;
	void* data_mmap;			void* data_mmap;
	off_t index_file_sz;			off_t index_file_sz;
	off_t data_file_sz;			off_t data_file_sz;

	bool large_fanout;			bool large_fanout;

	uint8_t version;			uint8_t version;

cdatapack/cdatapack.c


	// nope, no good.			// nope, no good.
	return false;			return false;
	}			}

	datapack_handle_t *open_datapack(			datapack_handle_t *open_datapack(
	char *indexfp, size_t indexfp_sz,			char *indexfp, size_t indexfp_sz,
	char *datafp, size_t datafp_sz) {			char *datafp, size_t datafp_sz) {
				int indexfd = -1;
				int datafd = -1;
	datapack_handle_t *handle = NULL;			datapack_handle_t *handle = NULL;
	char *buffer = NULL;			char *buffer = NULL;

	handle = malloc(sizeof(datapack_handle_t));			handle = malloc(sizeof(datapack_handle_t));
	if (handle == NULL) {			if (handle == NULL) {
	return NULL;			return NULL;
	}			}

	// can't just use memset because MAP_FAILED is the error result code, not			// can't just use memset because MAP_FAILED is the error result code, not
	// NULL.			// NULL.
	memset(handle, 0, sizeof(datapack_handle_t));			memset(handle, 0, sizeof(datapack_handle_t));
	handle->data_mmap = MAP_FAILED;			handle->data_mmap = MAP_FAILED;
	handle->index_mmap = MAP_FAILED;			handle->index_mmap = MAP_FAILED;

	buffer = malloc(1 + (indexfp_sz > datafp_sz ? indexfp_sz : datafp_sz));			buffer = malloc(1 + (indexfp_sz > datafp_sz ? indexfp_sz : datafp_sz));
	if (buffer == NULL) {			if (buffer == NULL) {
	handle->status = DATAPACK_HANDLE_OOM;			handle->status = DATAPACK_HANDLE_OOM;
	goto error_cleanup;			goto error_cleanup;
	}			}

	memcpy(buffer, indexfp, indexfp_sz);			memcpy(buffer, indexfp, indexfp_sz);
	buffer[indexfp_sz] = '\0';			buffer[indexfp_sz] = '\0';
	handle->indexfd = open(buffer, O_RDONLY);			indexfd = open(buffer, O_RDONLY);
	if (handle->indexfd < 0) {			if (indexfd < 0) {
	handle->status = DATAPACK_HANDLE_IO_ERROR;			handle->status = DATAPACK_HANDLE_IO_ERROR;
	goto error_cleanup;			goto error_cleanup;
	}			}

	handle->index_file_sz = lseek(handle->indexfd, 0, SEEK_END);			handle->index_file_sz = lseek(indexfd, 0, SEEK_END);
	lseek(handle->indexfd, 0, SEEK_SET);			lseek(indexfd, 0, SEEK_SET);

	memcpy(buffer, datafp, datafp_sz);			memcpy(buffer, datafp, datafp_sz);
	buffer[datafp_sz] = '\0';			buffer[datafp_sz] = '\0';
	handle->datafd = open(buffer, O_RDONLY);			datafd = open(buffer, O_RDONLY);
	if (handle->datafd < 0) {			if (datafd < 0) {
	handle->status = DATAPACK_HANDLE_IO_ERROR;			handle->status = DATAPACK_HANDLE_IO_ERROR;
	goto error_cleanup;			goto error_cleanup;
	}			}

	handle->data_file_sz = lseek(handle->datafd, 0, SEEK_END);			handle->data_file_sz = lseek(datafd, 0, SEEK_END);
	lseek(handle->datafd, 0, SEEK_SET);			lseek(datafd, 0, SEEK_SET);

	handle->index_mmap = mmap(NULL, (size_t) handle->index_file_sz, PROT_READ,			handle->index_mmap = mmap(NULL, (size_t) handle->index_file_sz, PROT_READ,
	MAP_PRIVATE, handle->indexfd, (off_t) 0);			MAP_PRIVATE, indexfd, (off_t) 0);
	if (handle->index_mmap == MAP_FAILED) {			if (handle->index_mmap == MAP_FAILED) {
	handle->status = DATAPACK_HANDLE_MMAP_ERROR;			handle->status = DATAPACK_HANDLE_MMAP_ERROR;
	goto error_cleanup;			goto error_cleanup;
	}			}
				close(indexfd);
				indexfd = -1;

	handle->data_mmap = mmap(NULL, (size_t) handle->data_file_sz, PROT_READ,			handle->data_mmap = mmap(NULL, (size_t) handle->data_file_sz, PROT_READ,
	MAP_PRIVATE, handle->datafd, (off_t) 0);			MAP_PRIVATE, datafd, (off_t) 0);
	if (handle->data_mmap == MAP_FAILED) {			if (handle->data_mmap == MAP_FAILED) {
	handle->status = DATAPACK_HANDLE_MMAP_ERROR;			handle->status = DATAPACK_HANDLE_MMAP_ERROR;
	goto error_cleanup;			goto error_cleanup;
	}			}
				close(datafd);
				datafd = -1;

	// read the headers and ensure that the file length is at least somewhat			// read the headers and ensure that the file length is at least somewhat
	// sane.			// sane.
	if (handle->index_file_sz < sizeof(disk_index_header_t)) {			if (handle->index_file_sz < sizeof(disk_index_header_t)) {
	handle->status = DATAPACK_HANDLE_CORRUPT;			handle->status = DATAPACK_HANDLE_CORRUPT;
	goto error_cleanup;			goto error_cleanup;
	}			}
	const disk_index_header_t header = (const disk_index_header_t )			const disk_index_header_t header = (const disk_index_header_t )
	if (handle->index_mmap != MAP_FAILED) {			if (handle->index_mmap != MAP_FAILED) {
	munmap(handle->index_mmap, (size_t) handle->index_file_sz);			munmap(handle->index_mmap, (size_t) handle->index_file_sz);
	}			}

	if (handle->data_mmap != MAP_FAILED) {			if (handle->data_mmap != MAP_FAILED) {
	munmap(handle->data_mmap, (size_t) handle->data_file_sz);			munmap(handle->data_mmap, (size_t) handle->data_file_sz);
	}			}

	if (handle && handle->indexfd != 0) {			if (indexfd != -1) {
	close(handle->indexfd);			close(indexfd);
	}			}

	if (handle && handle->datafd != 0) {			if (datafd != -1) {
	close(handle->datafd);			close(datafd);
	}			}

	if (handle != NULL) {			if (handle != NULL) {
	free(handle->fanout_table);			free(handle->fanout_table);
	}			}

	success_cleanup:			success_cleanup:

	free(buffer);			free(buffer);

	return handle;			return handle;
	}			}

	void close_datapack(datapack_handle_t *handle) {			void close_datapack(datapack_handle_t *handle) {
	munmap(handle->index_mmap, (size_t) handle->index_file_sz);			munmap(handle->index_mmap, (size_t) handle->index_file_sz);
	munmap(handle->data_mmap, (size_t) handle->data_file_sz);			munmap(handle->data_mmap, (size_t) handle->data_file_sz);
	close(handle->indexfd);
	close(handle->datafd);
	free(handle->fanout_table);			free(handle->fanout_table);
	free(handle);			free(handle);
	}			}

	#define DEFAULT_PACK_CHAIN_CAPACITY 64			#define DEFAULT_PACK_CHAIN_CAPACITY 64
	#define PACK_CHAIN_GROWTH_FACTOR 2.0			#define PACK_CHAIN_GROWTH_FACTOR 2.0
	#define PACK_CHAIN_MINIMUM_GROWTH 1024			#define PACK_CHAIN_MINIMUM_GROWTH 1024
	#define PACK_CHAIN_MAXIMUM_GROWTH 65536			#define PACK_CHAIN_MAXIMUM_GROWTH 65536

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3015		Oct 18 2017, 7:18 PM	★	★
Diff 2	3022	rFBHGX6a9de6c27e3aff105e6b9e693cdb2ec402bb86ef	Oct 18 2017, 7:52 PM	★	★