lkml.org 
[lkml]   [2017]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 72/73] xfs: Convert mru cache to XArray
    On Tue, Dec 05, 2017 at 04:41:58PM -0800, Matthew Wilcox wrote:
    > From: Matthew Wilcox <mawilcox@microsoft.com>
    >
    > This eliminates a call to radix_tree_preload().

    .....

    > void
    > @@ -431,24 +424,24 @@ xfs_mru_cache_insert(
    > unsigned long key,
    > struct xfs_mru_cache_elem *elem)
    > {
    > + XA_STATE(xas, &mru->store, key);
    > int error;
    >
    > ASSERT(mru && mru->lists);
    > if (!mru || !mru->lists)
    > return -EINVAL;
    >
    > - if (radix_tree_preload(GFP_NOFS))
    > - return -ENOMEM;
    > -
    > INIT_LIST_HEAD(&elem->list_node);
    > elem->key = key;
    >
    > - spin_lock(&mru->lock);
    > - error = radix_tree_insert(&mru->store, key, elem);
    > - radix_tree_preload_end();
    > - if (!error)
    > - _xfs_mru_cache_list_insert(mru, elem);
    > - spin_unlock(&mru->lock);
    > + do {
    > + xas_lock(&xas);
    > + xas_store(&xas, elem);
    > + error = xas_error(&xas);
    > + if (!error)
    > + _xfs_mru_cache_list_insert(mru, elem);
    > + xas_unlock(&xas);
    > + } while (xas_nomem(&xas, GFP_NOFS));

    Ok, so why does this have a retry loop on ENOMEM despite the
    existing code handling that error? And why put such a loop in this
    code and not any of the other XFS code that used
    radix_tree_preload() and is arguably much more important to avoid
    ENOMEM on insert (e.g. the inode cache)?

    Also, I really don't like the pattern of using xa_lock()/xa_unlock()
    to protect access to an external structure. i.e. the mru->lock
    context is protecting multiple fields and operations in the MRU
    structure, not just the radix tree operations. Turning that around
    so that a larger XFS structure and algorithm is now protected by an
    opaque internal lock from generic storage structure the forms part
    of the larger structure seems like a bad design pattern to me...

    Cheers,

    Dave.
    --
    Dave Chinner
    david@fromorbit.com

    \
     
     \ /
      Last update: 2017-12-06 02:45    [W:2.271 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site