On Tue, Apr 05, 2022 at 05:00:27PM +0200, Christoph Hellwig wrote: > 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. Agreed, but you're making two distinct, significant modifications in the one patchset. One is changing the way we use a generic library functionality, the other is changing the entire structure of the lookup path. IOWs, I was not saying the end result was bad, I was (clumsily) trying to suggest that you should split these two modifications into separate patches because they are largely separate changes. Once I thought about it that way, and looking that them that way made me want to structure the code quite differently. > > 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. I think you missed the point I was making. I'm not complaining about whether it's a tristate return or not, it's the fact that it can return buffers in different states and the caller has to handle that inconsistency itself whilst still maintaining an efficient fast path. That's what makes the code difficult to follow - xfs_buf_insert() is the slow path, so all the complexity and twisted logic should be inside that function rather than directly impacting the fast path code. e.g. Most of the complexity goes away if we factor out the buffer trylock/locking code into a helper (like we have in the iomap code) and then have xfs_buf_insert() call it when it finds an existing buffer. Then the -EEXIST return value can go away, and xfs_buf_insert can return a locked buffer exactly the same as if it inserted a new buffer. Have the newly allocated buffer take a new perag reference, too, instead of stealing the caller's reference, and then all the differences between insert and -EEXIST cases go away. Then you can move all the conditional lookup failure cases into xfs_buf_insert(), too, and we end up with high level fast path code that is clear and concise: /* cache hits generally outnumber misses by at least 10:1 */ bp = xfs_buf_lookup_fast(pag, &cmap); if (likely(bp)) error = xfs_buf_lookup_lock(bp, flags); else error = xfs_buf_lookup_insert(pag, &cmap, flags, &bp); xfs_perag_put(pag); if (!error) *bpp = bp; return error; Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
