On Tue, Mar 15, 2022 at 03:30:22PM -0400, John Snow wrote: > On Tue, Mar 15, 2022 at 4:47 AM Stefan Hajnoczi <[email protected]> wrote: > > > > On Mon, Mar 14, 2022 at 03:09:35PM -0400, John Snow wrote: > > > On Mon, Mar 14, 2022 at 1:23 PM Stefan Hajnoczi <[email protected]> > > > wrote: > > > > > > > > On Tue, Feb 15, 2022 at 06:59:38PM +0800, Xie Yongji wrote: > > > > > This supports passing NULL ops to blk_set_dev_ops() > > > > > so that we can remove stale ops in some cases. > > > > > > > > > > Signed-off-by: Xie Yongji <[email protected]> > > > > > --- > > > > > block/block-backend.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/block/block-backend.c b/block/block-backend.c > > > > > index 4ff6b4d785..08dd0a3093 100644 > > > > > --- a/block/block-backend.c > > > > > +++ b/block/block-backend.c > > > > > @@ -1015,7 +1015,7 @@ void blk_set_dev_ops(BlockBackend *blk, const > > > > > BlockDevOps *ops, > > > > > blk->dev_opaque = opaque; > > > > > > > > > > /* Are we currently quiesced? Should we enforce this right now? > > > > > */ > > > > > - if (blk->quiesce_counter && ops->drained_begin) { > > > > > + if (blk->quiesce_counter && ops && ops->drained_begin) { > > > > > ops->drained_begin(opaque); > > > > > } > > > > > } > > > > > > > > John: You added this code in f4d9cc88ee6. Does blk_set_dev_ops() need to > > > > call ->drained_end() when ops is set to NULL? > > > > > > > > Stefan > > > > > > I'm not sure I trust my memory from five years ago. > > > > > > From what I recall, the problem was that block jobs weren't getting > > > drained/paused when the backend was getting quiesced -- we wanted to > > > be sure that a blockjob wasn't continuing to run and submit requests > > > against a backend we wanted to have on ice during a sensitive > > > operation. This conditional stanza here is meant to check if the node > > > we're already attached to is *already quiesced* and we missed the > > > signal (so-to-speak), so we replay the drained_begin() request right > > > there. > > > > > > i.e. there was some case where blockjobs were getting added to an > > > already quiesced node, and this code here post-hoc relays that drain > > > request to the blockjob. This gets used in > > > 600ac6a0ef5c06418446ef2f37407bddcc51b21c to pause/unpause jobs. > > > Original thread is here: > > > https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg03416.html > > > > > > Now, I'm not sure why you want to set ops to NULL here. If we're in a > > > drained section, that sounds like it might be potentially bad because > > > we lose track of the operation to end the drained section. If your > > > intent is to destroy the thing that we'd need to call drained_end on, > > > I guess it doesn't matter -- provided you've cleaned up the target > > > object correctly. Just calling drained_end() pre-emptively seems like > > > it might be bad, what if it unpauses something you're in the middle of > > > trying to delete? > > > > > > I might need slightly more context to know what you're hoping to > > > accomplish, but I hope this info helps contextualize this code > > > somewhat. > > > > Setting to NULL in this patch is a subset of blk_detach_dev(), which > > gets called when a storage controller is hot unplugged. > > > > In this patch series there is no DeviceState because a VDUSE export is > > not a device. The VDUSE code calls blk_set_dev_ops() to > > register/unregister callbacks (e.g. ->resize_cb()). > > > > The reason I asked about ->drained_end() is for symmetry. If the > > device's ->drained_begin() callback changed state or allocated resources > > then they may need to be freed/reset. On the other hand, the > > blk_set_dev_ops(blk, NULL, NULL) call should be made by the dev_ops > > owner so they can clean up without a ->drained_end() call. > > OK, got it... Hm, we don't actually use these for BlockJobs anymore. > It looks like the only user of these callbacks now is the NBD driver. > > ad90febaf22d95e49fb6821bfb3ebd05b4919417 followed not long after my > initial patch and removed my intended user. I tried just removing the > fields, but the build chokes on NBD. > It looks like these usages are pretty modern, Sergio added them in > fd6afc50 (2021-06-02). So, I guess we do actually still use these > hooks. (After a period of maybe not using them for 4 years? Wow.) > > I'm not clear on what we *want* to happen here, though. It doesn't > sound like NBD is the anticipated use case, so maybe just make the > removal fail if the drained section is active and callbacks are > defined? That's the safe thing to do, probably.
I'm not sure either. CCing Eric. Maybe the answer is to just leave it as-is. Stefan
signature.asc
Description: PGP signature
