Hi, Stephen,

Stephen Bates <sbates@raithlin.com> writes:

> It is useful to be able to run blktrace in a way that it stops when a
> certain number of events is reached. This patch adds a new -e/--events
> command line argument. Note this limit is per block device and can be
> used in combination with the -w/--stopwatch argument.
>
> Documentation is updated to include the new argument.
>
> Signed-off-by: Stephen Bates <sbates@raithlin.com>
> ---
>  README           |  3 ++-
>  blktrace.c       | 49 +++++++++++++++++++++++++++++++++++++++++++++++-
>  doc/blktrace.8   | 15 +++++++++++++--
>  doc/blktrace.tex |  7 ++++++-
>  4 files changed, 69 insertions(+), 5 deletions(-)
>

[...]

> diff --git a/blktrace.c b/blktrace.c
> index 038b2cb..bc3fe84 100644
> --- a/blktrace.c
> +++ b/blktrace.c
> @@ -282,6 +282,8 @@ static int pagesize;
>  static int act_mask = ~0U;
>  static int kill_running_trace;
>  static int stop_watch;
> +static unsigned long long  events;

I'm not sure events needs to be that large, but I guess it's fine.
There is an extra space in there, though.

> +static volatile int events_done;

[...]

> @@ -1862,6 +1872,34 @@ static void *thread_main(void *arg)
>  		else if (ndone < 0 && errno != EINTR)
>  			fprintf(stderr, "Thread %d poll failed: %d/%s\n",
>  				tp->cpu, errno, strerror(errno));
> +
> +		if (ndone && events) {
> +			struct list_head *p;
> +			unsigned long long nevents;
> +
> +			__list_for_each(p, &devpaths) {
> +				int cpu;
> +				struct pdc_stats *sp;
> +				struct devpath *dpp = list_entry(p, struct devpath, head);
> +				nevents = 0;
> +
> +				for (cpu = 0, sp = dpp->stats; cpu < dpp->ncpus; cpu++, sp++) {

> +					/*
> +					 * Estimate events if not known...
> +					 */
> +					if (sp->nevents == 0) {
> +						nevents += sp->data_read /
> +							sizeof(struct blk_io_trace);
> +					} else
> +						nevents += sp->nevents;

That bit was copied from show_stats() and can probably be factored out.

> +
> +				}
> +				if (events && (nevents >= events))

redundant check for non-zero 'events'.

> +					events_done = 1;

Why continue to loop if you've already established that the event count
was reached?

> +			}
> +			if (events_done)
> +				tp->is_done = 1;
> +		}
>  	}

It would be nice to keep to 80 columns.  This whole stanza could
probably be its own function.

More importantly, I think you may run into trouble, here.  tp->is_done
is set to 1, which breaks out of the loop, but then this code runs:

	/*
	 * Trace is stopped, pull data until we get a short read
	 */
	while (handle_pfds(tp, ndevs, 1) > 0)
		;

which has the potential to run indefinitely if your storage is busy, I
think.  Have a look at what stop_tracers() does (which is currently the
only place tp->is_done gets set).  I just tested your patch and
confirmed that blktrace does not exit for me, even after I stop my
workload.

Next, the code is reading through per-cpu data without any coordination
with the writers.  I think it's fine, but I do expect a comment
explaining what it's doing and why it is safe.

Here's a patch that shows what I'm thinking.  It is not in final form (I
didn't modify show_stats() to used the helper function, for example),
but please feel free to work from there if it's convenient for you.  I
did test it lightly, and it at least exits after a number of events.
Note that if you want to specify a small number of events, you'll also
have to restrict the buffer size.  The default of 512k will hold over
10,000 events (per cpu).

Cheers,
Jeff