On Fri, Oct 30, 2020 at 10:58 AM Andres Freund <and...@anarazel.de> wrote: > > Hi, > > On 2020-10-30 10:01:08 +0800, Julien Rouhaud wrote: > > On Fri, Oct 30, 2020 at 2:17 AM Andres Freund <and...@anarazel.de> wrote: > > > The code does IO while holding the buffer mapping lock. That seems > > > *entirely* unacceptable to me. That basically locks 1/128 of shared > > > buffers against concurrent mapping changes, while reading data that is > > > likely not to be on disk? Seriously? > > > > The initial implementation had a different approach, reading the buffer once > > without holding the buffer mapping lock (which could lead to some false > > positive in some unlikely scenario), and only if a corruption is detected > > the > > read is done once again *while holding the buffer mapping lock* to ensure > > it's > > not a false positive. Some benchmarking showed that the performance was > > worse, > > so we dropped that optimisation. Should we go back to something like that > > or > > do you have a better way to ensure a consistent read of a buffer which > > isn't in > > shared buffers? > > I suspect that you're gonna need something quite different than what the > function is doing right now. Not because such a method will be faster in > isolation, but because there's a chance to have it correct and not have > a significant performance impact onto the rest of the system. > > I've not thought about it in detail yet. Is suspect you'll need to > ensure there is a valid entry in the buffer mapping table for the buffer > you're processing. By virtue of setting BM_IO_IN_PROGRESS on that entry > you're going to prevent concurrent IO from starting until your part is > done.
So I'm assuming that the previous optimization to avoid almost every time doing an IO while holding a buffer mapping lock isn't an option? In that case, I don't see any other option than reverting the patch and discussing a new approach.
