[v5,05/12] block: Introduce "drained begin/end" API

The semantics is that after bdrv_drained_begin(bs), bs will not get new external
requests until the matching bdrv_drained_end(bs).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c                | 17 +++++++++++++++++
 include/block/block.h     | 21 +++++++++++++++++++++
 include/block/block_int.h |  2 ++
 3 files changed, 40 insertions(+)

On Wed, Oct 21, 2015 at 10:06:42AM +0800, Fam Zheng wrote:
> The semantics is that after bdrv_drained_begin(bs), bs will not get new external
> requests until the matching bdrv_drained_end(bs).
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c                | 17 +++++++++++++++++
>  include/block/block.h     | 21 +++++++++++++++++++++
>  include/block/block_int.h |  2 ++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 2fd7a1d..5ac6256 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2624,3 +2624,20 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
>      }
>      bdrv_start_throttled_reqs(bs);
>  }
> +
> +void bdrv_drained_begin(BlockDriverState *bs)
> +{
> +    if (!bs->quiesce_counter++) {
> +        aio_disable_external(bdrv_get_aio_context(bs));
> +    }
> +    bdrv_drain(bs);
> +}
> +
> +void bdrv_drained_end(BlockDriverState *bs)
> +{
> +    assert(bs->quiesce_counter > 0);
> +    if (--bs->quiesce_counter > 0) {
> +        return;
> +    }
> +    aio_enable_external(bdrv_get_aio_context(bs));
> +}

Why do we need a quiesce counter, given that
aio_{disable, enable}_external() increments / decrements a counter?



> diff --git a/include/block/block.h b/include/block/block.h
> index 28d903c..6d38b62 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -610,4 +610,25 @@ void bdrv_io_plug(BlockDriverState *bs);
>  void bdrv_io_unplug(BlockDriverState *bs);
>  void bdrv_flush_io_queue(BlockDriverState *bs);
>  
> +BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
> +

Is the above line from a bad rebase?


> +/**
> + * bdrv_drained_begin:
> + *
> + * Begin a quiesced section for exclusive access to the BDS, by disabling
> + * external request sources including NBD server and device model. Note that
> + * this doesn't block timers or coroutines from submitting more requests, which
> + * means block_job_pause is still necessary.
> + *
> + * This function can be recursive.
> + */
> +void bdrv_drained_begin(BlockDriverState *bs);
> +
> +/**
> + * bdrv_drained_end:
> + *
> + * End a quiescent section started by bdrv_drained_begin().
> + */
> +void bdrv_drained_end(BlockDriverState *bs);
> +
>  #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index e472a03..e317b14 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -448,6 +448,8 @@ struct BlockDriverState {
>      /* threshold limit for writes, in bytes. "High water mark". */
>      uint64_t write_threshold_offset;
>      NotifierWithReturn write_threshold_notifier;
> +
> +    int quiesce_counter;
>  };
>  
>  struct BlockBackendRootState {
> -- 
> 2.4.3
> 
>

On Wed, 10/21 12:11, Jeff Cody wrote:
> On Wed, Oct 21, 2015 at 10:06:42AM +0800, Fam Zheng wrote:
> > The semantics is that after bdrv_drained_begin(bs), bs will not get new external
> > requests until the matching bdrv_drained_end(bs).
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/io.c                | 17 +++++++++++++++++
> >  include/block/block.h     | 21 +++++++++++++++++++++
> >  include/block/block_int.h |  2 ++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 2fd7a1d..5ac6256 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -2624,3 +2624,20 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
> >      }
> >      bdrv_start_throttled_reqs(bs);
> >  }
> > +
> > +void bdrv_drained_begin(BlockDriverState *bs)
> > +{
> > +    if (!bs->quiesce_counter++) {
> > +        aio_disable_external(bdrv_get_aio_context(bs));
> > +    }
> > +    bdrv_drain(bs);
> > +}
> > +
> > +void bdrv_drained_end(BlockDriverState *bs)
> > +{
> > +    assert(bs->quiesce_counter > 0);
> > +    if (--bs->quiesce_counter > 0) {
> > +        return;
> > +    }
> > +    aio_enable_external(bdrv_get_aio_context(bs));
> > +}
> 
> Why do we need a quiesce counter, given that
> aio_{disable, enable}_external() increments / decrements a counter?

No longer necessary in this version because we call bdrv_drain unconditionally,
but this makes it possible to introducd bdrv_is_quiesced() and assertion on it,
so I lean toward keeping it as it doesn't hurt.

> 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 28d903c..6d38b62 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -610,4 +610,25 @@ void bdrv_io_plug(BlockDriverState *bs);
> >  void bdrv_io_unplug(BlockDriverState *bs);
> >  void bdrv_flush_io_queue(BlockDriverState *bs);
> >  
> > +BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
> > +
> 
> Is the above line from a bad rebase?

Yes, will remove.