From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: Writeback tests Date: Wed, 10 Aug 2011 12:54:46 -0400 Message-ID: <20110810165446.GA9521@infradead.org> References: <20110715153324.GB2276@infradead.org> <20110715234423.GA10296@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Christoph Hellwig , Wu Fengguang , Jan Kara , Andrew Morton , Dave Chinner , Michael Rubin , linux-fsdevel@vger.kernel.org To: Curt Wohlgemuth Return-path: Received: from 173-166-109-252-newengland.hfc.comcastbusiness.net ([173.166.109.252]:55030 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753598Ab1HJQzA (ORCPT ); Wed, 10 Aug 2011 12:55:00 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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.