Re: [Patch 1/13] iostats

[Theodore Tso <tytso@mit.edu>]

On Tue, Jul 24, 2007 at 04:34:36PM +0530, Girish Shilamkar wrote:
>  
> +struct struct_io_stats {
> +	unsigned long long reads;
> +	unsigned long long writes;
> +};

I'd suggest doing something like this instead:

struct struct_io_stats {
	int	num_fields;
	int	reserved;
	unsigned long long bytes_read;
	unsigned long long bytes_written;
};

There are other statistics that you might want to gather.  For
example, "read_requests" and "write_requests".  Or perhaps some
statistics based on discontiguous read/writes (i.e., i/o operations
that cause seeks).  Also, in the future we might want to add some kind
of readahead functionality, and that would probably require more
statistics as well.

> +	memset(&track->io_start, 0, sizeof(struct struct_io_stats));
> +	if (channel && channel->manager && channel->manager->get_stats)
> +		channel->manager->get_stats(channel, &track->io_start);
>  }

If you're going to use a caller allocates paradigm, then the caller
would be responsible for doing this:

      track->io_start.num_fields = 2;

... so the library routine knows how much of the structure it is safe
for it to fill in.  Alternatively, it might be easier to simply have
the io_manager pass back a pointer to its own stats structure, and
then the caller would use the num_fields_size to figure out how much
of the structure it can trust.

						- Ted