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

[John Snow]

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