Re: Writeback tests

[Christoph Hellwig <hch@infradead.org>]

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.