diff options
author | Daniel Stodden <daniel.stodden@citrix.com> | 2010-04-30 18:01:15 -0400 |
---|---|---|
committer | Jens Axboe <jaxboe@fusionio.com> | 2010-08-07 12:31:34 -0400 |
commit | 5b61cb90c2ad8c853b4dd53eec200bacd2f02172 (patch) | |
tree | d518fde8701ce437ac87ee36732401fc115e9d7b /drivers/xen | |
parent | 2def141e71d54eccac98dc2c2ba71a82c91b324e (diff) |
xenbus: Make xenbus_switch_state transactional
According to the comments, this was how it's been done years ago, but
apparently took an xbt pointer from elsewhere back then. The code was
removed because of consistency issues: cancellation wont't roll back
the saved xbdev->state.
Still, unsolicited writes to the state field remain an issue,
especially if device shutdown takes thread synchronization, and subtle
races cause accidental recreation of the device node.
Fixed by reintroducing the transaction. An internal one is sufficient,
so the xbdev->state value remains consistent.
Also fixes the original hack to prevent infinite recursion. Instead of
bailing out on the first attempt to switch to Closing, checks call
depth now.
Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Diffstat (limited to 'drivers/xen')
-rw-r--r-- | drivers/xen/xenbus/xenbus_client.c | 90 |
1 files changed, 66 insertions, 24 deletions
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c index 7b3e973a1aee..7e49527189b6 100644 --- a/drivers/xen/xenbus/xenbus_client.c +++ b/drivers/xen/xenbus/xenbus_client.c | |||
@@ -133,17 +133,12 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, | |||
133 | } | 133 | } |
134 | EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); | 134 | EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); |
135 | 135 | ||
136 | static void xenbus_switch_fatal(struct xenbus_device *, int, int, | ||
137 | const char *, ...); | ||
136 | 138 | ||
137 | /** | 139 | static int |
138 | * xenbus_switch_state | 140 | __xenbus_switch_state(struct xenbus_device *dev, |
139 | * @dev: xenbus device | 141 | enum xenbus_state state, int depth) |
140 | * @state: new state | ||
141 | * | ||
142 | * Advertise in the store a change of the given driver to the given new_state. | ||
143 | * Return 0 on success, or -errno on error. On error, the device will switch | ||
144 | * to XenbusStateClosing, and the error will be saved in the store. | ||
145 | */ | ||
146 | int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) | ||
147 | { | 142 | { |
148 | /* We check whether the state is currently set to the given value, and | 143 | /* We check whether the state is currently set to the given value, and |
149 | if not, then the state is set. We don't want to unconditionally | 144 | if not, then the state is set. We don't want to unconditionally |
@@ -152,35 +147,65 @@ int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) | |||
152 | to it, as the device will be tearing down, and we don't want to | 147 | to it, as the device will be tearing down, and we don't want to |
153 | resurrect that directory. | 148 | resurrect that directory. |
154 | 149 | ||
155 | Note that, because of this cached value of our state, this function | 150 | Note that, because of this cached value of our state, this |
156 | will not work inside a Xenstore transaction (something it was | 151 | function will not take a caller's Xenstore transaction |
157 | trying to in the past) because dev->state would not get reset if | 152 | (something it was trying to in the past) because dev->state |
158 | the transaction was aborted. | 153 | would not get reset if the transaction was aborted. |
159 | |||
160 | */ | 154 | */ |
161 | 155 | ||
156 | struct xenbus_transaction xbt; | ||
162 | int current_state; | 157 | int current_state; |
163 | int err; | 158 | int err, abort; |
164 | 159 | ||
165 | if (state == dev->state) | 160 | if (state == dev->state) |
166 | return 0; | 161 | return 0; |
167 | 162 | ||
168 | err = xenbus_scanf(XBT_NIL, dev->nodename, "state", "%d", | 163 | again: |
169 | ¤t_state); | 164 | abort = 1; |
170 | if (err != 1) | 165 | |
166 | err = xenbus_transaction_start(&xbt); | ||
167 | if (err) { | ||
168 | xenbus_switch_fatal(dev, depth, err, "starting transaction"); | ||
171 | return 0; | 169 | return 0; |
170 | } | ||
171 | |||
172 | err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); | ||
173 | if (err != 1) | ||
174 | goto abort; | ||
172 | 175 | ||
173 | err = xenbus_printf(XBT_NIL, dev->nodename, "state", "%d", state); | 176 | err = xenbus_printf(xbt, dev->nodename, "state", "%d", state); |
174 | if (err) { | 177 | if (err) { |
175 | if (state != XenbusStateClosing) /* Avoid looping */ | 178 | xenbus_switch_fatal(dev, depth, err, "writing new state"); |
176 | xenbus_dev_fatal(dev, err, "writing new state"); | 179 | goto abort; |
177 | return err; | ||
178 | } | 180 | } |
179 | 181 | ||
180 | dev->state = state; | 182 | abort = 0; |
183 | abort: | ||
184 | err = xenbus_transaction_end(xbt, abort); | ||
185 | if (err) { | ||
186 | if (err == -EAGAIN && !abort) | ||
187 | goto again; | ||
188 | xenbus_switch_fatal(dev, depth, err, "ending transaction"); | ||
189 | } else | ||
190 | dev->state = state; | ||
181 | 191 | ||
182 | return 0; | 192 | return 0; |
183 | } | 193 | } |
194 | |||
195 | /** | ||
196 | * xenbus_switch_state | ||
197 | * @dev: xenbus device | ||
198 | * @state: new state | ||
199 | * | ||
200 | * Advertise in the store a change of the given driver to the given new_state. | ||
201 | * Return 0 on success, or -errno on error. On error, the device will switch | ||
202 | * to XenbusStateClosing, and the error will be saved in the store. | ||
203 | */ | ||
204 | int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) | ||
205 | { | ||
206 | return __xenbus_switch_state(dev, state, 0); | ||
207 | } | ||
208 | |||
184 | EXPORT_SYMBOL_GPL(xenbus_switch_state); | 209 | EXPORT_SYMBOL_GPL(xenbus_switch_state); |
185 | 210 | ||
186 | int xenbus_frontend_closed(struct xenbus_device *dev) | 211 | int xenbus_frontend_closed(struct xenbus_device *dev) |
@@ -284,6 +309,23 @@ void xenbus_dev_fatal(struct xenbus_device *dev, int err, const char *fmt, ...) | |||
284 | EXPORT_SYMBOL_GPL(xenbus_dev_fatal); | 309 | EXPORT_SYMBOL_GPL(xenbus_dev_fatal); |
285 | 310 | ||
286 | /** | 311 | /** |
312 | * Equivalent to xenbus_dev_fatal(dev, err, fmt, args), but helps | ||
313 | * avoiding recursion within xenbus_switch_state. | ||
314 | */ | ||
315 | static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err, | ||
316 | const char *fmt, ...) | ||
317 | { | ||
318 | va_list ap; | ||
319 | |||
320 | va_start(ap, fmt); | ||
321 | xenbus_va_dev_error(dev, err, fmt, ap); | ||
322 | va_end(ap); | ||
323 | |||
324 | if (!depth) | ||
325 | __xenbus_switch_state(dev, XenbusStateClosing, 1); | ||
326 | } | ||
327 | |||
328 | /** | ||
287 | * xenbus_grant_ring | 329 | * xenbus_grant_ring |
288 | * @dev: xenbus device | 330 | * @dev: xenbus device |
289 | * @ring_mfn: mfn of ring to grant | 331 | * @ring_mfn: mfn of ring to grant |