On Thu, May 27, 2010 at 07:50:41AM +0100, Al Viro wrote:
> On Tue, May 25, 2010 at 04:49:12PM +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@xxxxxxxxx>
> >
> > The 'sync_supers' thread wakes up every 5 seconds (by default) and
> > writes back all super blocks. It keeps waking up even if there
> > are no dirty super-blocks. For many file-systems the superblock
> > becomes dirty very rarely, if ever, so 'sync_supers' does not do
> > anything most of the time.
> >
> > This patch improves 'sync_supers' and makes sleep if all superblocks
> > are clean and there is nothing to do. This helps saving the power.
> > This optimization is important for small battery-powered devices.
>
> > +void mark_sb_dirty(struct super_block *sb)
> > +{
> > + sb->s_dirty = 1;
> > +
> > + spin_lock(&supers_timer_lock);
> > + if (!supers_timer_armed) {
> > + bdi_arm_supers_timer();
> > + supers_timer_armed = 1;
> > + } else if (supers_timer_armed == -1)
> > + supers_timer_armed = 1;
> > + spin_unlock(&supers_timer_lock);
> > +}
> > +EXPORT_SYMBOL(mark_sb_dirty);
>
> Ouch... That turns a previously trivial operation into something
> much heavier; moreover, I'd rather see serious review of s_dirt
> uses.
Yeah, we definitely don't want to add global cacheline writes in the
common case. Also I don't know why you do the strange -1 value. I
couldn't seem to find where you defined bdi_arm_supers_timer();
But why doesn't this work?
sb->s_dirty = 1;
smp_mb(); /* corresponding MB is in test_and_clear_bit */
if (unlikely(!supers_timer_armed)) {
if (!test_and_set_bit(0, &supers_timer_armed))
bdi_arm_supers_timer();
}
vs
supers_timer_armed = 0;
again:
sync_supers();
if (test_and_clear_bit(0, &supers_timer_armed))
goto again;
--
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
