More iobuf cross-shard checks, better doc #23263
Conversation
iobuf is a tricky class which is unsafe to use across threads even in cases which would be safe with almost all other classes, due to hidden sharing of the underlying buffers. Make this restriction clearer in the class commends and in the documentation for share() and copy(). Ref CORE-7061.
We do a check (in debug mode) in many mutating methods that we are on the origin shard of the iobuf. Move this check into its own method to reduce verbosity and to allow a split between mutating and non-mutating methods.
Add the mutating method check to more mutating methods. The only check that this does, that the "origin shard" is the same as the current shard, also should apply to const methods, but for now I'll add it only to mutating methods which are somehow "even worse" to call when internal sharing is present. Ref CORE-7061.
|
This is still missing the one line where we add the same-shard check to I don't think this precludes any of the stuff that @ballard26 was thinking about doing in terms of making internal sharing safe rather than unsafe-and-sometimes-checked-in-debug: if that goes in we just need to update the doc & checks, but since this is a real footgun it seems like we can do this conservative check now (and even if we make internal sharing safe this will catch cases where the same iobuf is shared across threads which wouldn't be allowed even if we made internal sharing safe). |
travisdowns
changed the title
|
new failures in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4ad-4e40-9286-c37678c13515: new failures in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4b0-447b-a8d5-9f492e43ca22: new failures in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4ab-44ed-862d-e86b6e63d109: new failures in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4b2-464e-8345-35a6e8f13361: new failures in https://buildkite.com/redpanda/redpanda/builds/54336#0191e23c-59bd-47ec-9df9-174d8a6e5372: new failures in https://buildkite.com/redpanda/redpanda/builds/54337#0191e267-7b33-42b1-a8a0-94c65b52e5cb: new failures in https://buildkite.com/redpanda/redpanda/builds/54348#0191e2be-ea10-4287-a00f-0898528f1796: |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4ad-4e40-9286-c37678c13515 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4b0-447b-a8d5-9f492e43ca22 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54271#0191ddcb-a4b2-464e-8345-35a6e8f13361 |
| * shard from the source and so on). | ||
| * | ||
| * The only safe way to get the contents of an iobuf from one shard to another | ||
| * is to pass the iobuf to the other shard and then call copy() on it, which is |
There was a problem hiding this comment.
This doesn't seem like a safe interface to me?
It means the submit_to caller always has to wait until the submit_to future resolves to do anything with its iobuf and hope the submit_to side is not keeping the original iobuf around somehow? It seems as unsafe to me as the more idiomatic way of just doing copy/share first and then moving the result to the other shard. It further pessimizes the cases where no copy is needed and the iobuf is simply EOL on the source shard.
I think the oncore stuff is just broken and doesn't really work as is. I tried to extend it in the past as well.
This really needs language support, maybe there is some thread_local hackery that might work?
There was a problem hiding this comment.
Good feedback, thanks.
It means the submit_to caller always has to wait until the submit_to future resolves to do anything with its iobuf and hope the submit_to side is not keeping the original iobuf around somehow?
Right, though waiting until the future resolves is almost always the existing use case for submit_to and related calls?
That said, I wrote this as a codification of the oncore existing restrictions of the class, and the existing behavior of the share/copy/move ctor methods: the thing that creates the new iobuf (here, copy() needs to happen on the target shard since it grabs its origin shard ID implicitly from the current shard.
As far as I can tell, iobuf does not currently allow any reasonable way to move an iobuf from one shard to another: the origin ID is preserved on move, so you can never get a moved to iobuf on another shard with the right origin ID. It may only happen to work in some cases because the verify check wasn't uniformly added everywhere so if you avoid the methods where it does exist you at least won't get an assert (but you may crash).
I don't think it's a slam dunk though that copy()-on-source is definitely better at least if we have any concept of origin shard: consider the case where you do an invoke_on (all shards), you really want to create N copies (at east with the current restriction), one for each shard. In that case copy() on the target just works: makes the right number of copies of the iobuf with right shard ID.
It further pessimizes the cases where no copy is needed and the iobuf is simply EOL on the source shard.
Yes, but as above this is "already the case" and don't think there's any way to move safely currently.
I think the oncore stuff is just broken and doesn't really work as is.
Well the restriction is "too strict", but we already have it so I'm trying to lean on that for safety at the cost of re-enforcing the existing pessimization. So it is at least partly workable in the sense that it has been like this since iobuf introduction.
That said, totally open to better approaches here too, either for the doc or the implementation. Just relying on 100% adherence to complex and mostly undocumented lifetime runs isn't going to cut it, I think: especially because the one of the main failure mode occurs due to a non-atomic increment race which is going to be very hard to pick up with testing and likely to slip through to production.
I tried to #14024 it in the past as well.
Ah, I wish I had seen that. Yeah it's the same vein. There's also more discussion in https://redpandadata.atlassian.net/browse/CORE-7061 if you hadn't seen it.
This really needs language support, maybe there is some thread_local hackery that might work?
Sure, we should be open to anything here. Maybe like @ballard26 said it's easier just to make the refcounting atomic, which would make the semantics more sane and hence easier to document :). That does look like a potentially invasive change performance-wise though since I think it means temporary_buffer needs use atomic refcounting everywhere in seastar (i.e., not opt-in e.g., via template parameter) since we buffers created and consumed in seastar to be like that from the start: plus the work of combing through all the deleter implementations to see if they are truly compatible with cross-shard use beyond the refcounting, since they can do arbitrary things at deletion time.
There was a problem hiding this comment.
As far as I can tell, iobuf does not currently allow any reasonable way to move an iobuf from one shard to another: the origin ID is preserved on move, so you can never get a moved to iobuf on another shard with the right origin ID. It may only happen to work in some cases because the verify check wasn't uniformly added everywhere so if you avoid the methods where it does exist you at least won't get an assert (but you may crash).
Yes, but as above this is "already the case" and don't think there's any way to move safely currently.
Right yes, I guess I should have clarified that I was thinking more about the release build usecase or with the oncore check removed as I think it doesn't make much sense in its current form as described otherwise. I do very much suspect it wasn't added to all methods because it would break some common current usage patterns that are fine from a thread safety perspective.
I don't think it's a slam dunk though that copy()-on-source is definitely better at least if we have any concept of origin shard: consider the case where you do an invoke_on (all shards), you really want to create N copies (at east with the current restriction), one for each shard. In that case copy() on the target just works: makes the right number of copies of the iobuf with right shard ID.
In that example, is a copy even needed at all? If it's safe to copy/read from N other threads in parallel then it would also be safe to just read directly without copying for the duration of the invoke_on_all call? It would require const == threadsafe, not sure whether that is given at least today.
said it's easier just to make the refcounting atomic ... potentially invasive change performance-wise though
I do think that is the best way forward. Back when I was looking into this because of the same bug/misuse in a different part of the code this was my conclusion as well. Note I think this could be a performance gain as it would mean that we can drop the extra copies on the produce and fetch path which are likely a lot more expensive.
For the checks, I am wondering whether we can add a method along the signature of:
iobuf&& move_to_shard(shard_id target_shard_id)
That invalidates the current object and creates a new one with the source shard id adapted. Then we could possibly add the checks in more places like share (outside of the constructors which are probably still needed implicitly as seastar might move the lambda under the hood). Haven't really thought this through.
There was a problem hiding this comment.
But no, move_to_shard is never really safe because at least in its current form the iobuf doesn't know whether it uniquely owns the temporary_buffer (in #13447 (comment) the issue was definitely that we could have already gotten a shared buffer out of ss::input_stream::read_up_to).
So I guess this really already pessimizes this case:
It further pessimizes the cases where no copy is needed and the iobuf is simply EOL on the source shard.
as it can never be correct unfortunately. And in that sense share with oncore check failing anywhere is a bug. As are the failures in this PR? Well maybe not if you can guarantee that the temporary_buf is not shared possibly because you constructed it yourself somewhere.
So possibly copy_to_shard(shard_id target_shard_id) might be a good interface but in practice just doing the copy in the submit_to callback is just easier then.
There was a problem hiding this comment.
Right yes, I guess I should have clarified that I was thinking more about the release build usecase or with the oncore check removed as I think it doesn't make much sense in its current form as described otherwise.
Well I feel like we shouldn't do be doing anything in the release build that would assert in the debug build, right? Both "in principle" but also actually it should be unlikely since we run (some subset of) our tests in debug and those don't assert. So I would say that the methods that have the oncore check are off-limits for cross-origin calls regardless of the build mode.
That aside, I basically agree with you that the oncore check seems like it's not really doing the right thing or at least pessimizes things unnecessarily. So I'll consider this on hold for now.
Edit: To add this was responding to your comment 2 up, I hadn't see the one immediately above so I'll have to consider that further.
There was a problem hiding this comment.
as it can never be correct unfortunately.
Right. Though some correct cases can be detected, conservatively, though it would require more tracking. E.g., every operation either potentially causes shared buffers to be added to the iobuf, or not. E.g., the constructors that just pass char*/size which is copied into the iobuf don't result in sharing, and something like the move-ctor should inherit the sharing state. clear() removes all sharing, etc. So you track this boolean state "not shared vs maybe shared" and only assert if it's in the maybe shared state or some other kind of logic. E.g., moving across shards is safe for not shared stuff.
As are the failures in this PR?
I did look at two cases and they both looked safe in the sense above: on one shard an iobuf is created "unshared" (e.g., as the result of parsing some json), then it is moved to another shard (produce flow in this case).
|
/ci-repeat 1 |
|
/ci-repeat 1 |
|
/ci-repeat 1 |
|
/ci-repeat 1 |
|
See also here for an upstream discussion on making |
|
This is obsoleted by redpanda-data/seastar#149. |
Increase the iobuf strictness in checking for "same shard" restrictions in debug mode.
Backports Required
Release Notes