On 2/16/22 18:30, Chaitanya Kulkarni wrote:
> Only caller of alloc_cmd() is null_submit_bio() unconditionally sets
> second parameter to true & that is statically hard-coded in null_blk.
Nit: s/&/and (let's write in proper English :))
> There is no point in having statically hardcoded function parameter.
>
> Remove the unnecessary parameter can_wait and adjust the code so it
> can retain existing behavior of waiting when we don't get valid
> nullb_cmd from __alloc_cmd() in alloc_cmd().
>
> The restructured code avoids multiple return statements, multiple
> calls to __alloc_cmd() and resulting a fast path call to
> prepare_to_wait() due to removal of first alloc_cmd() call.
>
> Signed-off-by: Chaitanya Kulkarni <kch@xxxxxxxxxx>
> ---
> drivers/block/null_blk/main.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 13004beb48ca..d78fc3edb22e 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -719,26 +719,23 @@ static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq)
> return NULL;
> }
>
> -static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, int can_wait)
> +static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq)
> {
> struct nullb_cmd *cmd;
> DEFINE_WAIT(wait);
>
> - cmd = __alloc_cmd(nq);
> - if (cmd || !can_wait)
> - return cmd;
> -
> do {
> - prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE);
> + /*
> + * This avoids multiple return statements, multiple calls to
> + * __alloc_cmd() and a fast path call to prepare_to_wait().
> + */
> cmd = __alloc_cmd(nq);
> if (cmd)
> - break;
> -
> + return cmd;
> + prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE);
> io_schedule();
> + finish_wait(&nq->wait, &wait);
> } while (1);
> -
> - finish_wait(&nq->wait, &wait);
> - return cmd;
> }
Personally, I would simplify further like this:
while (!(cmd = __alloc_cmd(nq)) {
prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE);
io_schedule();
finish_wait(&nq->wait, &wait);
}
return cmd;
But that is a matter of taste I guess.
>
> static void end_cmd(struct nullb_cmd *cmd)
> @@ -1478,7 +1475,7 @@ static void null_submit_bio(struct bio *bio)
> struct nullb_queue *nq = nullb_to_queue(nullb);
> struct nullb_cmd *cmd;
>
> - cmd = alloc_cmd(nq, 1);
> + cmd = alloc_cmd(nq);
> cmd->bio = bio;
>
> null_handle_cmd(cmd, sector, nr_sectors, bio_op(bio));
--
Damien Le Moal
Western Digital Research
