Fix concurrency bug when writing non-aligned data.

Currently, clients attempting to concurrently write two (non-overlapping) buffers may end-up overwriting each others changes if one of the buffers is non-aligned. This commit fixes the issue by always locking objects for mutation.

It also removes an accidental double-fetching of the object during read-modify-write.

See also: https://listman.redhat.com/archives/libguestfs/2022-May/028937.html

@ebblake What do you think? See also thread on the mailing list.

I read the mailing list thread first, but agree that we have a potentially nasty data corruption bug in our current locking mechanism in the blocksize filter. I'm less familiar with the S3 plugin to know if it is suffering from the same problem, but it looks like we need to consider some form of rwlock solution (where we allow parallel aligned operations under the rdlock, but exclusive unaligned operations under the wrlock, regardless of whether the operation is pread or pwrite, and where we then need to worry about starvation issues if we favor readers or favor writers).

The situation in the S3 plugin is a lot worse, because we do not even lock within partial writes (which the blocksize filter does). But the same solution should be applicable in both cases.

Friendly ping on the status of this merge request (that fixes a data corruption bug)

There are maybe 3 or 4 other merge requests outstanding that depend on this one, but are much simpler. IMHO it'd be better if those were changed so they are standalone, and they can go upstream easily, then we can spend more time considering this one.

I am a bit confused. The current form of the plugin causes data corruption. Not in theory, but in practice. When used with the Kernel's NBD client this happens within minutes of using the filesystem. The patch adds the missing locking procedures. Can you provide some more details on what you would like to consider in more depth?

Do you think there may not be an issue that needs fixing? Or that the patch does not fix the issue? Or that the patch has some other unintended effects?

It's a complicated patch that is going to take some time to understand. In the meantime there are simpler patches that are waiting and could go upstream first.

This really needs a comment about what you're calculating here. I'm assuming it's the block number + offset of the first byte of the request, and the block number + offset of the last byte + 1 of the request. Why last byte + 1?

done.

We've previously done all of this in a more straightforward way, by considering the unaligned head, aligned main, and unaligned tail of requests. An example would be this code in the cache filter. It's simpler, although it uses a single lock rather than per-block locks.

Yes, but I think it's not sufficiently simpler to justify the loss of parallelism. Do you see this differently?

It would get rid of the MultiLock class (so less code), but if we consider this an abstraction that we can assume to work, then in the actual plugin code it just replaces with obj_lock(key) with with global_lock. I do not think this is a big win.

All tests pass in my local build.

added 21 commits

• c54b4d1d...b2612d2c - 20 commits from branch nbdkit:master
• 04fcadd6 - Fix concurrency bug when writing non-aligned data.

Compare with previous version

added 13 commits

• 04fcadd6...83f1b300 - 12 commits from branch nbdkit:master
• f1ec9409 - Fix concurrency bug when writing non-aligned data.

Compare with previous version

Thanks, this is upstream as commit 97739efe.

closed