| Message ID | 20220215105943.90-2-xieyongji@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support exporting BDSs via VDUSE | expand |
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 <xieyongji@bytedance.com> > --- > 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
On Mon, Mar 14, 2022 at 1:23 PM Stefan Hajnoczi <stefanha@redhat.com> 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 <xieyongji@bytedance.com> > > --- > > 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. --js
On Mon, Mar 14, 2022 at 03:09:35PM -0400, John Snow wrote: > On Mon, Mar 14, 2022 at 1:23 PM Stefan Hajnoczi <stefanha@redhat.com> 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 <xieyongji@bytedance.com> > > > --- > > > 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. Stefan
On Tue, Mar 15, 2022 at 4:47 AM Stefan Hajnoczi <stefanha@redhat.com> 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 <stefanha@redhat.com> 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 <xieyongji@bytedance.com> > > > > --- > > > > 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. --js
On Tue, Mar 15, 2022 at 03:30:22PM -0400, John Snow wrote: > On Tue, Mar 15, 2022 at 4:47 AM Stefan Hajnoczi <stefanha@redhat.com> 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 <stefanha@redhat.com> 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 <xieyongji@bytedance.com> > > > > > --- > > > > > 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
Am 16.03.2022 um 13:18 hat Stefan Hajnoczi geschrieben: > On Tue, Mar 15, 2022 at 03:30:22PM -0400, John Snow wrote: > > On Tue, Mar 15, 2022 at 4:47 AM Stefan Hajnoczi <stefanha@redhat.com> 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 <stefanha@redhat.com> 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 <xieyongji@bytedance.com> > > > > > > --- > > > > > > 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. NBD never calls blk_set_dev_ops(NULL), so does the specific case matter? I guess the question is when blk_set_dev_ops(NULL) should be called in general and what that means for draining. blk_set_dev_ops() is currently taken as attaching a new user, and if its BlockBackend is drained, the user needs to be aware of that because it should not send requests until the node is undrained. What does blk_set_dev_ops(NULL) mean, though? That the user has gone away? If so, it doesn't make a difference because it won't send requests anyway. If it's only about to go away, calling .drained_end() might make it send requests even though the node is still drained. This would be a bug. It looks to me as if it's not necessary to balance .drain_begin/end on the BlockDevOps level to get some counter back to zero. A user can safely go away while the node is still drained. Kevin
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); } }
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 <xieyongji@bytedance.com> --- block/block-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)