On Thu, May 15, 2025 at 11:08:55PM -0700, Dan Williams wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofi...@intel.com> > > > > monitor.sh runs for 50 seconds and spends 48 of those seconds sleeping > > after sync. It sleeps for 3 seconds each time it restarts the monitor, > > and 3 seconds before checking for expected log entries, a total of > > 16 naps. > > > > Replace the sleeps with polling that waits for a max of 3 seconds but > > does so in 30 0.1 second intervals. This leaves the current behavior > > in place but offers a shorter run time for system configs capable of > > a faster sync. > > > > Again - I'd like to see some Tested-by's on this one because it wouldn't > > be the first time my golden environment wasn't representative of all > > environments where these tests are run. Thanks! > > > > Signed-off-by: Alison Schofield <alison.schofi...@intel.com> > > --- > > > > Changes in v2: > > - Poll for 3 seconds instead of removing sleep entirely (MarcH) > > - Update commit msg & log > > Link to v1: > > https://lore.kernel.org/nvdimm/20250514014133.1431846-1-alison.schofi...@intel.com/ > > > > test/monitor.sh | 43 ++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 40 insertions(+), 3 deletions(-) > > > > diff --git a/test/monitor.sh b/test/monitor.sh > > index be8e24d6f3aa..61cad098d87c 100755 > > --- a/test/monitor.sh > > +++ b/test/monitor.sh > > @@ -21,12 +21,45 @@ trap 'err $LINENO' ERR > > > > check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor > > service" > > > > +wait_for_logfile_update() > > +{ > > + local file="$1" > > + local prev_size="$2" > > + local timeout=30 > > + local i=0 > > + > > + # prev_size is always zero because start_monitor truncates it. > > + # Set and check against it anyway to future proof. > > + while [ $i -lt $timeout ]; do > > + local new_size=$(stat -c%s "$file" 2>/dev/null || echo 0) > > + if [ "$new_size" -gt "$prev_size" ]; then > > + return 0 > > + fi > > + sleep 0.1 > > + i=$((i+1)) > > + done > > + > > + echo "logfile not updated within 3 seconds" > > + err "$LINENO" > > Hmm... not a fan of this open coded "wait for file to change" bash > function. This feels like something that a tool can do... (searches) > > Does inotifywait fit the bill here?
It's not capable of knowing what happened before it started and there is no synchronizing. So a lot of extra checking when using it. v3 will use 'tail -F' and check for the expected number of timestamps. > > https://linux.die.net/man/1/inotifywait > > > +} > > + > > start_monitor() > > { > > logfile=$(mktemp) > > $NDCTL monitor -c "$monitor_conf" -l "$logfile" $1 & > > monitor_pid=$! > > - sync; sleep 3 > > + sync > > + for i in {1..30}; do > > + if ps -p "$monitor_pid" > /dev/null; then > > + sleep 0.1 > > + break > > + fi > > + sleep 0.1 > > + done > > + if ! ps -p "$monitor_pid" > /dev/null; then > > + echo "monitor not ready within 3 seconds" > > + err "$LINENO" > > + fi > > This does not make sense to. The shell got the pid from the launching > the executable. This is effectively testing that bash command execution > works. About the only use I can imagine for this is checking that the > monitor did not die early, but that should be determined by other parts > of the test. Understood. I couldn't find any clear 'signal' that monitor is ready. So, I've added one in a v3.
