Re: [PATCH 1/2] writeback: Add a 'reason' to wb_writeback_work

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jan:

On Fri, Aug 12, 2011 at 1:43 PM, Jan Kara <jack@xxxxxxx> wrote:
> On Fri 12-08-11 11:45:06, Curt Wohlgemuth wrote:
>> This creates a new 'reason' field in a wb_writeback_work
>> structure, which unambiguously identifies who initiates
>> writeback activity.  A 'wb_stats' enumeration has been added
>> to writeback.h, to enumerate the possible reasons.
>>
>> The 'writeback_work_class' tracepoint event class is updated
>> to include the symbolic 'reason' in all trace events.
>>
>> The 'writeback_queue_io' tracepoint now takes a work object,
>> in order to print out the 'reason' for queue_io.
>>
>> And the 'writeback_inodes_sbXXX' family of routines has had
>> a wb_stats parameter added to them, so callers can specify
>> why writeback is being started.
>>
>> Signed-off-by: Curt Wohlgemuth <curtw@xxxxxxxxxx>
>  The patch looks good. Just two minor comments below. So you can
> add:
>  Acked-by: Jan Kara <jack@xxxxxxx>
>
>> @@ -647,11 +651,12 @@ long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
>>               .nr_pages       = nr_pages,
>>               .sync_mode      = WB_SYNC_NONE,
>>               .range_cyclic   = 1,
>> +             .reason         = WB_STAT_BALANCE_DIRTY,
>>       };
>>
>>       spin_lock(&wb->list_lock);
>>       if (list_empty(&wb->b_io))
>> -             queue_io(wb, NULL);
>> +             queue_io(wb, &work);
>>       __writeback_inodes_wb(wb, &work);
>>       spin_unlock(&wb->list_lock);
>>
>  Umm, for consistency it would make more sense for writeback_inodes_wb()
> to take reason argument as well. Also strictly speaking, this function has
> two callers - balance_dirty_pages() and bdi_forker_thread()...

Yeah, good idea.

>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> index d196074..53c995e 100644
>> --- a/mm/page-writeback.c
>> +++ b/mm/page-writeback.c
>> @@ -737,8 +737,9 @@ static void balance_dirty_pages(struct address_space *mapping,
>>                */
>>               trace_balance_dirty_start(bdi);
>>               if (bdi_nr_reclaimable > task_bdi_thresh) {
>> -                     pages_written += writeback_inodes_wb(&bdi->wb,
>> -                                                          write_chunk);
>> +                     long wrote;
>> +                     wrote = writeback_inodes_wb(&bdi->wb, write_chunk);
>> +                     pages_written += wrote;
>  What is this hunk for?

Oops.  This should have been in the "PATCH 2/2" , not this one.  v2 of
these patches are in the mail.

Thanks,
Curt

>
>                                                        Honza
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]