Re: [PATCH 3/3 v3] writeback: Add writeback stats for pages written

[Jan Kara <jack@suse.cz>]

On Mon 22-08-11 11:38:47, Curt Wohlgemuth wrote:
> Add a new file, /proc/writeback, which displays
> machine global data for how many pages were cleaned for
> which reasons.
  I'm not sure about the placement in /proc/writeback - maybe I'd be
happier if it was somewhere under /sys/kernel/debug but I don't really have
a better suggestion and I don't care that much either. Maybe Christoph or
Andrew have some idea?

...
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index bdda069..5168ac9 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -59,6 +59,7 @@ enum wb_reason {
>  	WB_REASON_TRY_TO_FREE_PAGES,
>  	WB_REASON_SYNC,
>  	WB_REASON_PERIODIC,
> +	WB_REASON_FDATAWRITE,
>  	WB_REASON_LAPTOP_TIMER,
>  	WB_REASON_FREE_MORE_MEM,
>  	WB_REASON_FS_FREE_SPACE,
> @@ -67,6 +68,7 @@ enum wb_reason {
>  	WB_REASON_MAX,
>  };
>  
> +
  The additional empty line doesn't make much sense here?

>  /*
>   * A control structure which tells the writeback code what to do.  These are
>   * always on the stack, and hence need no locking.  They are always initialised
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 474bcfe..6613391 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
...
> @@ -56,9 +60,77 @@ void bdi_lock_two(struct bdi_writeback *wb1, struct bdi_writeback *wb2)
>  	}
>  }
>  
> +
  And another empty line here?

> +static const char *wb_stats_labels[WB_REASON_MAX] = {
> +	[WB_REASON_BALANCE_DIRTY] = "page: balance_dirty_pages",
> +	[WB_REASON_BACKGROUND] = "page: background_writeout",
> +	[WB_REASON_TRY_TO_FREE_PAGES] = "page: try_to_free_pages",
> +	[WB_REASON_SYNC] = "page: sync",
> +	[WB_REASON_PERIODIC] = "page: periodic",
> +	[WB_REASON_FDATAWRITE] = "page: fdatawrite",
> +	[WB_REASON_LAPTOP_TIMER] = "page: laptop_periodic",
> +	[WB_REASON_FREE_MORE_MEM] = "page: free_more_memory",
> +	[WB_REASON_FS_FREE_SPACE] = "page: fs_free_space",
> +};
 I don't think it's good to have two enum->string translation tables for
reasons. That's prone to errors which is in fact proven by the fact that
you ommitted FORKER_THREAD reason here.
  
> @@ -157,6 +248,7 @@ static inline void bdi_debug_unregister(struct backing_dev_info *bdi)
>  }
>  #endif
>  
> +
  Another empty line here? You seem to like them ;)

>  static ssize_t read_ahead_kb_store(struct device *dev,
>  				  struct device_attribute *attr,
>  				  const char *buf, size_t count)

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR