[Devel] [PATCH vz7 v3 2/2] block: store exec_ub on struct request and use it

[Kirill Tkhai]

On 27.08.2018 17:50, Konstantin Khorenko wrote:
> Good calltrace: we set proper exec_ub in ploop_req_state_process()
> and later process requests - accounting goes to correct ub.
> 
> loop35172  6275 [002]   755.716683:  probe:ub_writeback_io_3:
> 			(ffffffff813547aa) ub=0xffff880394a04000
>                   5547ab deadline_add_request
>                   52e76a __elv_add_request
>                   536350 blk_flush_plug_list
>                   5365aa blk_queue_bio
>                   5345bb generic_make_request
>                   534843 submit_bio
>                     43e4 dio_submit ([pio_direct])
>                     616f ploop_entry_request ([ploop])
>                     7209 ploop_req_state_process ([ploop])
>                     7735 ploop_thread ([ploop])
> 
> Bad calltrace: due to task->plug io batching we may miss calling
> ploop_req_state_process() and process requests with ploop thread's ub == ub0:
> 
> ploop35172  6275 [002]   755.716763:  probe:ub_writeback_io_2:
> 			 (ffffffff81354785) ub=0xffffffff81f7b880 (ub0!!!)
>                   554786 deadline_add_request
>                   52e76a __elv_add_request
>                   536350 blk_flush_plug_list
>                   536714 blk_finish_plug
>                     1a93 ploop_wait ([ploop])
>                     77a1 ploop_thread ([ploop])
> 
> => need to store exec_ub in struct request as well.
> 
> We are safe to check for NULL request:req_ub to detect set it or not
> because request is zeroed on creation:
> 
>    get_request
>     __get_request
>      blk_rq_init
>       memset(rq, 0, sizeof(*rq));
> 
> https://jira.sw.ru/browse/PSBM-86910
> 
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai at virtuozzo.com>

> 
> v2 changes:
>  - store exec_ub to request in __get_request() instead of
>    blk_queue_bio(), this covers more potential cases
>  - __elv_add_request() must get/put ub because request can be freed
>    before the end of __elv_add_request() execution
> v3 changes:
>  - move blk_rq_set_ub() declaration into blk-core.c to avoid
>    including beancounter.h into lot of other files
> ---
>  block/blk-core.c       | 21 +++++++++++++++++++++
>  block/elevator.c       | 18 ++++++++++++++++++
>  include/linux/blkdev.h |  1 +
>  3 files changed, 40 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 45e2d9a93fc8..3bab718b3f5f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -950,6 +950,12 @@ EXPORT_SYMBOL(blk_get_queue);
>  
>  static inline void blk_free_request(struct request_list *rl, struct request *rq)
>  {
> +#ifdef CONFIG_BEANCOUNTERS
> +	if (rq->req_ub) {
> +		put_beancounter(rq->req_ub);
> +		rq->req_ub = NULL;
> +	}
> +#endif
>  	if (rq->cmd_flags & REQ_ELVPRIV) {
>  		elv_put_request(rl->q, rq);
>  		if (rq->elv.icq)
> @@ -1093,6 +1099,20 @@ static bool blk_rq_should_init_elevator(struct bio *bio)
>  	return true;
>  }
>  
> +/**
> + * blk_rq_set_ub - associate a request with a user beancounter
> + * @rq: request of interest
> + *
> + * Store current exec_ub in @rq so later we can use it while adding the request
> + * to elevator.
> + */
> +static inline void blk_rq_set_ub(struct request *rq)
> +{
> +#ifdef CONFIG_BEANCOUNTERS
> +	rq->req_ub = get_beancounter(get_exec_ub());
> +#endif
> +}
> +
>  /**
>   * __get_request - get a free request
>   * @rl: request list to allocate from
> @@ -1197,6 +1217,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
>  
>  	blk_rq_init(q, rq);
>  	blk_rq_set_rl(rq, rl);
> +	blk_rq_set_ub(rq);
>  	rq->cmd_flags = rw_flags | REQ_ALLOCED;
>  	if (flags & BLK_MQ_REQ_PREEMPT)
>  		rq->cmd_flags |= REQ_PREEMPT;
> diff --git a/block/elevator.c b/block/elevator.c
> index e81087c3e72d..8163ec4fd63d 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -35,6 +35,7 @@
>  #include <linux/hash.h>
>  #include <linux/uaccess.h>
>  #include <linux/pm_runtime.h>
> +#include <bc/beancounter.h>
>  
>  #include <trace/events/block.h>
>  
> @@ -658,6 +659,17 @@ void elv_drain_elevator(struct request_queue *q)
>  
>  void __elv_add_request(struct request_queue *q, struct request *rq, int where)
>  {
> +#ifdef CONFIG_BEANCOUNTERS
> +	struct user_beancounter *ub = NULL;
> +	if (rq->req_ub) {
> +		/*
> +		 * Request can be already freed at the end of func,
> +		 * so get beancounter.
> +		 */
> +		get_beancounter(rq->req_ub);
> +		ub = set_exec_ub(rq->req_ub);
> +	}
> +#endif
>  	trace_block_rq_insert(q, rq);
>  
>  	blk_pm_add_request(q, rq);
> @@ -734,6 +746,12 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
>  		       __func__, where);
>  		BUG();
>  	}
> +#ifdef CONFIG_BEANCOUNTERS
> +	if (ub) {
> +		ub = set_exec_ub(ub);
> +		put_beancounter(ub);
> +	}
> +#endif
>  }
>  EXPORT_SYMBOL(__elv_add_request);
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 55cba7563e57..49bb13fa4d4f 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -200,6 +200,7 @@ struct request {
>  #endif
>  
>  	unsigned short ioprio;
> +	struct user_beancounter *req_ub;
>  
>  	void *special;		/* opaque pointer available for LLD use */
>  	char *buffer;		/* kaddr of the current segment if available */
>