Prev: staging: hv: Fix race condition on IC channel initialization (modified)
Next: [PATCH] power_end event (Resend)
From: Al Viro on 27 May 2010 03:00
On Tue, May 25, 2010 at 04:49:12PM +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy(a)nokia.com>
>
> 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.

Note, e.g., that in your series you've touched udf; it can set s_dirt
until the cows come home, but without ->write_super() it'll be ignored
by everything in VFS and fs/udf itself never looks at the damn thing.

A look around it shows fs/sysv, where we never clean the damn flag anymore
for r/w mounts. Yes, really (got broken a year ago, nobody noticed).

Or, e.g., BFS - there we have ->write_super() mark the buffer_head that
contains on-disk sb dirty, and the only place that sets ->s_dirt is doing
that immediately after having marked the same bh dirty itself. Interesting
place, at that - bfs_fill_super() at r/w mount time... Note that ->sync_fs()
there does *not* wait for anything, which is not the right thing to do.

IOW, this thing is a good topic for code review; I suspect that quite a few
users might be gone as the result.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 27 May 2010 03:30
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(a)nokia.com>
> >
> > 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-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Al Viro on 27 May 2010 05:10
On Thu, May 27, 2010 at 05:22:40PM +1000, Nick Piggin wrote:
> 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();

bdi_arm_supers_timer() is in the mainline; basically, it's "schedule
timer that'll wake sync_supers_tsk in $INTERVAL from now".
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 27 May 2010 06:30
On Thu, 2010-05-27 at 07:50 +0100, Al Viro wrote:
> On Tue, May 25, 2010 at 04:49:12PM +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy(a)nokia.com>
> > +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.

OK, I'll try to do something lighter with atomic variables or something
like Nick posted - need to think about this. And I'll try to review
s_dirty usage as much as my time and knowledge allow. Thanks.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 27 May 2010 07:00
Nick, thanks for serialization suggestion.

On Thu, 2010-05-27 at 17:22 +1000, Nick Piggin wrote:
> 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();

It is in mm/backing-dev.c:376 in today's Linus' tree. The -1 is used to
indicate that 'sync_supers()' is in progress and avoid arming timer in
that case. But yes, this is not really needed.

> But why doesn't this work?
>
> sb->s_dirty = 1;
> smp_mb(); /* corresponding MB is in test_and_clear_bit */

AFAIU, test_and_clear_bit assumes 2 barriers - before the test and after
the clear. Then I do not really understand why this smp_mb is needed.

> 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;

AFAIU, the following should be fine, no?:


if (unlikely(!supers_timer_armed))
if (!test_and_set_bit(0, &supers_timer_armed))
bdi_arm_supers_timer();

vs

again:
sync_supers();
if (test_and_clear_bit(0, &supers_timer_armed))
goto again;

I assume that it is harmless to run 'bdi_arm_supers_timer()'
concurrently;

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3 4
Prev: staging: hv: Fix race condition on IC channel initialization (modified)
Next: [PATCH] power_end event (Resend)