From a8443acaa76b79ebe74624af363008f0163a44f8 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Wed, 6 Mar 2019 16:57:42 -0600 Subject: Fix issue with direct chunk write not updating the "last chunk" index cache. Fix issues involving datasets being "no allocated" when they contain cached raw data. --- release_docs/RELEASE.txt | 12 +++++ src/H5Dchunk.c | 40 ++++++++++++++-- src/H5Dcompact.c | 1 + src/H5Dcontig.c | 25 ++++++++++ src/H5Defl.c | 1 + src/H5Dint.c | 3 +- src/H5Dio.c | 4 +- src/H5Dpkg.h | 9 +++- src/H5Dvirtual.c | 50 ++++++++++++++++++++ src/H5Olayout.c | 6 ++- test/direct_chunk.c | 121 +++++++++++++++++++++++++++++++++++++++-------- 11 files changed, 243 insertions(+), 29 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 3b68ba0..77f7f4e 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -327,6 +327,18 @@ Bug Fixes since HDF5-1.10.4 release Library ------- + - Fixed a bug that would cause an error or cause fill values to be + incorrectly read from a chunked dataset using the "single chunk" index if + the data was held in cache and there was no data on disk. + + (NAF - 2019/03/06) + + - Fixed a bug that could cause an error or cause fill values to be + incorrectly read from a dataset that was written to using H5Dwrite_chunk + if the dataset was not closed after writing. + + (NAF - 2019/03/06, HDFFV-10716) + - Fixed memory leak in scale offset filter In a special case where the MinBits is the same as the number of bits in diff --git a/src/H5Dchunk.c b/src/H5Dchunk.c index 553d75c..89c182d 100644 --- a/src/H5Dchunk.c +++ b/src/H5Dchunk.c @@ -331,6 +331,7 @@ const H5D_layout_ops_t H5D_LOPS_CHUNK[1] = {{ H5D__chunk_construct, H5D__chunk_init, H5D__chunk_is_space_alloc, + H5D__chunk_is_data_cached, H5D__chunk_io_init, H5D__chunk_read, H5D__chunk_write, @@ -358,6 +359,7 @@ const H5D_layout_ops_t H5D_LOPS_NONEXISTENT[1] = {{ NULL, NULL, NULL, + NULL, #ifdef H5_HAVE_PARALLEL NULL, NULL, @@ -412,10 +414,13 @@ H5D__chunk_direct_write(const H5D_t *dset, uint32_t filters, hsize_t *offset, FUNC_ENTER_PACKAGE_TAG(dset->oloc.addr) + /* Sanity checks */ + HDassert(layout->type == H5D_CHUNKED); + io_info.dset = dset; /* Allocate dataspace and initialize it if it hasn't been. */ - if(!(*layout->ops->is_space_alloc)(&layout->storage)) + if(!H5D__chunk_is_space_alloc(&layout->storage)) /* Allocate storage */ if(H5D__alloc_storage(&io_info, H5D_ALLOC_WRITE, FALSE, NULL) < 0) HGOTO_ERROR(H5E_DATASET, H5E_CANTINIT, FAIL, "unable to initialize storage") @@ -453,13 +458,17 @@ H5D__chunk_direct_write(const H5D_t *dset, uint32_t filters, hsize_t *offset, if(0 == idx_info.pline->nused && H5F_addr_defined(old_chunk.offset)) /* If there are no filters and we are overwriting the chunk we can just set values */ need_insert = FALSE; - else + else { /* Otherwise, create the chunk it if it doesn't exist, or reallocate the chunk * if its size has changed. */ if(H5D__chunk_file_alloc(&idx_info, &old_chunk, &udata.chunk_block, &need_insert, scaled) < 0) HGOTO_ERROR(H5E_DATASET, H5E_CANTALLOC, FAIL, "unable to allocate chunk") + /* Cache the new chunk information */ + H5D__chunk_cinfo_cache_update(&dset->shared->cache.chunk.last, &udata); + } /* end else */ + /* Make sure the address of the chunk is returned. */ if(!H5F_addr_defined(udata.chunk_block.offset)) HGOTO_ERROR(H5E_DATASET, H5E_BADVALUE, FAIL, "chunk address isn't defined") @@ -524,7 +533,8 @@ H5D__chunk_direct_read(const H5D_t *dset, hsize_t *offset, uint32_t* filters, *filters = 0; /* Allocate dataspace and initialize it if it hasn't been. */ - if(!(*layout->ops->is_space_alloc)(&layout->storage)) + if(!H5D__chunk_is_space_alloc(&layout->storage) + && !H5D__chunk_is_data_cached(dset->shared)) HGOTO_ERROR(H5E_DATASET, H5E_CANTINIT, FAIL, "storage is not initialized") /* Calculate the index of this chunk */ @@ -1037,6 +1047,30 @@ H5D__chunk_is_space_alloc(const H5O_storage_t *storage) /*------------------------------------------------------------------------- + * Function: H5D__chunk_is_data_cached + * + * Purpose: Query if raw data is cached for dataset + * + * Return: Non-negative on success/Negative on failure + * + * Programmer: Neil Fortner + * Wednessday, March 6, 2016 + * + *------------------------------------------------------------------------- + */ +hbool_t +H5D__chunk_is_data_cached(const H5D_shared_t *shared_dset) +{ + FUNC_ENTER_PACKAGE_NOERR + + /* Sanity checks */ + HDassert(shared_dset); + + FUNC_LEAVE_NOAPI(shared_dset->cache.chunk.nused > 0) +} /* end H5D__chunk_is_data_cached() */ + + +/*------------------------------------------------------------------------- * Function: H5D__chunk_io_init * * Purpose: Performs initialization before any sort of I/O on the raw data diff --git a/src/H5Dcompact.c b/src/H5Dcompact.c index c0c2a80..651aaf4 100644 --- a/src/H5Dcompact.c +++ b/src/H5Dcompact.c @@ -80,6 +80,7 @@ const H5D_layout_ops_t H5D_LOPS_COMPACT[1] = {{ H5D__compact_construct, NULL, H5D__compact_is_space_alloc, + NULL, H5D__compact_io_init, H5D__contig_read, H5D__contig_write, diff --git a/src/H5Dcontig.c b/src/H5Dcontig.c index ad12ba0..c2e9bfc 100644 --- a/src/H5Dcontig.c +++ b/src/H5Dcontig.c @@ -118,6 +118,7 @@ const H5D_layout_ops_t H5D_LOPS_CONTIG[1] = {{ H5D__contig_construct, H5D__contig_init, H5D__contig_is_space_alloc, + H5D__contig_is_data_cached, H5D__contig_io_init, H5D__contig_read, H5D__contig_write, @@ -538,6 +539,30 @@ H5D__contig_is_space_alloc(const H5O_storage_t *storage) /*------------------------------------------------------------------------- + * Function: H5D__contig_is_data_cached + * + * Purpose: Query if raw data is cached for dataset + * + * Return: Non-negative on success/Negative on failure + * + * Programmer: Neil Fortner + * Wednessday, March 6, 2016 + * + *------------------------------------------------------------------------- + */ +hbool_t +H5D__contig_is_data_cached(const H5D_shared_t *shared_dset) +{ + FUNC_ENTER_PACKAGE_NOERR + + /* Sanity checks */ + HDassert(shared_dset); + + FUNC_LEAVE_NOAPI(shared_dset->cache.contig.sieve_size > 0) +} /* end H5D__contig_is_data_cached() */ + + +/*------------------------------------------------------------------------- * Function: H5D__contig_io_init * * Purpose: Performs initialization before any sort of I/O on the raw data diff --git a/src/H5Defl.c b/src/H5Defl.c index b2f9b29..42b3947 100644 --- a/src/H5Defl.c +++ b/src/H5Defl.c @@ -91,6 +91,7 @@ const H5D_layout_ops_t H5D_LOPS_EFL[1] = {{ H5D__efl_construct, NULL, H5D__efl_is_space_alloc, + NULL, H5D__efl_io_init, H5D__contig_read, H5D__contig_write, diff --git a/src/H5Dint.c b/src/H5Dint.c index ea4245b..d3cad8f 100644 --- a/src/H5Dint.c +++ b/src/H5Dint.c @@ -2878,7 +2878,8 @@ H5D__set_extent(H5D_t *dset, const hsize_t *size) *------------------------------------------------------------------------- */ if(H5D_CHUNKED == dset->shared->layout.type) { - if(shrink && (*dset->shared->layout.ops->is_space_alloc)(&dset->shared->layout.storage)) + if(shrink && ((*dset->shared->layout.ops->is_space_alloc)(&dset->shared->layout.storage) + || (dset->shared->layout.ops->is_data_cached && (*dset->shared->layout.ops->is_data_cached)(dset->shared)))) /* Remove excess chunks */ if(H5D__chunk_prune_by_extent(dset, curr_dims) < 0) HGOTO_ERROR(H5E_DATASET, H5E_WRITEERROR, FAIL, "unable to remove chunks") diff --git a/src/H5Dio.c b/src/H5Dio.c index 8abcd81..a03f780 100644 --- a/src/H5Dio.c +++ b/src/H5Dio.c @@ -546,7 +546,8 @@ H5D__read(H5D_t *dataset, hid_t mem_type_id, const H5S_t *mem_space, * has been overwritten. So just proceed in reading. */ if(nelmts > 0 && dataset->shared->dcpl_cache.efl.nused == 0 && - !(*dataset->shared->layout.ops->is_space_alloc)(&dataset->shared->layout.storage)) { + !(*dataset->shared->layout.ops->is_space_alloc)(&dataset->shared->layout.storage) && + !(dataset->shared->layout.ops->is_data_cached && (*dataset->shared->layout.ops->is_data_cached)(dataset->shared))) { H5D_fill_value_t fill_status; /* Whether/How the fill value is defined */ /* Retrieve dataset's fill-value properties */ @@ -578,6 +579,7 @@ H5D__read(H5D_t *dataset, hid_t mem_type_id, const H5S_t *mem_space, /* Sanity check that space is allocated, if there are elements */ if(nelmts > 0) HDassert((*dataset->shared->layout.ops->is_space_alloc)(&dataset->shared->layout.storage) + || (dataset->shared->layout.ops->is_data_cached && (*dataset->shared->layout.ops->is_data_cached)(dataset->shared)) || dataset->shared->dcpl_cache.efl.nused > 0 || dataset->shared->layout.type == H5D_COMPACT); diff --git a/src/H5Dpkg.h b/src/H5Dpkg.h index da74d11..d4fd34d 100644 --- a/src/H5Dpkg.h +++ b/src/H5Dpkg.h @@ -113,12 +113,14 @@ typedef struct H5D_type_info_t { /* Forward declaration of structs used below */ struct H5D_io_info_t; struct H5D_chunk_map_t; +typedef struct H5D_shared_t H5D_shared_t; /* Function pointers for I/O on particular types of dataset layouts */ typedef herr_t (*H5D_layout_construct_func_t)(H5F_t *f, H5D_t *dset); typedef herr_t (*H5D_layout_init_func_t)(H5F_t *f, const H5D_t *dset, hid_t dapl_id); typedef hbool_t (*H5D_layout_is_space_alloc_func_t)(const H5O_storage_t *storage); +typedef hbool_t (*H5D_layout_is_data_cached_func_t)(const H5D_shared_t *shared_dset); typedef herr_t (*H5D_layout_io_init_func_t)(const struct H5D_io_info_t *io_info, const H5D_type_info_t *type_info, hsize_t nelmts, const H5S_t *file_space, const H5S_t *mem_space, @@ -144,6 +146,7 @@ typedef struct H5D_layout_ops_t { H5D_layout_construct_func_t construct; /* Layout constructor for new datasets */ H5D_layout_init_func_t init; /* Layout initializer for dataset */ H5D_layout_is_space_alloc_func_t is_space_alloc; /* Query routine to determine if storage is allocated */ + H5D_layout_is_data_cached_func_t is_data_cached; /* Query routine to determine if any raw data is cached. If routine is not present then the layout type never caches raw data. */ H5D_layout_io_init_func_t io_init; /* I/O initialization routine */ H5D_layout_read_func_t ser_read; /* High-level I/O routine for reading data in serial */ H5D_layout_write_func_t ser_write; /* High-level I/O routine for writing data in serial */ @@ -429,7 +432,7 @@ typedef struct H5D_rdcdc_t { * created once for a given dataset. Thus, if a dataset is opened twice, * there will be two IDs and two H5D_t structs, both sharing one H5D_shared_t. */ -typedef struct H5D_shared_t { +struct H5D_shared_t { size_t fo_count; /* Reference count */ hbool_t closing; /* Flag to indicate dataset is closing */ hid_t type_id; /* ID for dataset's datatype */ @@ -459,7 +462,7 @@ typedef struct H5D_shared_t { H5D_append_flush_t append_flush; /* Append flush property information */ char *extfile_prefix; /* expanded external file prefix */ char *vds_prefix; /* expanded vds prefix */ -} H5D_shared_t; +}; struct H5D_t { H5O_loc_t oloc; /* Object header location */ @@ -624,6 +627,7 @@ H5_DLL herr_t H5D__layout_oh_write(const H5D_t *dataset, H5O_t *oh, unsigned upd /* Functions that operate on contiguous storage */ H5_DLL herr_t H5D__contig_alloc(H5F_t *f, H5O_storage_contig_t *storage); H5_DLL hbool_t H5D__contig_is_space_alloc(const H5O_storage_t *storage); +H5_DLL hbool_t H5D__contig_is_data_cached(const H5D_shared_t *shared_dset); H5_DLL herr_t H5D__contig_fill(const H5D_io_info_t *io_info); H5_DLL herr_t H5D__contig_read(H5D_io_info_t *io_info, const H5D_type_info_t *type_info, hsize_t nelmts, const H5S_t *file_space, const H5S_t *mem_space, @@ -642,6 +646,7 @@ H5_DLL htri_t H5D__chunk_cacheable(const H5D_io_info_t *io_info, haddr_t caddr, H5_DLL herr_t H5D__chunk_create(const H5D_t *dset /*in,out*/); H5_DLL herr_t H5D__chunk_set_info(const H5D_t *dset); H5_DLL hbool_t H5D__chunk_is_space_alloc(const H5O_storage_t *storage); +H5_DLL hbool_t H5D__chunk_is_data_cached(const H5D_shared_t *shared_dset); H5_DLL herr_t H5D__chunk_lookup(const H5D_t *dset, const hsize_t *scaled, H5D_chunk_ud_t *udata); H5_DLL herr_t H5D__chunk_allocated(const H5D_t *dset, hsize_t *nbytes); diff --git a/src/H5Dvirtual.c b/src/H5Dvirtual.c index 1d2b191..2e8ca5d 100644 --- a/src/H5Dvirtual.c +++ b/src/H5Dvirtual.c @@ -78,6 +78,7 @@ /********************/ /* Layout operation callbacks */ +static hbool_t H5D__virtual_is_data_cached(const H5D_shared_t *shared_dset); static herr_t H5D__virtual_read(H5D_io_info_t *io_info, const H5D_type_info_t *type_info, hsize_t nelmts, const H5S_t *file_space, const H5S_t *mem_space, H5D_chunk_map_t *fm); @@ -121,6 +122,7 @@ const H5D_layout_ops_t H5D_LOPS_VIRTUAL[1] = {{ NULL, H5D__virtual_init, H5D__virtual_is_space_alloc, + H5D__virtual_is_data_cached, NULL, H5D__virtual_read, H5D__virtual_write, @@ -2235,6 +2237,54 @@ H5D__virtual_is_space_alloc(const H5O_storage_t H5_ATTR_UNUSED *storage) /*------------------------------------------------------------------------- + * Function: H5D__virtual_is_data_cached + * + * Purpose: Query if raw data is cached for dataset + * + * Return: Non-negative on success/Negative on failure + * + * Programmer: Neil Fortner + * Wednessday, March 6, 2016 + * + *------------------------------------------------------------------------- + */ +static hbool_t +H5D__virtual_is_data_cached(const H5D_shared_t *shared_dset) +{ + const H5O_storage_virtual_t *storage; /* Convenience pointer */ + size_t i, j; /* Local index variables */ + hbool_t ret_value = FALSE; /* Return value */ + + FUNC_ENTER_PACKAGE_NOERR + + /* Sanity checks */ + HDassert(shared_dset); + storage = &shared_dset->layout.storage.u.virt; + + /* Iterate over mappings */ + for(i = 0; i < storage->list_nused; i++) + /* Check for "printf" source dataset resolution */ + if(storage->list[i].psfn_nsubs || storage->list[i].psdn_nsubs) { + /* Iterate over sub-source dsets */ + for(j = storage->list[i].sub_dset_io_start; j < storage->list[i].sub_dset_io_end; j++) + /* Check for cahced data in source dset */ + if(storage->list[i].sub_dset[j].dset + && storage->list[i].sub_dset[j].dset->shared->layout.ops->is_data_cached + && storage->list[i].sub_dset[j].dset->shared->layout.ops->is_data_cached(storage->list[i].sub_dset[j].dset->shared)) + HGOTO_DONE(TRUE); + } /* end if */ + else + if(storage->list[i].source_dset.dset + && storage->list[i].source_dset.dset->shared->layout.ops->is_data_cached + && storage->list[i].source_dset.dset->shared->layout.ops->is_data_cached(storage->list[i].source_dset.dset->shared)) + HGOTO_DONE(TRUE); + +done: + FUNC_LEAVE_NOAPI(ret_value) +} /* end H5D__virtual_is_data_cached() */ + + +/*------------------------------------------------------------------------- * Function: H5D__virtual_pre_io * * Purpose: Project all virtual mappings onto mem_space, with the diff --git a/src/H5Olayout.c b/src/H5Olayout.c index d0c7f3e..ba63a16 100644 --- a/src/H5Olayout.c +++ b/src/H5Olayout.c @@ -1045,7 +1045,8 @@ H5O__layout_copy_file(H5F_t *file_src, void *mesg_src, H5F_t *file_dst, layout_dst->storage.u.contig.size = H5S_extent_nelem(udata->src_space_extent) * H5T_get_size(udata->src_dtype); - if(H5D__contig_is_space_alloc(&layout_src->storage)) { + if(H5D__contig_is_space_alloc(&layout_src->storage) + || (cpy_info->shared_fo && H5D__contig_is_data_cached((const H5D_shared_t *)cpy_info->shared_fo))) { /* copy contiguous raw data */ if(H5D__contig_copy(file_src, &layout_src->storage.u.contig, file_dst, &layout_dst->storage.u.contig, udata->src_dtype, cpy_info) < 0) HGOTO_ERROR(H5E_OHDR, H5E_CANTCOPY, NULL, "unable to copy contiguous storage") @@ -1054,7 +1055,8 @@ H5O__layout_copy_file(H5F_t *file_src, void *mesg_src, H5F_t *file_dst, break; case H5D_CHUNKED: - if(H5D__chunk_is_space_alloc(&layout_src->storage)) { + if(H5D__chunk_is_space_alloc(&layout_src->storage) + || (cpy_info->shared_fo && H5D__chunk_is_data_cached((const H5D_shared_t *)cpy_info->shared_fo))) { /* Create chunked layout */ if(H5D__chunk_copy(file_src, &layout_src->storage.u.chunk, &layout_src->u.chunk, file_dst, &layout_dst->storage.u.chunk, udata->src_space_extent, udata->src_dtype, udata->common.src_pline, cpy_info) < 0) HGOTO_ERROR(H5E_OHDR, H5E_CANTCOPY, NULL, "unable to copy chunked storage") diff --git a/test/direct_chunk.c b/test/direct_chunk.c index 1c5862d..f68fb93 100644 --- a/test/direct_chunk.c +++ b/test/direct_chunk.c @@ -58,6 +58,14 @@ #define OVERWRITE_CHUNK_NY 2 #define OVERWRITE_VALUE 42 +/* Test configurations */ +#define CONFIG_LATEST 0x01 +#define CONFIG_REOPEN_FILE 0x02 +#define CONFIG_REOPEN_DSET 0x04 +#define CONFIG_DIRECT_WRITE 0x08 +#define CONFIG_DIRECT_READ 0x10 +#define CONFIG_END 0x20 + /* Defines used in test_single_chunk_latest() */ #define FILE "single_latest.h5" #define DATASET "dataset" @@ -1973,7 +1981,7 @@ error: } /* test_read_unallocated_chunk() */ /*------------------------------------------------------------------------- - * Function: test_single_chunk_latest + * Function: test_single_chunk * * Purpose: This is to verify the fix for jira issue HDFFV-10425. * The problem was due to a bug in the internal ilbrary routine @@ -1989,13 +1997,16 @@ error: * index for the dataset. * Verify that the data read is the same as the written data. * + * Since expanded to test multiple combinations of cases + * involving a single chunk + * * Return: Success: 0 * Failure: 1 * *------------------------------------------------------------------------- */ static int -test_single_chunk_latest(void) +test_single_chunk(unsigned config) { hid_t fid; /* File ID */ hid_t fapl; /* File access property list ID */ @@ -2005,11 +2016,12 @@ test_single_chunk_latest(void) hsize_t dims[2] = {DIM0, DIM1}; /* Dimension sizes */ hsize_t chunk[2] = {CHUNK0, CHUNK1}; /* Chunk dimension sizes */ hsize_t offset[2] = {0,0}; /* Offset for writing */ + uint32_t filters; /* Filter mask out */ int wdata[DIM0][DIM1]; /* Write buffer */ int rdata[DIM0][DIM1]; /* Read buffer */ int i, j; /* Local index variable */ - TESTING("H5Dwrite_chunk with single chunk and latest format"); + TESTING("Single chunk I/O"); /* Initialize data */ for (i=0; i"); + if(config & CONFIG_LATEST) { + if(need_comma) + printf(", "); + printf("latest format"); + need_comma = TRUE; + } /* end if */ + if(config & CONFIG_REOPEN_FILE) { + if(need_comma) + printf(", "); + printf("reopen file"); + need_comma = TRUE; + } /* end if */ + else if(config & CONFIG_REOPEN_DSET) { + if(need_comma) + printf(", "); + printf("reopen dataset"); + need_comma = TRUE; + } /* end if */ + if(config & CONFIG_DIRECT_WRITE) { + if(need_comma) + printf(", "); + printf("direct write"); + need_comma = TRUE; + } /* end if */ + if(config & CONFIG_DIRECT_READ) { + if(need_comma) + printf(", "); + printf("direct read"); + need_comma = TRUE; + } /* end if */ + printf(":\n"); + fflush(stdout); + + nerrors += test_single_chunk(config); + } /* end for */ if(H5Fclose(file_id) < 0) goto error; @@ -2165,3 +2245,4 @@ error: HDputs("*** TESTS FAILED ***"); return EXIT_FAILURE; } + -- cgit v0.12