From 4b316ebcca7cf4649426e2f1b39f26d567e7d87b Mon Sep 17 00:00:00 2001 From: wkliao Date: Thu, 3 Sep 2026 16:59:50 -0500 Subject: [PATCH 1/3] Bug fix: check buf == NULL for zero-sized request varp is needed to tell if a variable is a record variable. With this change, varid must be valid when reaching to put_varm(). Another fix is required to return error earlier in dispatcher when a invalid varid is detected. --- src/drivers/ncmpio/ncmpio_getput.m4 | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/drivers/ncmpio/ncmpio_getput.m4 b/src/drivers/ncmpio/ncmpio_getput.m4 index a2850ed812..7678397253 100644 --- a/src/drivers/ncmpio/ncmpio_getput.m4 +++ b/src/drivers/ncmpio/ncmpio_getput.m4 @@ -122,7 +122,7 @@ put_varm(NC *ncp, MPI_Offset nelems=0, bnelems=0, nbytes=0; MPI_Datatype itype, imaptype; - if (varp == NULL) { /* zero-sized request */ + if (buf == NULL) { /* zero-sized request */ itype = MPI_BYTE; el_size = 0; bnelems = 0; @@ -345,7 +345,7 @@ get_varm(NC *ncp, MPI_Offset nelems=0, bnelems=0, nbytes=0; MPI_Datatype itype, imaptype=MPI_DATATYPE_NULL; - if (varp == NULL) { /* zero-sized request */ + if (buf == NULL) { /* zero-sized request */ itype = MPI_BYTE; el_size = 0; bnelems = 0; @@ -523,22 +523,20 @@ ncmpio_$1_var(void *ncdp, } } - /* sanity check has been done at dispatchers */ + /* obtain NC_var object pointer, varp. Note sanity check for ncdp and + * varid has been done in dispatchers + */ + varp = ncp->vars.value[varid]; if (fIsSet(reqMode, NC_REQ_ZERO) && fIsSet(reqMode, NC_REQ_COLL)) { /* In case some processes in an aggregation group have nothing to * write, they still need to participate the communication part of the * intra-node aggregation operation. */ - return $1_varm(ncp, NULL, NULL, NULL, NULL, imap, NULL, 0, + return $1_varm(ncp, varp, NULL, NULL, NULL, imap, NULL, 0, buftype, reqMode); } - /* obtain NC_var object pointer, varp. Note sanity check for ncdp and - * varid has been done in dispatchers - */ - varp = ncp->vars.value[varid]; - #if PNETCDF_SUBFILING == 1 /* call a separate routine if variable is stored in subfiles */ if (varp->num_subfiles > 1) { From dc29b831e9046282b5a89ec1374a923b284bf526 Mon Sep 17 00:00:00 2001 From: wkliao Date: Thu, 3 Sep 2026 17:03:43 -0500 Subject: [PATCH 2/3] Check varid and return immediately if it is invalid When writing to a record variable, a collective call to MPI_Allreduce() is necessary to update and sync-ed the number of records among all processes. For fix-sized variables, such a call to MPI_Allreduce() is not necessary. The argument varid of a put API supplied by the user may point to either a fix-sized or record variable. Because there is no way to tell if an invalid varid is a fix-sized variable or a record variable, there is no consistent if-condition can be devised to determine whether to call MPI_Allreduce(). The solution adopted by this commit adds a call to MPI_Allreduce() to check the error returned from sanity_check(), which unfortunately adds some communication cost. Luckily, this fix is only necessary for collective blocking put APIs. --- src/dispatchers/var_getput.m4 | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/dispatchers/var_getput.m4 b/src/dispatchers/var_getput.m4 index d7756b0c7b..3b26cca294 100644 --- a/src/dispatchers/var_getput.m4 +++ b/src/dispatchers/var_getput.m4 @@ -353,6 +353,16 @@ APINAME($1,$2,$3,$4)(int ncid, err = sanity_check(pncp, varid, IO_TYPE($1), ITYPE2MPI($3), IS_COLL($4)); + ifelse(`$4',`_all',`ifelse(`$1',`put',` + /* If varid is invalid and this is collective put API, we must return now. + * This is because there is a call to MPI_Allreduce() at the end to update + * the number of records. An invalid varid may result in only a subset of + * MPI processes calling MPI_Allreduce(). There is no way to tell the + * provide varid if for fix-sized or record variable. + */ + if (allreduce_error(pncp, err) != NC_NOERR) return err; + ')') + ifelse(`$2',`m',`if (imap == NULL && stride != NULL) api_kind = API_VARS; else if (imap == NULL && stride == NULL) api_kind = API_VARA;', `$2',`s',`if (stride == NULL) api_kind = API_VARA;') @@ -469,7 +479,16 @@ NAPINAME($1,$2,$3)(int ncid, if (err != NC_NOERR) return err; err = sanity_check(pncp, varid, IO_TYPE($1), ITYPE2MPI($2), IS_COLL($3)); - if (err != NC_NOERR) goto err_check; + + ifelse(`$3',`_all',`ifelse(`$1',`put',` + /* If varid is invalid and this is collective put API, we must return now. + * This is because there is a call to MPI_Allreduce() at the end to update + * the number of records. An invalid varid may result in only a subset of + * MPI processes calling MPI_Allreduce(). There is no way to tell the + * provide varid if for fix-sized or record variable. + */ + if (allreduce_error(pncp, err) != NC_NOERR) return err; + ')',`if (err != NC_NOERR) goto err_check;') if (num == 0) goto err_check; From 414b7ab34609cc61887abbd365eaf2b32590ef6b Mon Sep 17 00:00:00 2001 From: wkliao Date: Thu, 3 Sep 2026 17:29:41 -0500 Subject: [PATCH 3/3] tst_zero_len_recvar.c - zero-length request in a collective blocking put API This test program was provided by Blaise Bourdin (and revised a little). It calls a collective blocking put API to write to a record variable where one of the processes makes a zero-length request. When writing to a record variable, PnetCDF calls MPI_Allreduce to update and sync the number of records among all processes. If a subset of the processes has a zero-length request, these processes may not participate the call to MPI_Allreduce, causing program to hang. See PR #239. --- test/testcases/Makefile.am | 3 +- test/testcases/tst_zero_len_recvar.c | 165 +++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 test/testcases/tst_zero_len_recvar.c diff --git a/test/testcases/Makefile.am b/test/testcases/Makefile.am index 0f3b7fd8a6..91dd461d26 100644 --- a/test/testcases/Makefile.am +++ b/test/testcases/Makefile.am @@ -104,7 +104,8 @@ check_PROGRAMS = file_create_open \ put_all_kinds \ redef1 \ iput_all_kinds \ - tst_version + tst_version \ + tst_zero_len_recvar M4_SRCS = put_all_kinds.m4 \ erange_fill.m4 \ diff --git a/test/testcases/tst_zero_len_recvar.c b/test/testcases/tst_zero_len_recvar.c new file mode 100644 index 0000000000..4a4ddc8f08 --- /dev/null +++ b/test/testcases/tst_zero_len_recvar.c @@ -0,0 +1,165 @@ +/****************************************************************************** + * + * Copyright (C) 2026, Northwestern University and Argonne National Laboratory + * See COPYRIGHT notice in top-level directory. + * + *****************************************************************************/ + +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * This program tests a collective write to a record variable in which one of + * the processes has nothing to contribute, i.e. it passes count == 0. Every + * process still calls the collective API, so the call is legal. + * + * Such a request must not opt out of the collective synchronization of numrecs + * that follows the I/O. If it does, the processes that do have data to write + * are left calling an MPI_Allreduce() alone and the communicator goes out of + * step, which shows up as a wrong error code, as corruption of a later + * collective call, or as a hang. + * + * Fixed-size variables take no part in the numrecs synchronization and are + * therefore not affected; the record variable is the case worth guarding. + * + * To compile: + * mpicc -O2 tst_zero_len_recvar.c -o tst_zero_len_recvar -lpnetcdf + * + * % mpiexec -n 4 ./tst_zero_len_recvar testfile.nc + * + * This test program was contributed by Blaise Bourdin. See the github pull + * request in https://github.com/Parallel-NetCDF/PnetCDF/pull/238 + * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + +#include +#include +#include +#include /* basename() */ +#include + +#include + +#define NDBL 4 /* number of doubles written per process */ + +static +int test_io(const char *out_path, + const char *in_path, /* ignored */ + int format, + int coll_io, + MPI_Info info) +{ + char val[MPI_MAX_INFO_VAL]; + int i, err, nerrs=0, ncid, dimid[2], varid, rank, nprocs, is_empty; + int flag, bb_enabled=0; + MPI_Offset start[2], count[2], numrecs; + double buf[NDBL], rbuf[NDBL]; + + MPI_Comm_rank(MPI_COMM_WORLD, &rank); + MPI_Comm_size(MPI_COMM_WORLD, &nprocs); + + /* check whether burst buffering is enabled */ + MPI_Info_get(info, "nc_burst_buf", MPI_MAX_INFO_VAL - 1, val, &flag); + if (flag && strcasecmp(val, "enable") == 0) { + bb_enabled = 1; + /* does not work for NetCDF4 files when burst-buffering is enabled */ + if (format == NC_FORMAT_NETCDF4 || format == NC_FORMAT_NETCDF4_CLASSIC) + return 0; + } + + /* the last process contributes nothing; with a single process there is + * nobody left to write, so keep its request non-empty + */ + is_empty = (nprocs > 1 && rank == nprocs - 1); + + for (i=0; i