Skip to content

Commit

Permalink
Fix a bug in Subfiling VFD vector I/O setup
Browse files Browse the repository at this point in the history 
Fixes a bug where the vector I/O sizes weren't being extended when
one of the entries in the array is 0. This caused an overread of
the I/O sizes buffer and on some machines would cause a memory
allocation failure due to the calculated I/O vector size being too
large.
  • Loading branch information
@jhendersonHDF
jhendersonHDF committed Sep 10, 2024
1 parent b757ea9 commit 3ca48de
Show file tree
Hide file tree
Showing 2 changed files with 184 additions and 45 deletions.
113 changes: 68 additions & 45 deletions src/H5FDsubfiling/H5FDsubfiling.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,13 @@ static herr_t H5FD__subfiling_mirror_writes_to_stub(H5FD_subfiling_t *file, uint
const void *bufs[]);
static herr_t H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_count,
H5FD_mem_t types[], haddr_t file_offsets[],
size_t nelemts[], H5_flexible_const_ptr_t bufs[],
H5FD_subfiling_io_type_t io_type, size_t *ioreq_count,
uint32_t *iovec_len, H5FD_mem_t **io_types,
haddr_t **io_addrs, size_t **io_sizes,
H5_flexible_const_ptr_t **io_bufs);
static void H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count,
haddr_t file_offsets[], size_t nelemts[], size_t *max_iovec_depth,
size_t io_sizes[], H5_flexible_const_ptr_t bufs[],
H5FD_subfiling_io_type_t io_type, size_t *ioreq_count_out,
uint32_t *iovec_len_out, H5FD_mem_t **io_types_out,
haddr_t **io_addrs_out, size_t **io_sizes_out,
H5_flexible_const_ptr_t **io_bufs_out);
static herr_t H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count,
haddr_t file_offsets[], size_t io_sizes[], size_t *max_iovec_depth,
size_t *max_num_subfiles);
static herr_t H5FD__subfiling_translate_io_req_to_iovec(
subfiling_context_t *sf_context, size_t iovec_idx, size_t iovec_len, size_t iovec_count, H5FD_mem_t type,
Expand Down Expand Up @@ -2023,7 +2023,7 @@ H5FD__subfiling_io_helper(H5FD_subfiling_t *file, size_t io_count, H5FD_mem_t ty
io_count, /* IN: Number of entries in `types`, `addrs`, `sizes` and `bufs` */
types, /* IN: Array of memory types */
addrs, /* IN: Array of starting file offsets */
sizes, /* IN: Array of I/O sizes (in terms of elements) */
sizes, /* IN: Array of I/O sizes */
bufs, /* IN: Array of I/O buffers */
io_type, /* IN: Type of I/O being performed (IO_TYPE_WRITE or IO_TYPE_READ) */
&ioreq_count, /* OUT: Number of I/O requests to be made */
Expand Down Expand Up @@ -2335,30 +2335,30 @@ H5FD__subfiling_mirror_writes_to_stub(H5FD_subfiling_t *file, uint32_t count, H5
* - the type of I/O being performed (IO_TYPE_WRITE or
* IO_TYPE_READ)
*
* ioreq_count (OUT)
* ioreq_count_out (OUT)
* - the number of I/O requests needed to fully satisfy the
* I/O operation
*
* iovec_len (OUT)
* iovec_len_out (OUT)
* - the size of each I/O vector (in terms of array elements)
* for each I/O request to be made
*
* io_types (OUT)
* io_types_out (OUT)
* - I/O vector of memory types for the I/O operation.
* Allocated by this function and must be freed by the
* caller.
*
* io_addrs (OUT)
* io_addrs_out (OUT)
* - I/O vector of file addresses for the I/O operation.
* Allocated by this function and must be freed by the
* caller.
*
* io_sizes (OUT)
* io_sizes_out (OUT)
* - I/O vector of the I/O sizes for the I/O operation.
* Allocated by this function and must be freed by the
* caller.
*
* io_bufs (OUT)
* io_bufs_out (OUT)
* - I/O vector of the I/O buffers for the I/O operation.
* Allocated by this function and must be freed by the
* caller.
Expand All @@ -2368,10 +2368,10 @@ H5FD__subfiling_mirror_writes_to_stub(H5FD_subfiling_t *file, uint32_t count, H5
*/
static herr_t
H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_count, H5FD_mem_t types[],
haddr_t file_offsets[], size_t nelemts[], H5_flexible_const_ptr_t bufs[],
H5FD_subfiling_io_type_t io_type, size_t *ioreq_count,
uint32_t *iovec_len, H5FD_mem_t **io_types, haddr_t **io_addrs,
size_t **io_sizes, H5_flexible_const_ptr_t **io_bufs)
haddr_t file_offsets[], size_t io_sizes[], H5_flexible_const_ptr_t bufs[],
H5FD_subfiling_io_type_t io_type, size_t *ioreq_count_out,
uint32_t *iovec_len_out, H5FD_mem_t **io_types_out, haddr_t **io_addrs_out,
size_t **io_sizes_out, H5_flexible_const_ptr_t **io_bufs_out)
{
H5_flexible_const_ptr_t *loc_io_bufs = NULL;
H5FD_mem_t *loc_io_types = NULL;
Expand All @@ -2395,18 +2395,18 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c
assert(sf_context->topology);
assert(types || in_count == 0);
assert(file_offsets || in_count == 0);
assert(nelemts || in_count == 0);
assert(io_sizes || in_count == 0);
assert(bufs || in_count == 0);
assert(ioreq_count);
assert(iovec_len);
assert(io_types);
assert(io_addrs);
assert(io_sizes);
assert(io_bufs);
assert(ioreq_count_out);
assert(iovec_len_out);
assert(io_types_out);
assert(io_addrs_out);
assert(io_sizes_out);
assert(io_bufs_out);

/* Set some returned values early */
*ioreq_count = 0;
*iovec_len = 0;
*ioreq_count_out = 0;
*iovec_len_out = 0;

/* Nothing to do */
if (in_count == 0)
Expand All @@ -2416,11 +2416,16 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c
* Do some initial pre-processing to determine how large of I/O vectors we
* will need to allocate to satisfy the entire I/O request
*/
H5FD__subfiling_get_iovec_sizes(sf_context, in_count, file_offsets, nelemts, &max_iovec_depth,
&max_num_subfiles_touched);
if (H5FD__subfiling_get_iovec_sizes(sf_context, in_count, file_offsets, io_sizes,
&max_iovec_depth, &max_num_subfiles_touched) < 0)
HGOTO_ERROR(H5E_VFL, H5E_CANTGET, FAIL, "can't determine maximum I/O request size");

tot_iovec_len = in_count * max_iovec_depth * max_num_subfiles_touched;

/* Nothing to do */
if (tot_iovec_len == 0)
HGOTO_DONE(SUCCEED);

#ifdef H5_SUBFILING_DEBUG
H5FD__subfiling_log(
sf_context->sf_context_id,
Expand Down Expand Up @@ -2456,10 +2461,10 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c
}

if (!extend_sizes) {
if (io_idx > 0 && nelemts[io_idx] == 0)
if (io_idx > 0 && io_sizes[io_idx] == 0)
extend_sizes = true;
else
io_size = nelemts[io_idx];
io_size = io_sizes[io_idx];
}

if (H5FD__subfiling_translate_io_req_to_iovec(sf_context, iovec_idx, max_num_subfiles_touched,
Expand All @@ -2469,13 +2474,13 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c
HGOTO_ERROR(H5E_VFL, H5E_CANTINIT, FAIL, "can't translate I/O request to I/O vectors");
}

*ioreq_count = in_count * max_iovec_depth;
*ioreq_count_out = in_count * max_iovec_depth;
H5_CHECK_OVERFLOW(max_num_subfiles_touched, size_t, uint32_t);
*iovec_len = (uint32_t)max_num_subfiles_touched;
*io_types = loc_io_types;
*io_addrs = loc_io_addrs;
*io_sizes = loc_io_sizes;
*io_bufs = loc_io_bufs;
*iovec_len_out = (uint32_t)max_num_subfiles_touched;
*io_types_out = loc_io_types;
*io_addrs_out = loc_io_addrs;
*io_sizes_out = loc_io_sizes;
*io_bufs_out = loc_io_bufs;

done:
if (ret_value < 0) {
Expand All @@ -2497,26 +2502,28 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c
* info is used to calculate the total size of I/O vectors we
* need to allocate to satisfy an entire I/O request.
*
* Return: Maximum I/O vector depth and maximum number of subfiles
* touched (can't fail)
* Return: Non-negative on success/negative on failure
*
*-------------------------------------------------------------------------
*/
static void
static herr_t
H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count, haddr_t file_offsets[],
size_t nelemts[], size_t *max_iovec_depth, size_t *max_num_subfiles)
size_t io_sizes[], size_t *max_iovec_depth, size_t *max_num_subfiles)
{
int64_t stripe_size = 0;
int64_t block_size = 0;
size_t loc_max_iovec_depth = 0;
size_t loc_max_num_subfiles = 0;
size_t io_size = 0;
bool extend_sizes = false;
int num_subfiles = 0;
herr_t ret_value = SUCCEED;

FUNC_ENTER_PACKAGE_NOERR
FUNC_ENTER_PACKAGE

assert(sf_context);
assert(file_offsets);
assert(nelemts);
assert(io_sizes);
assert(max_iovec_depth);
assert(max_num_subfiles);

Expand All @@ -2538,7 +2545,22 @@ H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count
size_t cur_iovec_depth;

H5_CHECKED_ASSIGN(cur_file_offset, int64_t, file_offsets[io_idx], haddr_t);
H5_CHECKED_ASSIGN(data_size, int64_t, nelemts[io_idx], size_t);

if (!extend_sizes) {
if (io_idx > 0 && io_sizes[io_idx] == 0)
extend_sizes = true;
else
io_size = io_sizes[io_idx];
}

H5_CHECKED_ASSIGN(data_size, int64_t, io_size, size_t);

if (cur_file_offset < 0)
HGOTO_ERROR(H5E_VFL, H5E_BADVALUE, FAIL, "file offset of %" PRIuHADDR " at index %zu too large; wrapped around",
file_offsets[io_idx], io_idx);
if (data_size < 0)
HGOTO_ERROR(H5E_VFL, H5E_BADVALUE, FAIL, "I/O size of %zu at index %zu too large; wrapped around",
io_size, io_idx);

/*
* Calculate the following from the starting file offset:
Expand Down Expand Up @@ -2645,7 +2667,8 @@ H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count
*max_iovec_depth = loc_max_iovec_depth;
*max_num_subfiles = loc_max_num_subfiles;

FUNC_LEAVE_NOAPI_VOID
done:
FUNC_LEAVE_NOAPI(ret_value)
}

/*-------------------------------------------------------------------------
Expand Down
116 changes: 116 additions & 0 deletions testpar/t_subfiling_vfd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ static void test_read_different_stripe_size(void);
static void test_subfiling_precreate_rank_0(void);
static void test_subfiling_write_many_read_one(void);
static void test_subfiling_write_many_read_few(void);
static void test_subfiling_vector_io_extension(void);
static void test_subfiling_h5fuse(void);

static test_func tests[] = {
Expand All @@ -118,6 +119,7 @@ static test_func tests[] = {
test_subfiling_precreate_rank_0,
test_subfiling_write_many_read_one,
test_subfiling_write_many_read_few,
test_subfiling_vector_io_extension,
test_subfiling_h5fuse,
};

Expand Down Expand Up @@ -2568,6 +2570,120 @@ test_subfiling_write_many_read_few(void)
#undef SUBF_HDF5_TYPE
#undef SUBF_C_TYPE

/*
* Test to check for a bug where the vector I/O sizes
* array wasn't being extended when an entry in the
* array was 0.
*/
#define SUBF_FILENAME "test_subfiling_vector_io_extension.h5"
#define SUBF_C_TYPE int
static void
test_subfiling_vector_io_extension(void)
{
H5FD_subfiling_params_t cfg;
h5_stat_size_t file_size;
SUBF_C_TYPE *read_buf = NULL;
H5FD_mem_t *types = NULL;
h5_stat_t file_info;
uint32_t count = 64;
haddr_t *addrs = NULL;
haddr_t file_end_addr;
herr_t read_status;
size_t *sizes = NULL;
H5FD_t *file_ptr = NULL;
hid_t file_id = H5I_INVALID_HID;
hid_t fapl_id = H5I_INVALID_HID;
hid_t dxpl_id = H5I_INVALID_HID;
void **bufs = NULL;

curr_nerrors = nerrors;

if (MAINPROCESS)
TESTING_2("I/O vector size extension functionality");

/* Must use at least 2 subfiles to cause generation of
* I/O vectors within the VFD.
*/
cfg.ioc_selection = SELECT_IOC_ONE_PER_NODE;
cfg.stripe_size = (stripe_size_g > 0) ? stripe_size_g : 1048576;
cfg.stripe_count = num_iocs_g > 1 ? num_iocs_g : 2;

fapl_id = create_subfiling_ioc_fapl(comm_g, info_g, true, &cfg, H5FD_IOC_DEFAULT_THREAD_POOL_SIZE);
VRFY((fapl_id >= 0), "FAPL creation succeeded");

file_id = H5Fcreate(SUBF_FILENAME, H5F_ACC_TRUNC, H5P_DEFAULT, fapl_id);
VRFY((file_id >= 0), "H5Fcreate succeeded");

VRFY((H5Fclose(file_id) >= 0), "File close succeeded");

/* Re-open file through H5FDopen for direct reads */
file_ptr = H5FDopen(SUBF_FILENAME, H5F_ACC_RDWR, fapl_id, HADDR_UNDEF);
VRFY(file_ptr, "H5FDopen succeeded");

/*
* Get the current file size to see where we can safely
* write to in the file without overwriting the superblock
*/
memset(&file_info, 0, sizeof(h5_stat_t));
VRFY((HDstat(SUBF_FILENAME, &file_info) >= 0), "HDstat succeeded");
file_size = (h5_stat_size_t)file_info.st_size;

H5_CHECK_OVERFLOW(file_size, h5_stat_size_t, haddr_t);
file_end_addr = (haddr_t)file_size;

dxpl_id = H5Pcreate(H5P_DATASET_XFER);
VRFY((dxpl_id >= 0), "DXPL creation succeeded");

/* Set independent I/O on DXPL */
VRFY((H5Pset_dxpl_mpio(dxpl_id, H5FD_MPIO_INDEPENDENT) >= 0), "H5Pset_dxpl_mpio succeeded");

/* Set EOA for following read call */
VRFY((H5FDset_eoa(file_ptr, H5FD_MEM_DEFAULT, file_end_addr + (count * sizeof(int))) >= 0),
"H5FDset_eoa succeeded");

read_buf = malloc(count * sizeof(*read_buf));
types = malloc(count * sizeof(*types));
addrs = malloc(count * sizeof(*addrs));
sizes = malloc(2 * sizeof(size_t));
bufs = malloc(count * sizeof(*bufs));

sizes[0] = sizeof(SUBF_C_TYPE);
sizes[1] = 0;

for (size_t i = 0; i < count; i++) {
types[i] = H5FD_MEM_DRAW;
addrs[i] = file_end_addr + (i * sizeof(SUBF_C_TYPE));
bufs[i] = (void *)&(read_buf[i]);
}

read_status = H5FDread_vector(file_ptr, dxpl_id, count, types, addrs, sizes, bufs);
VRFY((read_status >= 0), "H5FDread_vector succeeded");

VRFY((H5FDclose(file_ptr) >= 0), "H5FDclose succeeded");

mpi_code_g = MPI_Barrier(comm_g);
VRFY((mpi_code_g == MPI_SUCCESS), "MPI_Barrier succeeded");

H5E_BEGIN_TRY
{
H5Fdelete(SUBF_FILENAME, fapl_id);
}
H5E_END_TRY

VRFY((H5Pclose(fapl_id) >= 0), "FAPL close succeeded");
VRFY((H5Pclose(dxpl_id) >= 0), "DXPL close succeeded");

free(bufs);
free(sizes);
free(addrs);
free(types);
free(read_buf);

CHECK_PASSED();
}
#undef SUBF_FILENAME
#undef SUBF_C_TYPE

/*
* Test that the subfiling file can be read with the
* sec2 driver after being fused back together with
Expand Down

0 comments on commit 3ca48de

Please sign in to comment.