On Mon, Apr 04, 2022 at 09:04:52AM +1000, Dave Chinner wrote: > On Sun, Apr 03, 2022 at 02:01:17PM +0200, Christoph Hellwig wrote: > > xfs_buf_get_map has a bit of a strange structure where the xfs_buf_find > > helper is called twice before we actually insert a new buffer on a cache > > miss. Given that the rhashtable has an interface to insert a new entry > > and return the found one on a conflict we can easily get rid of the > > double lookup by using that. > > We can do that without completely rewriting this code. We could. And I had something similar earlier. But I actually thing the structure of the code after this patch makes much more sense. All the logic for the fast path buffer lookup is clearly layed out in one function, which then just calls a helper to perform the lookup. The new scheme also is slightly less code overall. Even more so once the lockless lookup comes into play which requires different locking and refcount increments. > The return cases of this function end up being a bit of a mess. We can return: > > - error = 0 and a locked buffer in *bpp > - error = -EEXIST and an unlocked buffer in *bpp > - error != 0 and a modified *bpp pointer > - error != 0 and an unmodified *bpp pointer The last two are the same - the *bpp pointer simply is not valid on a "real" error return. So the return really is a tristate, similar to many other places in xfs.
