From mboxrd@z Thu Jan 1 00:00:00 1970 From: Artem Bityutskiy Subject: Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count Date: Thu, 27 May 2010 19:04:35 +0300 Message-ID: <1274976275.15516.79.camel@localhost> References: <1274795352-3551-1-git-send-email-dedekind1@gmail.com> <1274795352-3551-18-git-send-email-dedekind1@gmail.com> <20100527065041.GA31073@ZenIV.linux.org.uk> <20100527072240.GM22536@laptop> <1274957469.13159.20.camel@localhost> <20100527120737.GN22536@laptop> <1274973693.15516.67.camel@localhost> <20100527154435.GS22536@laptop> Reply-To: dedekind1@gmail.com Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , LKML , Jens Axboe , linux-fsdevel@vger.kernel.org To: Nick Piggin Return-path: In-Reply-To: <20100527154435.GS22536@laptop> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Fri, 2010-05-28 at 01:44 +1000, Nick Piggin wrote: > > I think this will change the behavior of 'sync_supers()' too much. = ATM, > > it makes only one SB pass, then sleeps, then another one, then slee= ps. > > And we should probably preserve this behavior. So I'd rather make i= t: > >=20 > > if (supers_dirty) > > bdi_arm_supers_timer(); > > set_current_state(TASK_INTERRUPTIBLE); > > schedule(); > >=20 > > This way we will keep the behavior closer to the original. >=20 > Well your previous code had the same issue (ie. it could loop again > in sync_supers). But fair point perhaps. I think no, it either armed the timer of went to sleep, but it does not matter much :-) > But we cannot do the above, because again the timer might go off > before we set current state. We'd lose the wakeup and never wake > up again. >=20 > Putting it inside set_current_state() should be OK. I suppose. Oh, right! > > There is spin_lock(&sb_lock) in sync_supers(), so strictly speak th= is > > 'smp_mb()' is not needed if we move supers_dirty =3D 0 into > > 'sync_supers()' and add a comment that a mb is required, in case so= me > > one modifies the code later? > >=20 > > Or this is not worth it? >=20 > It's a bit tricky. spin_lock only gives an acquire barrier, which > prevents CPU executing instructions inside the critical section > before acquiring the lock. It actually allows stores to be deferred > from becoming visible to other CPUs until inside the critical section= =2E > So the load of sb->s_dirty could indeed still happen before the > store is seen. >=20 > Locks do allow you to avoid thinking about barriers, but *only* when > all memory accesses to all shared variables are inside the locks > (or when a section has just a single access, which by definition don'= t > need ordering with another access). Oh, ok. I need to read carefully Documentation/memory-barriers.txt. > > BTW, do you want me to keep you to be the patch author, add your > > signed-off-by and my original commit message? >=20 > I'm not concerned. You contributed more to the idea+implementation, > so record yourself as author. Ok, but thank you a lot for helping! --=20 Best Regards, Artem Bityutskiy (=D0=90=D1=80=D1=82=D1=91=D0=BC =D0=91=D0=B8=D1=82=D1=8E= =D1=86=D0=BA=D0=B8=D0=B9)