Re: [PATCH 2/2 v2] writeback: Add writeback stats for pages written

[Curt Wohlgemuth <curtw@google.com>]

Hi Jan:

On Mon, Aug 15, 2011 at 8:03 AM, Jan Kara <jack@suse.cz> wrote:
> On Fri 12-08-11 15:47:25, Curt Wohlgemuth wrote:
>> Add a new file, /proc/writeback/stats, which displays
>> machine global data for how many pages were cleaned for
>> which reasons.  It also displays some additional counts for
>> various writeback events.
>>
>> These data are also available for each BDI, in
>> /sys/block/<device>/bdi/writeback_stats .
>  I think /sys/kernel/debug/bdi/<device>/writeback_stats might be a better
> place since we don't really want to make a stable interface out of this,
> do we?

Okay, I was waiting for someone to request this, I'll change it.

>> Sample output:
>>
>>    page: balance_dirty_pages           2561544
>>    page: background_writeout              5153
>>    page: try_to_free_pages                   0
>>    page: sync                                0
>>    page: kupdate                        102723
>>    page: fdatawrite                    1228779
>>    page: laptop_periodic                     0
>>    page: free_more_memory                    0
>>    page: fs_free_space                       0
>  The above stats are probably useful. I'm not so convinced about the stats
> below - it looks like it should be simple enough to get them by enabling
> some trace points and processing output (or if we are missing some
> tracepoints, it would be worthwhile to add them).

For these specifically, I'd agree with you.  In general, though, I
think that having generally available aggregated stats is really
useful, in a different way than tracepoints are.

>
>>    periodic writeback                      377
>>    single inode wait                         0
>>    writeback_wb wait                         1
>>
>> Signed-off-by: Curt Wohlgemuth <curtw@google.com>
> ...
>> +static size_t writeback_stats_to_str(struct writeback_stats *stats,
>> +                                 char *buf, size_t len)
>> +{
>> +     int bufsize = len - 1;
>> +     int i, printed = 0;
>> +     for (i = 0; i < WB_STAT_MAX; i++) {
>> +             const char *label = wb_stats_labels[i];
>> +             if (label == NULL)
>> +                     continue;
>> +             printed += snprintf(buf + printed, bufsize - printed,
>> +                             "%-32s %10llu\n", label, stats->stats[i]);
>  Cast stats->stats[i] to unsigned long long explicitely since it doesn't
> have to be u64...

Thanks.

>> +             if (printed >= bufsize) {
>> +                     buf[len - 1] = '\n';
>> +                     return len;
>> +             }
>> +     }
>> +
>> +     buf[printed - 1] = '\n';
>> +     return printed;
>> +}
>> +
>> +static int writeback_seq_show(struct seq_file *m, void *data)
>> +{
>> +     char *buf;
>> +     size_t size;
>> +     switch ((enum writeback_op)m->private) {
>> +     case WB_STATS_OP:
>  What's the point of WB_STATS_OP?

It's a vestige of the many more files under /proc/writeback/ that we
have in our kernels (see my response to Fengguang's email) -- and so
processing each file is done via a different WB_xxx_OP.  I forgot to
simplify this in the patch I sent out; will fix this.

>
>> +             size = seq_get_buf(m, &buf);
>> +             if (size == 0)
>> +                     return 0;
>> +             size = writeback_stats_print(writeback_sys_stats, buf, size);
>> +             seq_commit(m, size);
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int writeback_open(struct inode *inode, struct file *file)
>> +{
>> +     return single_open(file, writeback_seq_show, PDE(inode)->data);
>> +}
>> +
>> +static const struct file_operations writeback_ops = {
>> +     .open           = writeback_open,
>> +     .read           = seq_read,
>> +     .llseek         = seq_lseek,
>> +     .release        = single_release,
>> +};
>> +
>> +
>> +void __init proc_writeback_init(void)
>> +{
>> +     struct proc_dir_entry *base_dir;
>> +     base_dir = proc_mkdir("writeback", NULL);
>> +     if (base_dir == NULL) {
>> +             printk(KERN_ERR "Creating /proc/writeback/ failed");
>> +             return;
>> +     }
>> +
>> +     writeback_sys_stats = alloc_percpu(struct writeback_stats);
>> +
>> +     proc_create_data("stats", S_IRUGO|S_IWUSR, base_dir,
>  Can user really write to the file?

No to this file, I'll fix, thanks.  (Yes to some of our
/proc/writeback/ files, to clear them.)

Thanks,
Curt

>
>> +                     &writeback_ops, (void *)WB_STATS_OP);
>> +}
>
>                                                                Honza
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>