Re: [PATCH V4 0/6] null_blk: simplify null_handle_cmd()

[Jens Axboe <axboe@kernel.dk>]

On 8/22/19 10:45 PM, Chaitanya Kulkarni wrote:
> Hi,
> 
> The core function where we handle the request null_handle_cmd() in the
> null_blk does various things based on how null_blk is configured :-
> 
> 1. Handle throttling.
> 2. Handle badblocks.
> 3. Handle Memory backed device operations.
> 4. Handle Zoned Block device operations.
> 5. Completion of the requests.
> 
> With all the above functionality present in the one function,
> null_handle_cmd() is growing and becoming unreasonably lengthy when
> we want to add more features such as new Zone requests which is being
> worked on (See [1], [2]).
> 
> This is a cleanup patch-series which refactors the code in the
> null_handle_cmd(). We create a clear interface for each of the above
> mentioned functionality which leads to having null_handle_cmd() more
> clear and easy to manage with future changes. Please have a look at
> NVMe PCIe Driver (nvme_queue_rq()) (See [3]) which has a similar code
> structure and nicely structured code for doing various things such as
> setting up commands, mapping of the block layer requests, mapping
> PRPs/SGLs, handling integrity requests and finally submitting the
> NVMe Command etc.
> 
> With this patch-series we fix the following issues with the current
> code :-
> 
> 1. Get rid of the multiple nesting levels (> 2).
> 2. Easy to read, debug and extend the code for specific feature.
> 3. Get rid of the various line foldings which are there in the code
>     due to multiple level of nesting under if conditions.
> 4. Precise definition for the null_handle_cmd() and clear error
>     handling as helpers are responsible for handling errors.
> 
> Regards,
> Chaitanya
> 
> [1] https://www.spinics.net/lists/linux-block/msg41884.html
> [2] https://www.spinics.net/lists/linux-block/msg41883.html
> [3] https://github.com/torvalds/linux/blob/master/drivers/nvme/host/pci.c
> 
> * Changes from V3:-
> 1. Rename nullb_handle_cmd_completion () -> nullb_complete_cmd().
> 2. Move null_handle_zoned() to null_blk_zoned.c and adjust rest of the
>     code in null_blk_zoned.c.
> 3. Fixed the bug in null_handle_cmd() for REQ_OP_ZONE_RESET_ALL.
> 
> * Changes from V2:-
> 1. Adjust the code to latest upstream code.
> 
> * Changes from V1:-
> 1. Move bio vs req code into the callers for the null_handle_cmd() and
>     add required arguments to simplify the code in the same function.
> 2. Get rid of the extra braces for the null_handle_zoned().
> 3. Get rid of the multiple returns style and the goto.
> 4. For throttling, code keep the check in the caller.
> 5. Add uniform code format for setting the cmd->error in the
>     null_handle_cmd() and make required code changes so that each
>     feature specific function will return blk_status_t value.
> 
> Chaitanya Kulkarni (6):
>    null_blk: move duplicate code to callers
>    null_blk: create a helper for throttling
>    null_blk: create a helper for badblocks
>    null_blk: create a helper for mem-backed ops
>    null_blk: create a helper for zoned devices
>    null_blk: create a helper for req completion
> 
>   drivers/block/null_blk.h       |  13 +--
>   drivers/block/null_blk_main.c  | 162 +++++++++++++++++----------------
>   drivers/block/null_blk_zoned.c |  38 +++++---
>   3 files changed, 115 insertions(+), 98 deletions(-)

Applied, thanks.

-- 
Jens Axboe