On Wed, Apr 06, 2022 at 08:01:21AM +1000, Dave Chinner wrote: > 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. Ok, I'll see if I can split things up a bit better. > > 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. I actually had that earlier as well and really like the flow of the single function. So it certainly is doable.
