From owner-svn-src-head@FreeBSD.ORG Sat Jun 6 18:14:10 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 22FC51065674; Sat, 6 Jun 2009 18:14:10 +0000 (UTC) (envelope-from snb@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 0ECBB8FC2F; Sat, 6 Jun 2009 18:14:10 +0000 (UTC) (envelope-from snb@freebsd.org) Received: from ebi.local (root@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id n56IE7Sm086261; Sat, 6 Jun 2009 18:14:07 GMT (envelope-from snb@freebsd.org) Date: Sat, 6 Jun 2009 20:14:07 +0200 From: Nick Barkas To: Pawel Jakub Dawidek Message-ID: <20090606181405.GA2928@ebi.local> References: <200906030944.n539iM2K045164@svn.freebsd.org> <20090603210652.GD3821@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090603210652.GD3821@garage.freebsd.pl> User-Agent: Mutt/1.5.19 (2009-01-05) 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 18:14:10 -0000 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 > > > > 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 increase to > > vfs.ufs.dirhash_maxmem on machines that have lots of memory, without > > degrading performance by having too much memory reserved for dirhash when > > other things need it. The default value for dirhash_maxmem is being kept at > > 2MB for now, though. > > > > This work was mostly done during the 2008 Google Summer of Code. > > > > 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() > > +{ > [...] > > + /* > > + * Delete dirhashes not used for more than ufs_dirhashreclaimage > > + * seconds. If we can't get a lock on the dirhash, it will be skipped. > > + */ > > + for (dh = TAILQ_FIRST(&ufsdirhash_list); dh != NULL; dh = > > + TAILQ_NEXT(dh, dh_list)) { > > + if (!sx_try_xlock(&dh->dh_lock)) > > + continue; > > + if (time_second - dh->dh_lastused > ufs_dirhashreclaimage) > > + memfreed += ufsdirhash_destroy(dh); > > + /* Unlock if we didn't delete the dirhash */ > > + else > > + ufsdirhash_release(dh); > > + } > > + > > + /* > > + * If not enough memory was freed, keep deleting hashes from the head > > + * of the dirhash list. The ones closest to the head should be the > > + * oldest. > > + */ > > + for (dh = TAILQ_FIRST(&ufsdirhash_list); memfreed < memwanted && > > + dh !=NULL; dh = TAILQ_NEXT(dh, dh_list)) { > > + if (!sx_try_xlock(&dh->dh_lock)) > > + continue; > > + memfreed += ufsdirhash_destroy(dh); > > + } > > + DIRHASHLIST_UNLOCK(); > > +} > > I don't see how that works. If you remove dh from the tailq in > ufsdirhash_destroy(), you can't do 'dh = TAILQ_NEXT(dh, dh_list)' at the > end of the loop. > > You should use TAILQ_FOREACH_SAFE(3). In the second case you also need to > move this extra check into the loop, probably. 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. > 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)? 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. But, I don't really understand why the lock is dropped within ufsdirhash_destroy(), anyway. Perhaps it is not necessary to do so. 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. Nick