>> drivers/block/null_blk/main.c | 90 +++++++++++++++++++++++++++++--
>> drivers/block/null_blk/null_blk.h | 10 ++++
>> 2 files changed, 97 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>> index 1f154f92f4c2..52db6b99b448 100644
>> --- a/drivers/block/null_blk/main.c
>> +++ b/drivers/block/null_blk/main.c
>> @@ -77,6 +77,10 @@ enum {
>> NULL_IRQ_TIMER = 2,
>> };
>>
>> +static unsigned int g_rq_abort_limit = 5;
>> +module_param_named(rq_abort_limit, g_rq_abort_limit, uint, 0644);
>> +MODULE_PARM_DESC(rq_abort_limit, "request timeout teardown limit. Default:5");
>
> Number of request timeout to trigger device teardown ?
>
> That would a lot clearer in my opinion.
okay, will add it to V2.
>
>> +
>> static bool g_virt_boundary = false;
>> module_param_named(virt_boundary, g_virt_boundary, bool, 0444);
>> MODULE_PARM_DESC(virt_boundary, "Require a virtual boundary for the device. Default: False");
>> @@ -247,6 +251,7 @@ static void null_del_dev(struct nullb *nullb);
>> static int null_add_dev(struct nullb_device *dev);
>> static struct nullb *null_find_dev_by_name(const char *name);
>> static void null_free_device_storage(struct nullb_device *dev, bool is_cache);
>> +static void null_destroy_dev(struct nullb *nullb);
>>
>> static inline struct nullb_device *to_nullb_device(struct config_item *item)
>> {
>> @@ -578,6 +583,18 @@ config_item *nullb_group_make_item(struct config_group *group, const char *name)
>> {
>> struct nullb_device *dev;
>>
>> + if (g_rq_abort_limit) {
>> + /*
>> + * abort_on_timeout removes the null_blk and resources. When
>
> ...the null_blk device and its resources. When the null_blk device is
> created using configfs, ...
>
> The remaining of the sentence does not parse at all.
okay, will rephrase it in V2.
>
>> + * null_blk is created using configfs entry by user we will also
>> + * need to cleanup the those entries when abort_on_timeout is set
>> + * from null_abort_work() and that we shold not do it, since
>> + * manupulating user's entries from kernel can create confusion,
>> + * so just don't allow it.
>> + */
>> + pr_err("don't use g_abort_on_timeout with configfs entries\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> if (null_find_dev_by_name(name))
>> return ERR_PTR(-EEXIST);
>>
>> @@ -614,7 +631,7 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page)
>> "poll_queues,power,queue_mode,shared_tag_bitmap,size,"
>> "submit_queues,use_per_node_hctx,virt_boundary,zoned,"
>> "zone_capacity,zone_max_active,zone_max_open,"
>> - "zone_nr_conv,zone_size\n");
>> + "zone_nr_conv,zone_size,abort_on_timeout,rq_abort_limit\n");
>
> Where is abort_on_timeout defined ? Nowhere to be seen. Does this patch
> even compile ? Also, assuming this is a boolean, why introduce it ?
Please note that this patch is complied and tested with the test report
of applying this patch see above for the git am, compile and running fio
n dd commands to trigger multiple timeouts and teardown.
The abort on timeout needs to be removed since I got confused between
what name should be used...
> Wouldn't using "rq_abort_limit > 0" be equivalent ?
>
this is exactly what it is doing right now, unless I missed that...
[...]
>> + /*
>> + * null_blk request timeout teardown limit when device is in the
>> + * stable state, i.e. once this limit is reached issue
>> + * null_abort_work() to teardown the device from block lyaer
>> + * request timeout callback and cleanup resources such as
>> + * memory and pathname.
>
> s/issue/execute
> s/lyaer/layer
>
> But I think this can be simplified to something like:
>
> /*
> * Number of requests timeout failures allowed before trigerring
> * a device teardown from the block layer request timeout callback.
> */
>
Okay, if that works for everyone will add to V2.
>> + */
>> + atomic_t rq_abort_count;
>> + /* tear down work to be scheduled from block layer request handler */
>
> This comment is not really useful.
>
>> + struct work_struct abort_work;
>> char disk_name[DISK_NAME_LEN];
>> };
>>
>
thanks for the comments, will send out V2 with comments fixed.
-ck
