| Message ID | 20220216172945.31124-2-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: cleanup null_submit_bio() & alloc_cmd() | expand |
On 2/17/22 02:29, Chaitanya Kulkarni wrote: > Only caller of alloc_cmd() is null_submit_bio() unconditionally sets > second parameter to true and that is statically hard-coded in null_blk. > 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. > > Follow the pattern that we have in bio_alloc() to set the structure > members in the structure allocation function in alloc_cmd() and pass > bio to initialize newly allocated cmd->bio member. > > Follow the pattern in copy_to_nullb() to use result of one function call > (null_cache_active()) to be used as a parameter to another function call > (null_insert_page()), use result of alloc_cmd() as a first parameter to > the null_handle_cmd() in null_submit_bio() function. This allow us to > remove the local variable cmd on stack in null_submit_bio() that is in > fast path. > > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/block/null_blk/main.c | 29 ++++++++++++----------------- > 1 file changed, 12 insertions(+), 17 deletions(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 90b6bd2a114b..29e183719e77 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -720,26 +720,25 @@ 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 bio *bio) > { > 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; > - > + if (cmd) { > + cmd->bio = bio; > + 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; > } > > static void end_cmd(struct nullb_cmd *cmd) > @@ -1477,12 +1476,8 @@ static void null_submit_bio(struct bio *bio) > sector_t nr_sectors = bio_sectors(bio); > struct nullb *nullb = bio->bi_bdev->bd_disk->private_data; > struct nullb_queue *nq = nullb_to_queue(nullb); > - struct nullb_cmd *cmd; > - > - cmd = alloc_cmd(nq, 1); > - cmd->bio = bio; > > - null_handle_cmd(cmd, sector, nr_sectors, bio_op(bio)); > + null_handle_cmd(alloc_cmd(nq, bio), sector, nr_sectors, bio_op(bio)); > } > > static bool should_timeout_request(struct request *rq) I would have preferred the simple while () {} loop I suggested (this do { } while (1) is ugly...) but anyway, looks good. Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 90b6bd2a114b..29e183719e77 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -720,26 +720,25 @@ 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 bio *bio) { 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; - + if (cmd) { + cmd->bio = bio; + 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; } static void end_cmd(struct nullb_cmd *cmd) @@ -1477,12 +1476,8 @@ static void null_submit_bio(struct bio *bio) sector_t nr_sectors = bio_sectors(bio); struct nullb *nullb = bio->bi_bdev->bd_disk->private_data; struct nullb_queue *nq = nullb_to_queue(nullb); - struct nullb_cmd *cmd; - - cmd = alloc_cmd(nq, 1); - cmd->bio = bio; - null_handle_cmd(cmd, sector, nr_sectors, bio_op(bio)); + null_handle_cmd(alloc_cmd(nq, bio), sector, nr_sectors, bio_op(bio)); } static bool should_timeout_request(struct request *rq)
Only caller of alloc_cmd() is null_submit_bio() unconditionally sets second parameter to true and that is statically hard-coded in null_blk. 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. Follow the pattern that we have in bio_alloc() to set the structure members in the structure allocation function in alloc_cmd() and pass bio to initialize newly allocated cmd->bio member. Follow the pattern in copy_to_nullb() to use result of one function call (null_cache_active()) to be used as a parameter to another function call (null_insert_page()), use result of alloc_cmd() as a first parameter to the null_handle_cmd() in null_submit_bio() function. This allow us to remove the local variable cmd on stack in null_submit_bio() that is in fast path. Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/null_blk/main.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)