| Message ID | 20210224185335.13844-1-bostroesser@gmail.com |
|---|---|
| Headers | show |
| Series | scsi: target: tcmu: Replace IDR and radix_tree with XArray | expand |
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@oracle.com> 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.
On 26.02.21 04:59, michael.christie@oracle.com 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@oracle.com> > > 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? 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?
On 2/26/21 2:41 AM, Bodo Stroesser wrote: > On 26.02.21 04:59, michael.christie@oracle.com 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@oracle.com> >> >> 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.
On 26.02.21 17:04, Mike Christie wrote: > On 2/26/21 2:41 AM, Bodo Stroesser wrote: >> On 26.02.21 04:59, michael.christie@oracle.com 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@oracle.com> >>> >>> 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. > I'll send a patch changing it to GFP_NOIO. There is no reason to disallow sleeping. Just IO should be avoided as you said. >> >> 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. > Good point. I'll stay with GFP_NOIO. > 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. > > >
Bodo, > 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. Applied to 5.13/scsi-staging, thanks! -- Martin K. Petersen Oracle Linux Engineering
On Wed, 24 Feb 2021 19:53:33 +0100, 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. > > [...] Applied to 5.13/scsi-queue, thanks! [1/2] scsi: target: tcmu: Replace IDR by XArray https://git.kernel.org/mkp/scsi/c/d3cbb743c362 [2/2] scsi: target: tcmu: Replace radix_tree with XArray https://git.kernel.org/mkp/scsi/c/f7c89771d07d -- Martin K. Petersen Oracle Linux Engineering