lkml.org 
[lkml]   [2011]   [Aug]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 13/13] dcache: convert to use new lru list infrastructure
    > +	struct list_head *freeable = arg;
    > + struct dentry *dentry = container_of(item, struct dentry, d_lru);
    > +
    > +
    > + /*

    double empty line.

    > + * we are inverting the lru lock/dentry->d_lock here,
    > + * so use a trylock. If we fail to get the lock, just skip
    > + * it
    > + */
    > + if (!spin_trylock(&dentry->d_lock))
    > + return 2;
    > +
    > + /*
    > + * Referenced dentries are still in use. If they have active
    > + * counts, just remove them from the LRU. Otherwise give them
    > + * another pass through the LRU.
    > + */
    > + if (dentry->d_count) {
    > + list_del_init(&dentry->d_lru);
    > + spin_unlock(&dentry->d_lock);
    > + return 0;
    > + }
    > +
    > + if (dentry->d_flags & DCACHE_REFERENCED) {

    The comment aove seems odd, given that it doesn't match the code.
    I'd rather have something like:

    /*
    * Used dentry, remove it from the LRU.
    */

    in its place, and a second one above the DCACHE_REFERENCED check:

    /*
    * Referenced dentry, give it another pass through the LRU.
    */

    > + dentry->d_flags &= ~DCACHE_REFERENCED;
    > + spin_unlock(&dentry->d_lock);
    > +
    > + /*
    > + * XXX: this list move should be be done under d_lock. Need to
    > + * determine if it is safe just to do it under the lru lock.
    > + */
    > + return 1;
    > + }
    > +
    > + list_move_tail(&dentry->d_lru, freeable);

    Another odd comment. It talks about doing a list_move in the branch
    that doesn't do the list_move, and the list_move outside the branch
    actually has the d_lock, thus disagreeing with the comment.

    > + this_cpu_dec(nr_dentry_unused);
    > + spin_unlock(&dentry->d_lock);

    No need to decrement the per-cpu counter while still having the lock
    held.

    > @@ -1094,11 +1069,10 @@ resume:
    > /*
    > * move only zero ref count dentries to the dispose list.
    > */
    > + dentry_lru_del(dentry);
    > if (!dentry->d_count) {
    > - dentry_lru_move_list(dentry, dispose);
    > + list_add_tail(&dentry->d_lru, dispose);
    > found++;
    > - } else {
    > - dentry_lru_del(dentry);

    I'd rather move this hunk to the previous patch, as it fits into the
    logical change done there.



    \
     
     \ /
      Last update: 2011-08-24 08:35    [W:0.025 / U:29.688 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site