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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5957 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]