{"action":"created","issue":{"url":"https://api.github.com/repos/ceph/ceph/issues/23126","repository_url":"https://api.github.com/repos/ceph/ceph","labels_url":"https://api.github.com/repos/ceph/ceph/issues/23126/labels{/name}","comments_url":"https://api.github.com/repos/ceph/ceph/issues/23126/comments","events_url":"https://api.github.com/repos/ceph/ceph/issues/23126/events","html_url":"https://github.com/ceph/ceph/pull/23126","id":342680049,"node_id":"MDExOlB1bGxSZXF1ZXN0MjAyNTIxNDU2","number":23126,"title":"[WIP] config: drop md_comfig_impl::lock when invoking config observer","user":{"login":"vshankar","id":105507,"node_id":"MDQ6VXNlcjEwNTUwNw==","avatar_url":"https://avatars2.githubusercontent.com/u/105507?v=4","gravatar_id":"","url":"https://api.github.com/users/vshankar","html_url":"https://github.com/vshankar","followers_url":"https://api.github.com/users/vshankar/followers","following_url":"https://api.github.com/users/vshankar/following{/other_user}","gists_url":"https://api.github.com/users/vshankar/gists{/gist_id}","starred_url":"https://api.github.com/users/vshankar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vshankar/subscriptions","organizations_url":"https://api.github.com/users/vshankar/orgs","repos_url":"https://api.github.com/users/vshankar/repos","events_url":"https://api.github.com/users/vshankar/events{/privacy}","received_events_url":"https://api.github.com/users/vshankar/received_events","type":"User","site_admin":false},"labels":[{"id":144329561,"node_id":"MDU6TGFiZWwxNDQzMjk1NjE=","url":"https://api.github.com/repos/ceph/ceph/labels/core","name":"core","color":"0041a2","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-07-19T10:57:46Z","updated_at":"2018-07-20T11:41:57Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/ceph/ceph/pulls/23126","html_url":"https://github.com/ceph/ceph/pull/23126","diff_url":"https://github.com/ceph/ceph/pull/23126.diff","patch_url":"https://github.com/ceph/ceph/pull/23126.patch"},"body":"This is WIP -- sending out for early reviews and discussions. For details of the issue see: http://tracker.ceph.com/issues/24823 (basically locking order deadlock in the MDS on `config set` via admin socket during active IO).\r\n\r\nThis PR invokes observer callback without holding `md_config_impl::lock` (before falling back in doing it this way I tried to pass the lock reference (`std::unique_lock`*) to the observer callback so as to acquire locks in the MDS in a sane order, but I'm not sure if that would be the preferred way).\r\n\r\nI'm not sure what else would break due to this (`config.h` mentions that the lock needs to be held when calling config observers).\r\n\r\n(*): commit a0a4c483bc1 mentions about this\r\n\r\nSigned-off-by: Venky Shankar <vshankar@redhat.com>"},"comment":{"url":"https://api.github.com/repos/ceph/ceph/issues/comments/406576153","html_url":"https://github.com/ceph/ceph/pull/23126#issuecomment-406576153","issue_url":"https://api.github.com/repos/ceph/ceph/issues/23126","id":406576153,"node_id":"MDEyOklzc3VlQ29tbWVudDQwNjU3NjE1Mw==","user":{"login":"vshankar","id":105507,"node_id":"MDQ6VXNlcjEwNTUwNw==","avatar_url":"https://avatars2.githubusercontent.com/u/105507?v=4","gravatar_id":"","url":"https://api.github.com/users/vshankar","html_url":"https://github.com/vshankar","followers_url":"https://api.github.com/users/vshankar/followers","following_url":"https://api.github.com/users/vshankar/following{/other_user}","gists_url":"https://api.github.com/users/vshankar/gists{/gist_id}","starred_url":"https://api.github.com/users/vshankar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vshankar/subscriptions","organizations_url":"https://api.github.com/users/vshankar/orgs","repos_url":"https://api.github.com/users/vshankar/repos","events_url":"https://api.github.com/users/vshankar/events{/privacy}","received_events_url":"https://api.github.com/users/vshankar/received_events","type":"User","site_admin":false},"created_at":"2018-07-20T11:41:57Z","updated_at":"2018-07-20T11:41:57Z","author_association":"CONTRIBUTOR","body":"> This adds a subtle race in that you might update options a and b, drop then lock, the someone else updates a only, drops the lock, and the second thread calls the observer first. I think this doesn't really matter since the observer will read the most recent config value--it just means that observers shouldn't read anything into the grouping of options that they are passed.\r\n> \r\n> The more concerning change is that observers are reentrant with this code. That might cause problems.\r\n\r\nYeh -- with this change observer implementations should make no assumptions that they would be invoked serially. Any such implementations would need to fixed with this change.\r\n\r\n> \r\n> And the final issue is that an observer might deregister itself and then delete itself, but race with a notification, which would lead to a use-after-free. I think this one has to be fixed... probably by keeping a counter of theads calling observers and on unregister, blocking until those flush. That means unregister callers have to be careful not to hold locks that their observer implementations might require. I suspect this leads to the same category of deadlock that the original bug does, except that lockdep probably won't see it? For example, the observer might\r\n\r\nWould making unregister (`remove_observer()`) complete a Context callback when all outstanding observer calls are finished be an alternate approach? The context callback would need to be invoked in a way such that it can acquire a clean lock context (async via worker thread). Or will it be too much to have all this in config and resort to blocking instead?\r\n\r\n> \r\n> take foo.lock\r\n> take config.lock\r\n> (This is what triggered a lockdep error before, since we were holding config.lock and calling into the foo observer). Unregister would need to be careful not to hold foo.lock while unregistering or else the other thread will deadlock in the same way.. even though holding foo.lock and calling into unregister (and taking config.lock) is the normal, safe ordering. But the multiple threads still makes it deadlock because it would block waiting for the observer to complete.\r\n> \r\n> Am I getting that right?\r\n\r\nThanks! All these pointers really help.\r\n\r\n"}}
{"actor":{"display_login":"vshankar"}}