search

ceph / ceph-rust

Rust-lang interface to Ceph.
Apache License 2.0
98 stars 43 forks source link

Add Send Sync Impl for IoCtx #74

Closed mzhong1 closed 3 years ago

mzhong1 commented 3 years ago

Add unsafe impl Sync Send for IoCtx so they can be passed through threads

cholcombe973 commented 3 years ago

Interesting. Does the underlying C library support this?

mzhong1 commented 3 years ago

Interesting. Does the underlying C library support this?

through librados? I'm not actually sure. I've been trying to figure out an issue with a librbd call (using IoCtx) that keeps hanging forever (and I've tried implementing timeout/retry, hence the need for Send Sync). There are a few functions to flush the ioctx (both syncronously and asyncronously). Not that its particularly safe nor am I particularly comfortable actually pushing this since well, its an unsafe impl of Send/Sync. It does work for the method I'm trying to use this for though.

cholcombe973 commented 3 years ago

Honestly I'd ask the Ceph developers if the IOCtx is thread safe. If it is then this PR is fine. I think I left those off because I was never totally sure if it was or wasn't thread safe. You can probably send an email to the ceph developers list and get an answer pretty quickly.

mzhong1 commented 3 years ago

Honestly I'd ask the Ceph developers if the IOCtx is thread safe. If it is then this PR is fine. I think I left those off because I was never totally sure if it was or wasn't thread safe. You can probably send an email to the ceph developers list and get an answer pretty quickly.

Hey Chris!
I went to the ceph-devel IRC and asked, as it turns out, it is in fact thread-safe!

"[13:53] Does anyone happen to know if the IoCtx in librados is thread-safe?
[13:56] == dustinm[~dustinm@static.38.6.217.95.clients.your-server.de] has quit [] [13:58] mzhong1: it is"

cholcombe973 commented 3 years ago

That's great! Thanks for following up

cholcombe973 commented 3 years ago

I'm not sure if this should be a minor or a major change. The behavior changes but the API doesn't

mzhong1 commented 3 years ago

I'm not sure if this should be a minor or a major change. The behavior changes but the API doesn't

Er, mostly it makes rados calls and librbd calls (and other calls which depend on IoCtx) thread-safe, but its not really changing the API? Its just adding a thread-safety feature to calls? Would that count as major or minor?

cholcombe973 commented 3 years ago

I think i'll bump the minor version for now. We can always pull it from crates.io and make it a major if it's a problem

mzhong1 commented 3 years ago

I think i'll bump the minor version for now. We can always pull it from crates.io and make it a major if it's a problem

Thanks! I'll update the ceph-rbd crate to use this new version once its out

cholcombe973 commented 3 years ago

I can land the PR change right now but the CI tests are failing because of issues with the install packaging.

mzhong1 commented 3 years ago

I can land the PR change right now but the CI tests are failing because of issues with the install packaging.

Is there anything I need to do? For fixing/etc?

cholcombe973 commented 3 years ago

You can try to make a PR to fix the CI if you're feeling up to it. I pushed a change but it still appears slightly broken

cholcombe973 commented 3 years ago

I pushed a new version to crates.io in the mean time so you're not blocked.

mzhong1 commented 3 years ago

I pushed a new version to crates.io in the mean time so you're not blocked.

Moop, thanks! I'll take a look at the CI, I think it might be something outdated packages or something

cholcombe973 commented 3 years ago

Yeah I updated the packages. It seems to build/check and then something kills the job. I'm not sure what is wrong

On Mon, Apr 5, 2021 at 12:04 PM mzhong1 @.***> wrote:

I pushed a new version to crates.io in the mean time so you're not blocked.

Moop, thanks! I'll take a look at the CI, I think it might be something outdated packages or something

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ceph/ceph-rust/pull/74#issuecomment-813580451, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXKUE5J4VQBG6IGDXEG4GDTHICTTANCNFSM42JJZOOQ .