svn commit: r193375 - head/sys/ufs/ufs
Pawel Jakub Dawidek
pjd at FreeBSD.org
Sat Jun 6 21:22:36 UTC 2009
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
> > >
> > > 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. [...]
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 = 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.
>
> 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.
--
Pawel Jakub Dawidek http://www.wheel.pl
pjd at FreeBSD.org http://www.FreeBSD.org
FreeBSD committer Am I Evil? Yes, I Am!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20090606/600833f3/attachment-0001.pgp
More information about the svn-src-all
mailing list