Hi Jan:
On Mon, Aug 29, 2011 at 9:36 AM, Jan Kara <jack@xxxxxxx> wrote:
> 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?
I'm open to suggestions...
> ...
>> 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?
Sigh. Yes, I do like whitespace, but not usually this much; I'll fix them all.
>> /*
>> * 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.
Ah, thanks for catching the omitted reason. I assume you mean the
table above, and
+#define show_work_reason(reason)
from the patch 2/3 (in the trace events file). Hmm, that could be a
challenge, given the limitations on what you can do in trace macros.
I'll think on this though.
Thanks,
Curt
>
>> @@ -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@xxxxxxx>
> SUSE Labs, CR
>
--
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
