From mboxrd@z Thu Jan  1 00:00:00 1970
From: Artem Bityutskiy <dedekind1@gmail.com>
Subject: Re: [RFC][PATCH 16/16] writeback: prevent unnecessary bdi threads
 wakeups
Date: Tue, 20 Jul 2010 16:13:14 +0300
Message-ID: <1279631594.16462.139.camel@localhost>
References: <1279284312-2411-1-git-send-email-dedekind1@gmail.com>
	 <1279284312-2411-17-git-send-email-dedekind1@gmail.com>
	 <20100718074536.GA1191@infradead.org>
Reply-To: dedekind1@gmail.com
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Jens Axboe <axboe@kernel.dk>, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
To: Christoph Hellwig <hch@infradead.org>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20100718074536.GA1191@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: linux-fsdevel.vger.kernel.org

On Sun, 2010-07-18 at 03:45 -0400, Christoph Hellwig wrote:
> > +		if (wb_has_dirty_io(wb) && dirty_writeback_interval) {
> > +			unsigned long wait;
> > =20
> > -			wait_jiffies =3D msecs_to_jiffies(dirty_writeback_interval * 10=
);
> > -			schedule_timeout(wait_jiffies);
> > +			wait =3D msecs_to_jiffies(dirty_writeback_interval * 10);
> > +			schedule_timeout(wait);
>=20
> No need for a local variable.  If you want to shorten things a bit a
> schedule_timeout_msecs helper in generic code would be nice, as there
> are lots of patterns like this in various kernel threads.

OK, do you want me to ignore the 80-lines limitation or you want me
to add schedule_timeout_msecs() as a part of this patch series?

> >  void __mark_inode_dirty(struct inode *inode, int flags)
> >  {
> > +	bool wakeup_bdi;
> >  	struct super_block *sb =3D inode->i_sb;
> > +	struct backing_dev_info *uninitialized_var(bdi);
>=20
> Just initialize wakeup_bdi and bdi here - a smart compiler will defer
> them until we need them, and it makes the code a lot easier to read, =
as
> well as getting rid of the uninitialized_var hack.

OK.

> > +			/*
> > +			 * If this is the first dirty inode for this bdi, we
> > +			 * have to wake-up the corresponding bdi thread to make
> > +			 * sure background write-back happens later.
> > +			 */
> > +			if (!wb_has_dirty_io(&bdi->wb) &&
> > +			    bdi_cap_writeback_dirty(bdi))
> > +				wakeup_bdi =3D true;
>=20
> How about redoing this as:
>=20
> 			if (bdi_cap_writeback_dirty(bdi)) {
> 				WARN(!test_bit(BDI_registered, &bdi->state),
> 				     "bdi-%s not registered\n", bdi->name);
>=20
> 				/*
> 				 * If this is the first dirty inode for this
> 				 * bdi, we have to wake-up the corresponding
> 				 * flusher thread to make sure background
> 				 * writeback happens later.
> 				 */
> 				if (!wb_has_dirty_io(&bdi->wb))
> 					wakeup_bdi =3D true;
> 			}

OK.

> > +	if (wakeup_bdi) {
> > +		bool wakeup_default =3D false;
> > +
> > +		spin_lock(&bdi->wb_lock);
> > +		if (unlikely(!bdi->wb.task))
> > +			wakeup_default =3D true;
> > +		else
> > +			wake_up_process(bdi->wb.task);
> > +		spin_unlock(&bdi->wb_lock);
> > +
> > +		if (wakeup_default)
> > +			wake_up_process(default_backing_dev_info.wb.task);
>=20
> Same comment about just keeping wb_lock over the
> default_backing_dev_info wakup as for one of the earlier patches appl=
ies
> here.

I just figured that I have to add 'trace_writeback_nothread(bdi, work)'
here, just like in 'bdi_queue_work()'. I'd feel safer to call tracer
outside the spinlock. What do you think?

> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -326,7 +326,7 @@ static unsigned long bdi_longest_inactive(void)
> >  	unsigned long interval;
> > =20
> >  	interval =3D msecs_to_jiffies(dirty_writeback_interval * 10);
> > -	return max(5UL * 60 * HZ, wait_jiffies);
> > +	return max(5UL * 60 * HZ, interval);
>=20
> So previously we just ignored interval here?=20

Yes, my fault, thanks for catching.

--=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)