On Wed, Dec 06, 2017 at 12:36:48PM +1100, Dave Chinner wrote: > > - 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)?
If we need more nodes in the tree, xas_store() will try to allocate them with GFP_NOWAIT | __GFP_NOWARN. If that fails, it signals it in xas_error(). xas_nomem() will notice that we're in an ENOMEM situation, and allocate a node using your preferred GFP flags (NOIO in your case). Then we retry, guaranteeing forward progress. [1] The other conversions use the normal API instead of the advanced API, so all of this gets hidden away. For example, the inode cache does this: + curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS); and xa_cmpxchg internally does: do { xa_lock_irqsave(xa, flags); curr = xas_create(&xas); if (curr == old) xas_store(&xas, entry); xa_unlock_irqrestore(xa, flags); } while (xas_nomem(&xas, gfp)); > 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... It's the design pattern I've always intended to use. Naturally, the xfs radix trees weren't my initial target; it was the page cache, and the page cache does the same thing; uses the tree_lock to protect both the radix tree and several other fields in that same data structure. I'm open to argument on this though ... particularly if you have a better design pattern in mind! [1] I actually have this documented! It's in the xas_nomem() kernel-doc: * If we need to add new nodes to the XArray, we try to allocate memory * with GFP_NOWAIT while holding the lock, which will usually succeed. * If it fails, @xas is flagged as needing memory to continue. The caller * should drop the lock and call xas_nomem(). If xas_nomem() succeeds, * the caller should retry the operation. * * Forward progress is guaranteed as one node is allocated here and * stored in the xa_state where it will be found by xas_alloc(). More * nodes will likely be found in the slab allocator, but we do not tie * them up here. * * Return: true if memory was needed, and was successfully allocated. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html