* [PATCH] scsi: replace GFP_ATOMIC with GFP_KERNEL for sdev allocation @ 2019-02-20 18:48 Benjamin Block 2019-02-20 19:11 ` Bart Van Assche 2019-02-21 9:18 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c Benjamin Block 0 siblings, 2 replies; 7+ messages in thread From: Benjamin Block @ 2019-02-20 18:48 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen Cc: Benjamin Block, Steffen Maier, linux-scsi, linux-s390, linux-kernel We had a test-report where, under memory pressure, adding LUNs to the systems would fail (the tests add LUNs strictly in sequence): [ 5525.853432] scsi 0:0:1:1088045124: Direct-Access IBM 2107900 .148 PQ: 0 ANSI: 5 [ 5525.853826] scsi 0:0:1:1088045124: alua: supports implicit TPGS [ 5525.853830] scsi 0:0:1:1088045124: alua: device naa.6005076303ffd32700000000000044da port group 0 rel port 43 [ 5525.853931] sd 0:0:1:1088045124: Attached scsi generic sg10 type 0 [ 5525.854075] sd 0:0:1:1088045124: [sdk] Disabling DIF Type 1 protection [ 5525.855495] sd 0:0:1:1088045124: [sdk] 2097152 512-byte logical blocks: (1.07 GB/1.00 GiB) [ 5525.855606] sd 0:0:1:1088045124: [sdk] Write Protect is off [ 5525.855609] sd 0:0:1:1088045124: [sdk] Mode Sense: ed 00 00 08 [ 5525.855795] sd 0:0:1:1088045124: [sdk] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 5525.857838] sdk: sdk1 [ 5525.859468] sd 0:0:1:1088045124: [sdk] Attached SCSI disk [ 5525.865073] sd 0:0:1:1088045124: alua: transition timeout set to 60 seconds [ 5525.865078] sd 0:0:1:1088045124: alua: port group 00 state A preferred supports tolusnA [ 5526.015070] sd 0:0:1:1088045124: alua: port group 00 state A preferred supports tolusnA [ 5526.015213] sd 0:0:1:1088045124: alua: port group 00 state A preferred supports tolusnA [ 5526.587439] scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured [ 5526.588562] scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured Looking at the code of scsi_alloc_sdev(), and all the calling contexts, there seems to be no reason to use GFP_ATMOIC here. All the different call-contexts use a mutex at some point, and nothing in between that requires no sleeping, as far as I could see. Additionally, the code that allocates the block queue for the device later (scsi_mq_alloc_queue()) already uses GFP_KERNEL. So replace it, and give the allocation a bit of a better chance to succeed, with more ways of reclaim. Signed-off-by: Benjamin Block <bblock@linux.ibm.com> --- drivers/scsi/scsi_scan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index dd0d516f65e2..e49e6099b852 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -220,7 +220,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size, - GFP_ATOMIC); + GFP_KERNEL); if (!sdev) goto out; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: replace GFP_ATOMIC with GFP_KERNEL for sdev allocation 2019-02-20 18:48 [PATCH] scsi: replace GFP_ATOMIC with GFP_KERNEL for sdev allocation Benjamin Block @ 2019-02-20 19:11 ` Bart Van Assche 2019-02-20 20:03 ` Benjamin Block 2019-02-21 9:18 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c Benjamin Block 1 sibling, 1 reply; 7+ messages in thread From: Bart Van Assche @ 2019-02-20 19:11 UTC (permalink / raw) To: Benjamin Block, James E . J . Bottomley, Martin K . Petersen Cc: Steffen Maier, linux-scsi, linux-s390, linux-kernel On Wed, 2019-02-20 at 19:48 +0100, Benjamin Block wrote: > We had a test-report where, under memory pressure, adding LUNs to the > systems would fail (the tests add LUNs strictly in sequence): Hi Benjamin, There are two more instances of GFP_ATOMIC in scsi_scan.c. Have you verified whether or not it is safe to change these into GFP_KERNEL too? Thanks, Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: replace GFP_ATOMIC with GFP_KERNEL for sdev allocation 2019-02-20 19:11 ` Bart Van Assche @ 2019-02-20 20:03 ` Benjamin Block 0 siblings, 0 replies; 7+ messages in thread From: Benjamin Block @ 2019-02-20 20:03 UTC (permalink / raw) To: Bart Van Assche Cc: James E . J . Bottomley, Martin K . Petersen, Steffen Maier, linux-scsi, linux-s390, linux-kernel On Wed, Feb 20, 2019 at 11:11:31AM -0800, Bart Van Assche wrote: > On Wed, 2019-02-20 at 19:48 +0100, Benjamin Block wrote: > > We had a test-report where, under memory pressure, adding LUNs to the > > systems would fail (the tests add LUNs strictly in sequence): > > Hi Benjamin, > > There are two more instances of GFP_ATOMIC in scsi_scan.c. Have you verified > whether or not it is safe to change these into GFP_KERNEL too? > No, I was lazy, but I can take a look tomorrow and fix them up as well if they are similar to scsi_alloc_sdev(). -- With Best Regards, Benjamin Block / Linux on IBM Z Kernel Development IBM Systems & Technology Group / IBM Deutschland Research & Development GmbH Vorsitz. AufsR.: Matthias Hartmann / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c 2019-02-20 18:48 [PATCH] scsi: replace GFP_ATOMIC with GFP_KERNEL for sdev allocation Benjamin Block 2019-02-20 19:11 ` Bart Van Assche @ 2019-02-21 9:18 ` Benjamin Block 2019-02-21 9:18 ` [PATCH v2 2/2] scsi: whitespace cleanup " Benjamin Block ` (2 more replies) 1 sibling, 3 replies; 7+ messages in thread From: Benjamin Block @ 2019-02-21 9:18 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen Cc: Benjamin Block, Steffen Maier, linux-scsi, linux-s390, linux-kernel, Bart Van Assche We had a test-report where, under memory pressure, adding LUNs to the systems would fail (the tests add LUNs strictly in sequence): [ 5525.853432] scsi 0:0:1:1088045124: Direct-Access IBM 2107900 .148 PQ: 0 ANSI: 5 [ 5525.853826] scsi 0:0:1:1088045124: alua: supports implicit TPGS [ 5525.853830] scsi 0:0:1:1088045124: alua: device naa.6005076303ffd32700000000000044da port group 0 rel port 43 [ 5525.853931] sd 0:0:1:1088045124: Attached scsi generic sg10 type 0 [ 5525.854075] sd 0:0:1:1088045124: [sdk] Disabling DIF Type 1 protection [ 5525.855495] sd 0:0:1:1088045124: [sdk] 2097152 512-byte logical blocks: (1.07 GB/1.00 GiB) [ 5525.855606] sd 0:0:1:1088045124: [sdk] Write Protect is off [ 5525.855609] sd 0:0:1:1088045124: [sdk] Mode Sense: ed 00 00 08 [ 5525.855795] sd 0:0:1:1088045124: [sdk] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 5525.857838] sdk: sdk1 [ 5525.859468] sd 0:0:1:1088045124: [sdk] Attached SCSI disk [ 5525.865073] sd 0:0:1:1088045124: alua: transition timeout set to 60 seconds [ 5525.865078] sd 0:0:1:1088045124: alua: port group 00 state A preferred supports tolusnA [ 5526.015070] sd 0:0:1:1088045124: alua: port group 00 state A preferred supports tolusnA [ 5526.015213] sd 0:0:1:1088045124: alua: port group 00 state A preferred supports tolusnA [ 5526.587439] scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured [ 5526.588562] scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured Looking at the code of scsi_alloc_sdev(), and all the calling contexts, there seems to be no reason to use GFP_ATMOIC here. All the different call-contexts use a mutex at some point, and nothing in between that requires no sleeping, as far as I could see. Additionally, the code that later allocates the block queue for the device (scsi_mq_alloc_queue()) already uses GFP_KERNEL. There are similar allocations in two other functions: scsi_probe_and_add_lun(), and scsi_add_lun(),; that can also be done with GFP_KERNEL. Here is the contexts for the three functions so far: scsi_alloc_sdev() scsi_probe_and_add_lun() scsi_sequential_lun_scan() __scsi_scan_target() scsi_scan_target() mutex_lock() scsi_scan_channel() scsi_scan_host_selected() mutex_lock() scsi_report_lun_scan() __scsi_scan_target() ... __scsi_add_device() mutex_lock() __scsi_scan_target() ... scsi_report_lun_scan() ... scsi_get_host_dev() mutex_lock() scsi_probe_and_add_lun() ... scsi_add_lun() scsi_probe_and_add_lun() ... So replace all these, and give them a bit of a better chance to succeed, with more chances of reclaim. Signed-off-by: Benjamin Block <bblock@linux.ibm.com> --- drivers/scsi/scsi_scan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index dd0d516f65e2..53380e07b40e 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -220,7 +220,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size, - GFP_ATOMIC); + GFP_KERNEL); if (!sdev) goto out; @@ -788,7 +788,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, */ sdev->inquiry = kmemdup(inq_result, max_t(size_t, sdev->inquiry_len, 36), - GFP_ATOMIC); + GFP_KERNEL); if (sdev->inquiry == NULL) return SCSI_SCAN_NO_RESPONSE; @@ -1079,7 +1079,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget, if (!sdev) goto out; - result = kmalloc(result_len, GFP_ATOMIC | + result = kmalloc(result_len, GFP_KERNEL | ((shost->unchecked_isa_dma) ? __GFP_DMA : 0)); if (!result) goto out_free_sdev; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] scsi: whitespace cleanup in scsi_scan.c 2019-02-21 9:18 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c Benjamin Block @ 2019-02-21 9:18 ` Benjamin Block 2019-02-21 18:45 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations " Bart Van Assche 2019-02-27 14:40 ` Martin K. Petersen 2 siblings, 0 replies; 7+ messages in thread From: Benjamin Block @ 2019-02-21 9:18 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen Cc: Benjamin Block, Steffen Maier, linux-scsi, linux-s390, linux-kernel, Bart Van Assche Noticed during editing that vim would remove some trailing spaces. Signed-off-by: Benjamin Block <bblock@linux.ibm.com> --- drivers/scsi/scsi_scan.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 53380e07b40e..7e1a6c3dd42c 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -108,7 +108,7 @@ MODULE_PARM_DESC(scan, "sync, async, manual, or none. " static unsigned int scsi_inq_timeout = SCSI_TIMEOUT/HZ + 18; module_param_named(inq_timeout, scsi_inq_timeout, uint, S_IRUGO|S_IWUSR); -MODULE_PARM_DESC(inq_timeout, +MODULE_PARM_DESC(inq_timeout, "Timeout (in seconds) waiting for devices to answer INQUIRY." " Default is 20. Some devices may need more; most need less."); @@ -604,7 +604,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, * not-ready to ready transition [asc/ascq=0x28/0x0] * or power-on, reset [asc/ascq=0x29/0x0], continue. * INQUIRY should not yield UNIT_ATTENTION - * but many buggy devices do so anyway. + * but many buggy devices do so anyway. */ if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr)) { @@ -850,7 +850,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, * Don't set the device offline here; rather let the upper * level drivers eval the PQ to decide whether they should * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check. - */ + */ sdev->inq_periph_qual = (inq_result[0] >> 5) & 7; sdev->lockable = sdev->removable; @@ -994,7 +994,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, } #ifdef CONFIG_SCSI_LOGGING -/** +/** * scsi_inq_str - print INQUIRY data from min to max index, strip trailing whitespace * @buf: Output buffer with at least end-first+1 bytes of space * @inq: Inquiry buffer (input) @@ -1495,7 +1495,7 @@ EXPORT_SYMBOL(__scsi_add_device); int scsi_add_device(struct Scsi_Host *host, uint channel, uint target, u64 lun) { - struct scsi_device *sdev = + struct scsi_device *sdev = __scsi_add_device(host, channel, target, lun, NULL); if (IS_ERR(sdev)) return PTR_ERR(sdev); -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c 2019-02-21 9:18 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c Benjamin Block 2019-02-21 9:18 ` [PATCH v2 2/2] scsi: whitespace cleanup " Benjamin Block @ 2019-02-21 18:45 ` Bart Van Assche 2019-02-27 14:40 ` Martin K. Petersen 2 siblings, 0 replies; 7+ messages in thread From: Bart Van Assche @ 2019-02-21 18:45 UTC (permalink / raw) To: Benjamin Block, James E . J . Bottomley, Martin K . Petersen Cc: Steffen Maier, linux-scsi, linux-s390, linux-kernel On Thu, 2019-02-21 at 10:18 +0100, Benjamin Block wrote: > Looking at the code of scsi_alloc_sdev(), and all the calling contexts, > there seems to be no reason to use GFP_ATMOIC here. All the different > call-contexts use a mutex at some point, and nothing in between that > requires no sleeping, as far as I could see. Additionally, the code that > later allocates the block queue for the device (scsi_mq_alloc_queue()) > already uses GFP_KERNEL. > > > [ ... ] > > So replace all these, and give them a bit of a better chance to succeed, > with more chances of reclaim. Reviewed-by: Bart Van Assche <bvanassche@acm.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c 2019-02-21 9:18 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c Benjamin Block 2019-02-21 9:18 ` [PATCH v2 2/2] scsi: whitespace cleanup " Benjamin Block 2019-02-21 18:45 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations " Bart Van Assche @ 2019-02-27 14:40 ` Martin K. Petersen 2 siblings, 0 replies; 7+ messages in thread From: Martin K. Petersen @ 2019-02-27 14:40 UTC (permalink / raw) To: Benjamin Block Cc: James E . J . Bottomley, Martin K . Petersen, Steffen Maier, linux-scsi, linux-s390, linux-kernel, Bart Van Assche Benjamin, > We had a test-report where, under memory pressure, adding LUNs to the > systems would fail (the tests add LUNs strictly in sequence): Applied to 5.1/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-27 14:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-20 18:48 [PATCH] scsi: replace GFP_ATOMIC with GFP_KERNEL for sdev allocation Benjamin Block 2019-02-20 19:11 ` Bart Van Assche 2019-02-20 20:03 ` Benjamin Block 2019-02-21 9:18 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations in scsi_scan.c Benjamin Block 2019-02-21 9:18 ` [PATCH v2 2/2] scsi: whitespace cleanup " Benjamin Block 2019-02-21 18:45 ` [PATCH v2 1/2] scsi: replace GFP_ATOMIC with GFP_KERNEL for allocations " Bart Van Assche 2019-02-27 14:40 ` Martin K. Petersen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.