Re: [PATCH 0/2] scsi: target: tcmu: Replace IDR and radix_tree with XArray

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.






[Index of Archives]     [Linux SCSI]     [Kernel Newbies]     [Linux SCSI Target Infrastructure]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Device Mapper]

  Powered by Linux