Re: [PATCH v2 1/6] block: Support passing NULL ops to blk_set_dev_ops()

[John Snow]

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