Looks like this mostly got lost in the noise. Can you resend it
with a proper subject to linux-mm and fsdevel outside of this threads?
A few comments that could be addressed below:
> @@ -39,6 +39,7 @@ struct wb_writeback_work {
> unsigned int for_kupdate:1;
> unsigned int range_cyclic:1;
> unsigned int for_background:1;
> + int why;
Needs an explanation what it really is. Also maybe reason is a better
name for the variable?
> @@ -554,6 +562,9 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
> */
> redirty_tail(inode);
> }
> + if (inode->i_ino == 0)
> + bdi_writeback_stat_add(wb->bdi,
> + WB_STAT_METADATA_PAGES_CLEANED, wrote);
inode->i_ino doesn't nessecarily imply it's metdata. A filesystem
might simply not use the vfs inode number, or use a too large data
type that gets truncated. But even when you check for inodes on
the block device filesystem people still can use those for data I/O.
> @@ -724,6 +737,12 @@ static long wb_writeback(struct bdi_writeback *wb,
> writeback_inodes_wb(wb, &wbc);
> trace_wbc_writeback_written(&wbc, wb->bdi);
>
> + if (work->why < WB_STAT_PG_COUNT_BASE) {
> + bdi_writeback_stat_add(wb->bdi,
> + work->why + WB_STAT_PG_COUNT_BASE,
> + write_chunk - wbc.nr_to_write);
> + }
> +
Can you explain the WB_STAT_PG_COUNT_BASE magic a bit better? Maybe
hiding it in a helper would be useful, which would also get the
comments.
> work->nr_pages -= write_chunk - wbc.nr_to_write;
> wrote += write_chunk - wbc.nr_to_write;
Given how often we do this calculation it would be good to pull it into
a local variable.
> @@ -1192,6 +1217,7 @@ void writeback_inodes_sb_nr(struct super_block *sb, unsigned long nr)
> .sync_mode = WB_SYNC_NONE,
> .done = &done,
> .nr_pages = nr,
> + .why = WB_STAT_SYNC, /* XXX: Not always correct */
> };
>
> WARN_ON(!rwsem_is_locked(&sb->s_umount));
> @@ -1270,6 +1296,7 @@ void sync_inodes_sb(struct super_block *sb)
> .nr_pages = LONG_MAX,
> .range_cyclic = 0,
> .done = &done,
> + .why = WB_STAT_SYNC,
> };
Indeed. Either you want to pass the argument from the caller, or
use writeback_inodes_sb/sync_inodes_sb as the stat name and thus make
it even more fine-grained.
> diff --git a/fs/proc/proc_writeback.c b/fs/proc/proc_writeback.c
> new file mode 100644
> index 0000000..4614697
> --- /dev/null
> +++ b/fs/proc/proc_writeback.c
I think the details of the stats file shouldn't be in fs/proc/
but in mm/ or fs/ where they are accumulated.
Last but not least you really should pass the why/reason argument to the
VM tracepoints. Maybe just passing the reason down and adding it to
the tracepoints could be patch 1/2, with the second one beeing the
actual statistics counters.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
