From dcbf871fed9a07211458188fb52174c72d93f759 Mon Sep 17 00:00:00 2001 From: Quincey Koziol Date: Thu, 8 Oct 2009 23:09:34 -0500 Subject: [svn-r17624] Description: Don't allow reads to change or add to the metadata accumulator, since they might be speculative and could bring raw data into the metadata accumulator. Tested on: FreeBSD/32 6.3 (duty) in debug mode FreeBSD/64 6.3 (liberty) w/C++ & FORTRAN, in debug mode Linux/32 2.6 (jam) w/PGI compilers, w/default API=1.8.x, w/C++ & FORTRAN, w/threadsafe, in debug mode Linux/64-amd64 2.6 (smirom) w/Intel compilers, w/default API=1.6.x, w/C++ & FORTRAN, in production mode Solaris/32 2.10 (linew) w/deprecated symbols disabled, w/C++ & FORTRAN, w/szip filter, in production mode Linux/64-ia64 2.6 (cobalt) w/Intel compilers, w/C++ & FORTRAN, in production mode Linux/64-ia64 2.4 (tg-login3) w/parallel, w/FORTRAN, in debug mode Linux/64-amd64 2.6 (abe) w/parallel, w/FORTRAN, in production mode Mac OS X/32 10.6.1 (amazon) in debug mode Mac OS X/32 10.6.1 (amazon) w/C++ & FORTRAN, w/threadsafe, in production mode --- MANIFEST | 2 + src/H5Faccum.c | 70 ++++------------------------------- test/Makefile.am | 2 +- test/Makefile.in | 17 +++++++-- test/gen_specmetaread.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++ test/specmetaread.h5 | Bin 0 -> 1752 bytes test/tmisc.c | 67 +++++++++++++++++++++++++++++++++ 7 files changed, 187 insertions(+), 67 deletions(-) create mode 100644 test/gen_specmetaread.c create mode 100644 test/specmetaread.h5 diff --git a/MANIFEST b/MANIFEST index 16714aa..1c69313 100644 --- a/MANIFEST +++ b/MANIFEST @@ -851,6 +851,7 @@ ./test/gen_old_group.c _DO_NOT_DISTRIBUTE_ ./test/gen_old_layout.c _DO_NOT_DISTRIBUTE_ ./test/gen_old_mtime.c _DO_NOT_DISTRIBUTE_ +./test/gen_specmetaread.c _DO_NOT_DISTRIBUTE_ ./test/gen_udlinks.c _DO_NOT_DISTRIBUTE_ ./test/getname.c ./test/gheap.c @@ -876,6 +877,7 @@ ./test/pool.c ./test/set_extent.c ./test/space_overflow.c _DO_NOT_DISTRIBUTE_ +./test/specmetaread.h5 ./test/stab.c ./test/tarray.c ./test/tarrold.h5 diff --git a/src/H5Faccum.c b/src/H5Faccum.c index d8b9820..e525a08 100644 --- a/src/H5Faccum.c +++ b/src/H5Faccum.c @@ -99,6 +99,10 @@ H5FL_BLK_DEFINE_STATIC(meta_accum); * Purpose: Attempts to read some data from the metadata accumulator for * a file into a buffer. * + * Note: We can't change (or add to) the metadata accumulator, because + * this might be a speculative read and could possibly read raw + * data into the metadata accumulator. + * * Return: Non-negative on success/Negative on failure * * Programmer: Quincey Koziol @@ -185,69 +189,11 @@ H5F_accum_read(const H5F_t *f, hid_t dxpl_id, H5FD_mem_t type, haddr_t addr, /* Make certain we've read it all */ HDassert(size == 0); } /* end if */ - /* Current read doesn't overlap with metadata accumulator, read it into accumulator */ + /* Current read doesn't overlap with metadata accumulator, read it from file */ else { - /* Only update the metadata accumulator if it is not dirty or if - * we are allowed to write the accumulator out during reads (when - * it is dirty) - */ - if(f->shared->feature_flags & H5FD_FEAT_ACCUMULATE_METADATA_READ || !f->shared->accum.dirty) { - /* Flush current contents, if dirty */ - if(f->shared->accum.dirty) { - if(H5FD_write(f->shared->lf, dxpl_id, H5FD_MEM_DEFAULT, f->shared->accum.loc, f->shared->accum.size, f->shared->accum.buf) < 0) - HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "driver write request failed") - - /* Reset accumulator dirty flag */ - f->shared->accum.dirty = FALSE; - } /* end if */ - - /* Cache the new piece of metadata */ - /* Check if we need to resize the buffer */ - if(size > f->shared->accum.alloc_size) { - size_t new_size; /* New size of accumulator */ - - /* Adjust the buffer size to be a power of 2 that is large enough to hold data */ - new_size = (size_t)1 << (1 + H5V_log2_gen((uint64_t)(size - 1))); - - /* Grow the metadata accumulator buffer */ - if(NULL == (f->shared->accum.buf = H5FL_BLK_REALLOC(meta_accum, f->shared->accum.buf, new_size))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "unable to allocate metadata accumulator buffer") - - /* Note the new buffer size */ - f->shared->accum.alloc_size = new_size; - } /* end if */ - else { - /* Check if we should shrink the accumulator buffer */ - if(size < (f->shared->accum.alloc_size / H5F_ACCUM_THROTTLE) && - f->shared->accum.alloc_size > H5F_ACCUM_THRESHOLD) { - size_t new_size = (f->shared->accum.alloc_size / H5F_ACCUM_THROTTLE); /* New size of accumulator buffer */ - - /* Shrink the accumulator buffer */ - if(NULL == (f->shared->accum.buf = H5FL_BLK_REALLOC(meta_accum, f->shared->accum.buf, new_size))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "unable to allocate metadata accumulator buffer") - - /* Note the new buffer size */ - f->shared->accum.alloc_size = new_size; - } /* end if */ - } /* end else */ - - /* Update accumulator information */ - f->shared->accum.loc = addr; - f->shared->accum.size = size; - f->shared->accum.dirty = FALSE; - - /* Read into accumulator */ - if(H5FD_read(f->shared->lf, dxpl_id, H5FD_MEM_DEFAULT, f->shared->accum.loc, f->shared->accum.size, f->shared->accum.buf) < 0) - HGOTO_ERROR(H5E_IO, H5E_READERROR, FAIL, "driver read request failed") - - /* Copy into buffer */ - HDmemcpy(buf, f->shared->accum.buf, size); - } /* end if */ - else { - /* Dispatch to driver */ - if(H5FD_read(f->shared->lf, dxpl_id, type, addr, size, buf) < 0) - HGOTO_ERROR(H5E_IO, H5E_READERROR, FAIL, "driver read request failed") - } /* end else */ + /* Dispatch to driver */ + if(H5FD_read(f->shared->lf, dxpl_id, type, addr, size, buf) < 0) + HGOTO_ERROR(H5E_IO, H5E_READERROR, FAIL, "driver read request failed") } /* end else */ /* Indicate success */ diff --git a/test/Makefile.am b/test/Makefile.am index d233f96..ec1eed7 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -62,7 +62,7 @@ check_PROGRAMS=$(TEST_PROG) error_test err_compat tcheck_version testmeta # so do not appear in this list. BUILD_ALL_PROGS=gen_bad_ohdr gen_bogus gen_cross gen_deflate gen_filters gen_new_array \ gen_new_fill gen_new_group gen_new_mtime gen_new_super gen_noencoder \ - gen_nullspace gen_udlinks space_overflow gen_filespace + gen_nullspace gen_udlinks space_overflow gen_filespace gen_specmetaread if BUILD_ALL_CONDITIONAL noinst_PROGRAMS=$(BUILD_ALL_PROGS) diff --git a/test/Makefile.in b/test/Makefile.in index e1ed182..3f063e9 100644 --- a/test/Makefile.in +++ b/test/Makefile.in @@ -92,7 +92,8 @@ am__EXEEXT_2 = gen_bad_ohdr$(EXEEXT) gen_bogus$(EXEEXT) \ gen_new_group$(EXEEXT) gen_new_mtime$(EXEEXT) \ gen_new_super$(EXEEXT) gen_noencoder$(EXEEXT) \ gen_nullspace$(EXEEXT) gen_udlinks$(EXEEXT) \ - space_overflow$(EXEEXT) gen_filespace$(EXEEXT) + space_overflow$(EXEEXT) gen_filespace$(EXEEXT) \ + gen_specmetaread$(EXEEXT) PROGRAMS = $(noinst_PROGRAMS) app_ref_SOURCES = app_ref.c app_ref_OBJECTS = app_ref.$(OBJEXT) @@ -246,6 +247,10 @@ gen_nullspace_SOURCES = gen_nullspace.c gen_nullspace_OBJECTS = gen_nullspace.$(OBJEXT) gen_nullspace_LDADD = $(LDADD) gen_nullspace_DEPENDENCIES = libh5test.la $(LIBHDF5) +gen_specmetaread = gen_specmetaread.c +gen_specmetaread_OBJECTS = gen_specmetaread.$(OBJEXT) +gen_specmetaread_LDADD = $(LDADD) +gen_specmetaread_DEPENDENCIES = libh5test.la $(LIBHDF5) gen_udlinks_SOURCES = gen_udlinks.c gen_udlinks_OBJECTS = gen_udlinks.$(OBJEXT) gen_udlinks_LDADD = $(LDADD) @@ -377,7 +382,7 @@ SOURCES = $(libh5test_la_SOURCES) app_ref.c big.c bittests.c btree2.c \ mount.c mtime.c ntypes.c objcopy.c ohdr.c pool.c reserved.c \ set_extent.c space_overflow.c stab.c tcheck_version.c \ $(testhdf5_SOURCES) testmeta.c $(ttsafe_SOURCES) unlink.c \ - vfd.c + vfd.c gen_specmetaread.c DIST_SOURCES = $(libh5test_la_SOURCES) app_ref.c big.c bittests.c \ btree2.c cache.c cache_api.c cmpd_dset.c cross_read.c dangle.c \ dsets.c dt_arith.c dtransform.c dtypes.c earray.c enum.c \ @@ -390,7 +395,7 @@ DIST_SOURCES = $(libh5test_la_SOURCES) app_ref.c big.c bittests.c \ istore.c lheap.c links.c mf.c mount.c mtime.c ntypes.c \ objcopy.c ohdr.c pool.c reserved.c set_extent.c \ space_overflow.c stab.c tcheck_version.c $(testhdf5_SOURCES) \ - testmeta.c $(ttsafe_SOURCES) unlink.c vfd.c + testmeta.c $(ttsafe_SOURCES) unlink.c vfd.c gen_specmetaread.c ETAGS = etags CTAGS = ctags am__tty_colors = \ @@ -712,7 +717,7 @@ TEST_PROG = testhdf5 lheap ohdr stab gheap cache cache_api \ # so do not appear in this list. BUILD_ALL_PROGS = gen_bad_ohdr gen_bogus gen_cross gen_deflate gen_filters gen_new_array \ gen_new_fill gen_new_group gen_new_mtime gen_new_super gen_noencoder \ - gen_nullspace gen_udlinks space_overflow gen_filespace + gen_nullspace gen_udlinks space_overflow gen_filespace gen_specmetaread # The libh5test library provides common support code for the tests. @@ -937,6 +942,9 @@ gen_noencoder$(EXEEXT): $(gen_noencoder_OBJECTS) $(gen_noencoder_DEPENDENCIES) gen_nullspace$(EXEEXT): $(gen_nullspace_OBJECTS) $(gen_nullspace_DEPENDENCIES) @rm -f gen_nullspace$(EXEEXT) $(LINK) $(gen_nullspace_OBJECTS) $(gen_nullspace_LDADD) $(LIBS) +gen_specmetaread$(EXEEXT): $(gen_specmetaread_OBJECTS) $(gen_specmetaread_DEPENDENCIES) + @rm -f gen_specmetaread$(EXEEXT) + $(LINK) $(gen_specmetaread_OBJECTS) $(gen_specmetaread_LDADD) $(LIBS) gen_udlinks$(EXEEXT): $(gen_udlinks_OBJECTS) $(gen_udlinks_DEPENDENCIES) @rm -f gen_udlinks$(EXEEXT) $(LINK) $(gen_udlinks_OBJECTS) $(gen_udlinks_LDADD) $(LIBS) @@ -1055,6 +1063,7 @@ distclean-compile: @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gen_new_super.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gen_noencoder.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gen_nullspace.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gen_specmetaread.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gen_udlinks.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/getname.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gheap.Po@am__quote@ diff --git a/test/gen_specmetaread.c b/test/gen_specmetaread.c new file mode 100644 index 0000000..f489119 --- /dev/null +++ b/test/gen_specmetaread.c @@ -0,0 +1,96 @@ +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * Copyright by The HDF Group. * + * Copyright by the Board of Trustees of the University of Illinois. * + * All rights reserved. * + * * + * This file is part of HDF5. The full HDF5 copyright notice, including * + * terms governing use, modification, and redistribution, is contained in * + * the files COPYING and Copyright.html. COPYING can be found at the root * + * of the source code distribution tree; Copyright.html can be found at the * + * root level of an installed copy of the electronic HDF5 document set and * + * is linked from the top-level documents page. It can also be found at * + * http://hdfgroup.org/HDF5/doc/Copyright.html. If you do not have * + * access to either file, you may request a copy from help@hdfgroup.org. * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + +/* + * Programmer: Quincey Koziol + * Thursday, October 8, 2009 + * + * Purpose: Create a file with a dataset who's raw data immediately follows + * its object header, so that when the dataset is unlinked from its parent + * group, a speculative read of the object header would get the raw data + * into the metadata accumulator, "polluting" it. + * To build the test file, this program MUST be compiled and linked with + * the library on the trunk as of when this file is checked in. + */ + +#include "hdf5.h" +#include + +#define FILENAME "specmetaread.h5" +#define DIM 10 + +int +main(void) +{ + hid_t fid; + hid_t fapl; + hid_t did; + hid_t space; + hsize_t dim[1] = {DIM}; + unsigned data[DIM]; + unsigned u; + herr_t ret; /* Generic return value */ + + /* Initialize the data */ + for(u = 0; u < DIM; u++) + data[u] = u; + + /* Create a FAPL with the metadata and small data aggregators turned off */ + fapl = H5Pcreate(H5P_FILE_ACCESS); + assert(fapl > 0); + ret = H5Pset_meta_block_size(fapl, (hsize_t)0); + assert(ret >= 0); + ret = H5Pset_small_data_block_size(fapl, (hsize_t)0); + assert(ret >= 0); + + /* Create file */ + fid = H5Fcreate(FILENAME, H5F_ACC_TRUNC, H5P_DEFAULT, fapl); + assert(fid > 0); + + /* Close FAPL */ + ret = H5Pclose(fapl); + assert(ret >= 0); + + /* Create dataspace */ + space = H5Screate_simple(1, dim, NULL); + assert(space > 0); + + /* Create dataset #1 */ + did = H5Dcreate2(fid, "dset1", H5T_NATIVE_UINT, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT); + assert(did > 0); + ret = H5Dwrite(did, H5T_NATIVE_UINT, H5S_ALL, H5S_ALL, H5P_DEFAULT, data); + assert(ret >= 0); + ret = H5Dclose(did); + assert(ret >= 0); + + /* Create dataset #2 */ + did = H5Dcreate2(fid, "dset2", H5T_NATIVE_UINT, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT); + assert(did > 0); + ret = H5Dwrite(did, H5T_NATIVE_UINT, H5S_ALL, H5S_ALL, H5P_DEFAULT, data); + assert(ret >= 0); + ret = H5Dclose(did); + assert(ret >= 0); + + /* Close dataspace */ + ret = H5Sclose(space); + assert(ret >= 0); + + /* Close file */ + ret = H5Fclose(fid); + assert(ret >= 0); + + return 0; +} + diff --git a/test/specmetaread.h5 b/test/specmetaread.h5 new file mode 100644 index 0000000..50b0e69 Binary files /dev/null and b/test/specmetaread.h5 differ diff --git a/test/tmisc.c b/test/tmisc.c index 41128a2..21715b4 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -32,6 +32,9 @@ #include "testhdf5.h" #include "H5Dpkg.h" /* Datasets */ +#define NAME_BUF_SIZE 1024 +#define READ_OLD_BUFSIZE 1024 + /* Definitions for misc. test #1 */ #define MISC1_FILE "tmisc1.h5" #define MISC1_VAL (13417386) /* 0xccbbaa */ @@ -305,6 +308,11 @@ unsigned m13_rdata[MISC13_DIM1][MISC13_DIM2]; /* Data read from dataset #define MISC28_SIZE 10 #define MISC28_NSLOTS 10000 +/* Definitions for misc. test #29 */ +#define MISC29_ORIG_FILE "specmetaread.h5" +#define MISC29_COPY_FILE "tmisc29.h5" +#define MISC29_DSETNAME "dset2" + /**************************************************************** ** ** test_misc1(): test unlinking a dataset from a group and immediately @@ -5106,6 +5114,63 @@ test_misc28(void) CHECK_I(ret, "H5Pclose"); } /* end test_misc28() */ + +/**************************************************************** +** +** test_misc29(): Ensure that speculative metadata reads don't +** get raw data into the metadata accumulator. +** +****************************************************************/ +static void +test_misc29(void) +{ + int fd_old = (-1), fd_new = (-1); /* File descriptors for copying data */ + char buf[READ_OLD_BUFSIZE]; /* Buffer for copying data */ + ssize_t nread; /* Number of bytes read in */ + char *srcdir = HDgetenv("srcdir"); /* where the src code is located */ + char filename[NAME_BUF_SIZE] = ""; /* old test file name */ + hid_t fid; /* File ID */ + herr_t ret; /* Generic return value */ + + /* Output message about test being performed */ + MESSAGE(5, ("Speculative metadata reads\n")); + + /* Generate correct name for test file by prepending the source path */ + if(srcdir && ((HDstrlen(srcdir) + HDstrlen(MISC29_ORIG_FILE) + 1) < sizeof(filename))) { + HDstrcpy(filename, srcdir); + HDstrcat(filename, "/"); + } /* end if */ + HDstrcat(filename, MISC29_ORIG_FILE); + + /* Copy old file into temporary file */ + fd_old = HDopen(filename, O_RDONLY, 0666); + CHECK(fd_old, -1, "HDopen"); + fd_new = HDopen(MISC29_COPY_FILE, O_RDWR|O_CREAT|O_TRUNC, 0666); + CHECK(fd_new, -1, "HDopen"); + + /* Copy data */ + while((nread = HDread(fd_old, buf, (size_t)READ_OLD_BUFSIZE)) > 0) + HDwrite(fd_new, buf, (size_t)nread); + + /* Close files */ + ret = HDclose(fd_old); + CHECK(ret, -1, "HDclose"); + ret = HDclose(fd_new); + CHECK(ret, -1, "HDclose"); + + /* Open the copied file */ + fid = H5Fopen(MISC29_COPY_FILE, H5F_ACC_RDWR, H5P_DEFAULT); + CHECK(fid, FAIL, "H5Fopen"); + + /* Delete the last dataset */ + ret = H5Ldelete(fid, MISC29_DSETNAME, H5P_DEFAULT); + CHECK(ret, FAIL, "H5Ldelete"); + + /* Close the file */ + ret = H5Fclose(fid); + CHECK(ret, FAIL, "H5Fclose"); +} /* end test_misc29() */ + /**************************************************************** ** ** test_misc(): Main misc. test routine. @@ -5149,6 +5214,7 @@ test_misc(void) test_misc26(); /* Test closing property lists with long filter pipelines */ test_misc27(); /* Test opening file with object that has bad # of object header messages */ test_misc28(); /* Test that chunks are cached appropriately */ + test_misc29(); /* Test that speculative metadata reads are handled correctly */ } /* test_misc() */ @@ -5203,5 +5269,6 @@ cleanup_misc(void) HDremove(MISC25C_FILE); HDremove(MISC26_FILE); HDremove(MISC28_FILE); + HDremove(MISC29_COPY_FILE); } -- cgit v0.12