On 2/26/21 2:41 AM, Bodo Stroesser wrote:
> On 26.02.21 04:59, michael.christie@xxxxxxxxxx wrote:
>> On 2/24/21 12:53 PM, Bodo Stroesser wrote:
>>> This small series is based on Martin's for-next.
>>>
>>> Future patches will need something like the - meanwhile removed -
>>> radix_tree_for_each_contig macro.
>>> Since general direction is to use xarray as replacement for
>>> radix_tree and IDR, instead of re-introducing or open coding the
>>> removed macro, with this series we switch over to xarray API.
>>> Based on xarray a future patch easily can implement an analog
>>> to radix_tree_for_each_contig.
>>>
>>> Bodo Stroesser (2):
>>> scsi: target: tcmu: Replace IDR by XArray
>>> scsi: target: tcmu: Replace radix_tree with XArray
>>>
>>> drivers/target/target_core_user.c | 64 +++++++++++++++++++--------------------
>>> 1 file changed, 31 insertions(+), 33 deletions(-)
>>>
>>
>> Looks ok to me.
>>
>> Reviewed-by: Mike Christie <michael.christie@xxxxxxxxxx>
>>
>> I think in a separate patch we need to change:
>>
>> + if (xa_store(&udev->data_blocks, dbi, page, GFP_KERNEL))
>> goto err_insert;
>>
>> to GFP_NOIO. There were some users doing tcm loop + tcmu and
>> were hitting issues when GFP_KERNEL lead to write backs back on
>> to the same tcmu device. We tried to change all the gfp flags but
>> we missed that one, because it was hidden in the radix tree's
>> xa_flags.
>>
>
> But then, for consistency reasons, shouldn't we change
>
> + if (xa_alloc(&udev->commands, &cmd_id, tcmu_cmd, XA_LIMIT(1, 0xffff),
> + GFP_NOWAIT) < 0) {
>
> to GFP_NOIO also?
I think so, but am not sure. I've always wondered why we used
GFP_NOWAIT and meant to test with different gfp values but didn't
have time. It wouldn't hit the same issue I mentioned though.
>
> Additionally I have to change memory allocation in tcmu_tmr_notify from
> GFP_KERNEL to GFP_NOIO, since it happens while holding cmdr_lock mutex,
> which could also cause the problems you desribed.
>
> Shouldn't we better change to new API memalloc_noio_save and
> memalloc_noio_restore in that new patch?
I think it's your preference. I like to use the correct gfp
when I can, so you can just look at that chunk of code and
know it's right. If you put memalloc_noio_save in tcmu_queue_cmd
then a couple functions down you have a GFP_KERNEL it's
confusing when you are just searching the code.
I think memalloc_noio_save is handy in other places like the
iscsi/nbd code since when calling into the network stack you can't
control all the allocations sometimes.
