From owner-svn-src-head@FreeBSD.ORG Sat Jun 6 21:22:36 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 30C251065687; Sat, 6 Jun 2009 21:22:36 +0000 (UTC) (envelope-from pjd@garage.freebsd.pl) Received: from mail.garage.freebsd.pl (chello087206192061.chello.pl [87.206.192.61]) by mx1.freebsd.org (Postfix) with ESMTP id 9E5668FC1C; Sat, 6 Jun 2009 21:22:35 +0000 (UTC) (envelope-from pjd@garage.freebsd.pl) Received: by mail.garage.freebsd.pl (Postfix, from userid 65534) id 284F445685; Sat, 6 Jun 2009 23:22:33 +0200 (CEST) Received: from localhost (chello087206192061.chello.pl [87.206.192.61]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.garage.freebsd.pl (Postfix) with ESMTP id 37F024569A; Sat, 6 Jun 2009 23:22:27 +0200 (CEST) Date: Sat, 6 Jun 2009 23:22:30 +0200 From: Pawel Jakub Dawidek To: Nick Barkas Message-ID: <20090606212229.GD2313@garage.freebsd.pl> References: <200906030944.n539iM2K045164@svn.freebsd.org> <20090603210652.GD3821@garage.freebsd.pl> <20090606181405.GA2928@ebi.local> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="d01dLTUuW90fS44H" Content-Disposition: inline In-Reply-To: <20090606181405.GA2928@ebi.local> User-Agent: Mutt/1.4.2.3i X-PGP-Key-URL: http://people.freebsd.org/~pjd/pjd.asc X-OS: FreeBSD 8.0-CURRENT i386 X-Spam-Checker-Version: SpamAssassin 3.0.4 (2005-06-05) on mail.garage.freebsd.pl X-Spam-Level: X-Spam-Status: No, score=-0.6 required=4.5 tests=BAYES_00,RCVD_IN_SORBS_DUL autolearn=no version=3.0.4 Cc: svn-src-head@freebsd.org, dwmalone@maths.tcd.ie, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r193375 - head/sys/ufs/ufs X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Jun 2009 21:22:36 -0000 --d01dLTUuW90fS44H Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jun 06, 2009 at 08:14:07PM +0200, Nick Barkas wrote: > On Wed, Jun 03, 2009 at 11:06:52PM +0200, Pawel Jakub Dawidek wrote: > > On Wed, Jun 03, 2009 at 09:44:22AM +0000, Sean Nicholas Barkas wrote: > > > Author: snb > > > Date: Wed Jun 3 09:44:22 2009 > > > New Revision: 193375 > > > URL: http://svn.freebsd.org/changeset/base/193375 > > >=20 > > > Log: > > > Add vm_lowmem event handler for dirhash. This will cause dirhashes = to be > > > deleted when the system is low on memory. This ought to allow an in= crease to > > > vfs.ufs.dirhash_maxmem on machines that have lots of memory, without > > > degrading performance by having too much memory reserved for dirhas= h when > > > other things need it. The default value for dirhash_maxmem is being= kept at > > > 2MB for now, though. > > > =20 > > > This work was mostly done during the 2008 Google Summer of Code. > > > =20 > > > Approved by: dwmalone (mentor), re > > > MFC after: 3 months > > [...] > > > +static int > > > +ufsdirhash_destroy(struct dirhash *dh) > > > +{ > > [...] > > > + /* Remove it from the list and detach its memory. */ > > > + TAILQ_REMOVE(&ufsdirhash_list, dh, dh_list); > > [...] > > > +static void > > > +ufsdirhash_lowmem() > > > +{ > > [...] > > > + /*=20 > > > + * Delete dirhashes not used for more than ufs_dirhashreclaimage=20 > > > + * seconds. If we can't get a lock on the dirhash, it will be skipp= ed. > > > + */ > > > + for (dh =3D TAILQ_FIRST(&ufsdirhash_list); dh !=3D NULL; dh =3D=20 > > > + TAILQ_NEXT(dh, dh_list)) { > > > + if (!sx_try_xlock(&dh->dh_lock)) > > > + continue; > > > + if (time_second - dh->dh_lastused > ufs_dirhashreclaimage) > > > + memfreed +=3D ufsdirhash_destroy(dh); > > > + /* Unlock if we didn't delete the dirhash */ > > > + else > > > + ufsdirhash_release(dh); > > > + } > > > + > > > + /*=20 > > > + * If not enough memory was freed, keep deleting hashes from the he= ad=20 > > > + * of the dirhash list. The ones closest to the head should be the= =20 > > > + * oldest.=20 > > > + */ > > > + for (dh =3D TAILQ_FIRST(&ufsdirhash_list); memfreed < memwanted && > > > + dh !=3DNULL; dh =3D TAILQ_NEXT(dh, dh_list)) { > > > + if (!sx_try_xlock(&dh->dh_lock)) > > > + continue; > > > + memfreed +=3D ufsdirhash_destroy(dh); > > > + } > > > + DIRHASHLIST_UNLOCK(); > > > +} > >=20 > > I don't see how that works. If you remove dh from the tailq in > > ufsdirhash_destroy(), you can't do 'dh =3D TAILQ_NEXT(dh, dh_list)' at = the > > end of the loop. > >=20 > > You should use TAILQ_FOREACH_SAFE(3). In the second case you also need = to > > move this extra check into the loop, probably. >=20 > Yes, I think you are right. Thanks for catching that. I think that, as > written, both loops will only try to delete the first hash they can lock > in ufsdirhash_list. I'll try to correct that.=20 >=20 > > In addition you drop DIRHASHLIST lock in ufsdirhash_destroy() during the > > loop. Can't the tailq be modified from elsewhere? Or even from parallel > > call to ufsdirhash_lowmem() (I don't think we serialize those)? >=20 > The lock is held on the tailq while we are doing TAILQ_REMOVE(). It is > only released for freeing data structures contained in dirhashes that > have been removed from the tailq. I don't see how this would be a > problem, but I certainly could be missing something. [...] The ufsdirhash_destroy() function does this: TAILQ_REMOVE(&ufsdirhash_list, dh, dh_list); Once you return from ufsdirhash_destroy(), dh is no longer on the queue, so how this is suppose to work: dh =3D TAILQ_NEXT(dh, dh_list)? TAILQ_NEXT() should panic on you when called for element which is not on the queue, but we don't have such strict checks compiled-in by default. Try to define QUEUE_MACRO_DEBUG somewhere in sys/queue.h to get the debug. All in all, if you traverse the queue and want to remove current element from inside the loop, TAILQ_FOREACH_SAFE() is for you. > [...] But, I don't really > understand why the lock is dropped within ufsdirhash_destroy(), anyway. > Perhaps it is not necessary to do so.=20 >=20 > I mostly just copied the code needed out of ufsdirhash_recycle() to > write ufsdirhash_destroy(). ufsdirhash_recycle() I think could be > concurrently called (by ufsdirhash_build()) previously, with the lock on > ufsdirhash_list lock dropped in the same place, and I don't think this > caused any problems. --=20 Pawel Jakub Dawidek http://www.wheel.pl pjd@FreeBSD.org http://www.FreeBSD.org FreeBSD committer Am I Evil? Yes, I Am! --d01dLTUuW90fS44H Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4 (FreeBSD) iD8DBQFKKt4VForvXbEpPzQRAjvPAJwIhUY1dVZSYbHPmadaG7JXtp5qdACePeeS Zj1fgRN4QPs0K0jGmqbcMRQ= =RG7Z -----END PGP SIGNATURE----- --d01dLTUuW90fS44H--