Re: [PATCH 2/5] writeback: convert WB_WRITTEN/WB_DIRITED counters to bytes

[Josef Bacik <jbacik@fb.com>]

On 10/25/2016 03:03 PM, Tejun Heo wrote:
> Hello, Josef.
>
> On Tue, Oct 25, 2016 at 02:41:41PM -0400, Josef Bacik wrote:
>> These are counters that constantly go up in order to do bandwidth calculations.
>> It isn't important what the units are in, as long as they are consistent between
>> the two of them, so convert them to count bytes written/dirtied, and allow the
>> metadata accounting stuff to change the counters as well.
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>
> Acked-by: Tejun Heo <tj@kernel.org>
>
> A small nit below.
>
>> @@ -2547,12 +2547,16 @@ void account_page_redirty(struct page *page)
>>  	if (mapping && mapping_cap_account_dirty(mapping)) {
>>  		struct inode *inode = mapping->host;
>>  		struct bdi_writeback *wb;
>> +		unsigned long flags;
>>  		bool locked;
>>
>>  		wb = unlocked_inode_to_wb_begin(inode, &locked);
>>  		current->nr_dirtied--;
>> -		dec_node_page_state(page, NR_DIRTIED);
>> -		dec_wb_stat(wb, WB_DIRTIED);
>> +
>> +		local_irq_save(flags);
>> +		__dec_node_page_state(page, NR_DIRTIED);
>> +		__add_wb_stat(wb, WB_DIRTIED_BYTES, -(long)PAGE_SIZE);
>> +		local_irq_restore(flags);
>
> Hmmm... so, the explicit irq clustering is neutral or win as the code
> currently stands but AFAICS that's just because add_wb_stat() doesn't
> use the right percpu ops.  If we convert add_wb_stat() to use the
> matching percpu ops, the above change would be more expensive at least
> on x86.  Maybe just skip this part?
>

Yeah I can convert it over and then just not do the irq dance.  I'll fix that up 
for the next go around.  Thanks,

Josef