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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature