Arvados

This doesn't appear to be related to r/o disks, I have seen it happen on tb05z as well, where all disks can be written.

While it was happening there, datamanager was trying to do its thing, and failing (), and restarting, over and over and over (due to #5834).

a memory use bug¶

Currently, keepstore will allocate as many buffers as it has clients. There is no limit on the number of concurrent clients.

• During a PUT request, the buffer grows (up to 64 mebibytes) while receiving data from the client, and of course has to last long enough to write the block to disk. When -serialize is enabled, this interval includes the "wait for serialize mutex" phase, which can last many seconds.
• During a successful GET request, the buffer grows (up to 64 mebibytes) while reading data from disk and lasts long enough to verify the checksum and send the data to the client. Sending the data over the network can take many seconds (e.g., network congestion, slow client, slow network on the client side of a keepproxy).

If keepstore gets busy enough to exceed the available disk bandwidth, the PUT requests waiting for the serialize mutex can exhaust available RAM and the kernel will start swapping some buffers out to disk. At this point, each additional request will make all queued requests slower and less likely to finish at all.

Additionally, if a client times out and disconnects while its request is waiting in the queue, keepstore won't notice this: abandoned PUT requests waste memory just by staying in the queue, and (assuming the worst) get swapped in and written to a volume before the dead connection is noticed; meanwhile abandoned GET requests waste disk activity and (albeit for a short time, between reading and the first Write call, which presumably fails) memory.

proposed improvement¶

Rather than allocating them on the fly, use a pool¹ of data buffers. The total number of buffers is configured with a command line flag. Whenever a handler needs a non-trivial buffer to store a data block -- for either PUT or GET -- it gets one from the pool. If the pool is empty, the request waits in a queue to get one.

This should be a reasonable way to limit keepstore's memory without unduly restricting operations that don't need much of it (like index, status, and GET 404).

¹ probably based on http://golang.org/pkg/sync/#Pool -- maybe WaitingPool, wrapping Pool with Get() and Put() methods that use a sync.Cond to ensure size never goes above MaxSize.

Reviewing 4d53c7e. This is good, just a couple cleanliness things.

• There are a couple of references to old Index name still lurking around: in the last sentence of the IndexTo doc comment, and in gotCall() in the MockVolume implementation.
• In IndexTo, the comment "Print filenames beginning with prefix" seems misplaced right above a condition that skips this loop. Would it improve readability to merge that condition into the previous one? Or if not, move the comment down above the Fprintf?

Assuming those are all patched straightforwardly, I'm happy to see this merged. Thanks.

Reviewing 5748-max-buffers 978afd4

• The default maxBuffers = 128 seems excessive (that's 8 GiB). With serialize=true, it should probably be a small multiple of the number of volumes (between 2 and 4 buffers per volume) since we can't do more concurrent work than we have volumes, and the goal here is to prevent keepstore from over committing on the number of requests it can handle at once (since it Gets the block from the buffer pool before taking the volume serialize lock).

• Suggest moving bufferpool into the Go SDK, it might be useful for other services such as keepproxy.

Peter Amstutz wrote:

Reviewing 5748-max-buffers 978afd4

• The default maxBuffers = 128 seems excessive (that's 8 GiB). With serialize=true, it should probably be a small multiple of the number of volumes (between 2 and 4 buffers per volume) since we can't do more concurrent work than we have volumes, and the goal here is to prevent keepstore from over committing on the number of requests it can handle at once (since it Gets the block from the buffer pool before taking the volume serialize lock).

Not exactly. The goal here is to limit memory use, and memory (unlike the serialize lock) stays occupied longer when clients send/receive slowly. If you have 8 GiB of RAM, and you are waiting for 100 clients to receive the response data for their GET requests, you've got 6.4GiB of block data stored in buffers but the serialize locks are all free. Similarly, while you are waiting to receive PUT body content from 100 clients, you need buffers but you don't need any serialize locks yet. Essentially the minimum buffers:spindles ratio corresponds to the diskspeed:networkspeed ratio, and (especially since keepproxy pipelines reads instead of buffering them) that ratio can easily be much higher than 2 or 4. (If we could assume/detect that the network bottleneck is the keepstore node itself, it would be reasonable to stall new clients to help older clients finish, but we can't/don't.)

I thought "total system RAM ÷ block size" would be a good default, and a way to specify "50%" like sort(1) (perhaps based on https://godoc.org/github.com/cloudfoundry/gosigar#Mem.Get) would probably be handy, but rather than overthink/predict this I figured we should start with a simple config knob and a probably-not-too-low default, and get operational feedback.

I figured it would be better to err on the "too big" side because the sysadmin story "keepstore is using too much ram" seems easier to understand than "clients are starved but keepstore looks idle". (Hm, perhaps the "waiting..." message should recommend increasing -max-buffers?)

• Suggest moving bufferpool into the Go SDK, it might be useful for other services such as keepproxy.

True. I thought about this too, but figured we can do those things together (move it to SDK and adopt it in keepproxy) / we might confirm how well it works before we arrange too much more code around it...

Tom Clegg wrote:

I figured it would be better to err on the "too big" side because the sysadmin story "keepstore is using too much ram" seems easier to understand than "clients are starved but keepstore looks idle". (Hm, perhaps the "waiting..." message should recommend increasing -max-buffers?)

That's true, it's easy enough to configure the usage, the important thing will be to communicate to our admins that setting -max-buffers is not really optional, if it's not going to be smart about adjusting its default buffers to the RAM size of the system.

• Suggest moving bufferpool into the Go SDK, it might be useful for other services such as keepproxy.

True. I thought about this too, but figured we can do those things together (move it to SDK and adopt it in keepproxy) / we might confirm how well it works before we arrange too much more code around it...

Ok, we should file a separate story for that, I think there have been some issues with memory usage of Keepproxy as well.

Peter Amstutz wrote:

That's true, it's easy enough to configure the usage, the important thing will be to communicate to our admins that setting -max-buffers is not really optional, if it's not going to be smart about adjusting its default buffers to the RAM size of the system.

True. Added a note to http://doc.arvados.org/install/install-keepstore.html. (Probably a good idea to point this out even if/when we do default to total system RAM, in case the node is expected to have memory available for anything else.)

Ok, we should file a separate story for that, I think there have been some issues with memory usage of Keepproxy as well.

Added #5962.