possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
Hello,
==
WARNING: possible circular locking dependency detected
4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
--
fsck.ext4/148 is trying to acquire lock:
(&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
but now in release context of a crosslock acquired at the following:
((complete)&wait#2){+.+.}, at: [] blk_execute_rq+0xbb/0xda
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 ((complete)&wait#2){+.+.}:
lock_acquire+0x176/0x19e
__wait_for_common+0x50/0x1e3
blk_execute_rq+0xbb/0xda
scsi_execute+0xc3/0x17d [scsi_mod]
sd_revalidate_disk+0x112/0x1549 [sd_mod]
rescan_partitions+0x48/0x2c4
__blkdev_get+0x14b/0x37c
blkdev_get+0x191/0x2c0
device_add_disk+0x2b4/0x3e5
sd_probe_async+0xf8/0x17e [sd_mod]
async_run_entry_fn+0x34/0xe0
process_one_work+0x2af/0x4d1
worker_thread+0x19a/0x24f
kthread+0x133/0x13b
ret_from_fork+0x27/0x40
-> #0 (&bdev->bd_mutex){+.+.}:
__blkdev_put+0x33/0x190
blkdev_close+0x24/0x27
__fput+0xee/0x18a
task_work_run+0x79/0xa0
prepare_exit_to_usermode+0x9b/0xb5
other info that might help us debug this:
Possible unsafe locking scenario by crosslock:
CPU0CPU1
lock(&bdev->bd_mutex);
lock((complete)&wait#2);
lock(&bdev->bd_mutex);
unlock((complete)&wait#2);
*** DEADLOCK ***
4 locks held by fsck.ext4/148:
#0: (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
#1: (rcu_read_lock){}, at: [] rcu_lock_acquire+0x0/0x20
#2: (&(&host->lock)->rlock){-.-.}, at: []
ata_scsi_queuecmd+0x23/0x74 [libata]
#3: (&x->wait#14){-...}, at: [] complete+0x18/0x50
stack backtrace:
CPU: 1 PID: 148 Comm: fsck.ext4 Not tainted
4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746
Call Trace:
dump_stack+0x67/0x8e
print_circular_bug+0x2a1/0x2af
? zap_class+0xc5/0xc5
check_prev_add+0x76/0x20d
? __lock_acquire+0xc27/0xcc8
lock_commit_crosslock+0x327/0x35e
complete+0x24/0x50
scsi_end_request+0x8d/0x176 [scsi_mod]
scsi_io_completion+0x1be/0x423 [scsi_mod]
__blk_mq_complete_request+0x112/0x131
ata_scsi_simulate+0x212/0x218 [libata]
__ata_scsi_queuecmd+0x1be/0x1de [libata]
ata_scsi_queuecmd+0x41/0x74 [libata]
scsi_dispatch_cmd+0x194/0x2af [scsi_mod]
scsi_queue_rq+0x1e0/0x26f [scsi_mod]
blk_mq_dispatch_rq_list+0x193/0x2a7
? _raw_spin_unlock+0x2e/0x40
blk_mq_sched_dispatch_requests+0x132/0x176
__blk_mq_run_hw_queue+0x59/0xc5
__blk_mq_delay_run_hw_queue+0x5f/0xc1
blk_mq_flush_plug_list+0xfc/0x10b
blk_flush_plug_list+0xc6/0x1eb
blk_finish_plug+0x25/0x32
generic_writepages+0x56/0x63
do_writepages+0x36/0x70
__filemap_fdatawrite_range+0x59/0x5f
filemap_write_and_wait+0x19/0x4f
__blkdev_put+0x5f/0x190
blkdev_close+0x24/0x27
__fput+0xee/0x18a
task_work_run+0x79/0xa0
prepare_exit_to_usermode+0x9b/0xb5
entry_SYSCALL_64_fastpath+0xab/0xad
RIP: 0033:0x7ff5755a2f74
RSP: 002b:7ffe46fce038 EFLAGS: 0246 ORIG_RAX: 0003
RAX: RBX: 555ddeddded0 RCX: 7ff5755a2f74
RDX: 1000 RSI: 555ddede2580 RDI: 0004
RBP: R08: 555ddede2580 R09: 555ddedde080
R10: 00010800 R11: 0246 R12: 555ddedddfa0
R13: 7ff576523680 R14: 1000 R15: 555ddeddc2b0
-ss
Re: [PATCH 15/15] usb: make device_type const
On Sat, Aug 19, 2017 at 01:52:26PM +0530, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
>
> Signed-off-by: Bhumika Goyal
Acked-by: Heikki Krogerus
> ---
> drivers/usb/common/ulpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 930e8f3..4aa5195 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -135,7 +135,7 @@ static void ulpi_dev_release(struct device *dev)
> kfree(to_ulpi_dev(dev));
> }
>
> -static struct device_type ulpi_dev_type = {
> +static const struct device_type ulpi_dev_type = {
> .name = "ulpi_device",
> .groups = ulpi_dev_attr_groups,
> .release = ulpi_dev_release,
Thanks,
--
heikki
RE: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)
> Wouldn't it make sense to backport the changes to set the virt_boundary > (which probably still is the SG_GAPS flag in such an old kernel)? We can make storvsc use SG_GAPS. But the following patch is missing in 4.1 stable block layer to make this work on some I/O situations. Backporting is more difficult and affect other code. commit 5e7c4274a70aa2d6f485996d0ca1dad52d0039ca Author: Jens Axboe Date: Thu Sep 3 19:28:20 2015 +0300 block: Check for gaps on front and back merges We are checking for gaps to previous bio_vec, which can only detect back merges gaps. Moreover, at the point where we check for a gap, we don't know if we will attempt a back or a front merge. Thus, check for gap to prev in a back merge attempt and check for a gap to next in a front merge attempt. Signed-off-by: Jens Axboe [sagig: Minor rename change] Signed-off-by: Sagi Grimberg
[PATCH] scsi: lpfc: remove useless code in lpfc_sli4_bsg_link_diag_test
Remove variable assignments. The value stored in local variable _rc_ is
overwritten at line 2448:rc = lpfc_sli4_bsg_set_link_diag_state(phba, 0);
before it can be used.
Addresses-Coverity-ID: 1226935
Signed-off-by: Gustavo A. R. Silva
---
This issue was detected by Coverity and it was tested by compilation only.
Notice that this code has been there since 2011.
drivers/scsi/lpfc/lpfc_bsg.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c
index a1686c2..fe9e1c0 100644
--- a/drivers/scsi/lpfc/lpfc_bsg.c
+++ b/drivers/scsi/lpfc/lpfc_bsg.c
@@ -2384,20 +2384,17 @@ lpfc_sli4_bsg_link_diag_test(struct bsg_job *job)
goto job_error;
pmboxq = mempool_alloc(phba->mbox_mem_pool, GFP_KERNEL);
- if (!pmboxq) {
- rc = -ENOMEM;
+ if (!pmboxq)
goto link_diag_test_exit;
- }
req_len = (sizeof(struct lpfc_mbx_set_link_diag_state) -
sizeof(struct lpfc_sli4_cfg_mhdr));
alloc_len = lpfc_sli4_config(phba, pmboxq, LPFC_MBOX_SUBSYSTEM_FCOE,
LPFC_MBOX_OPCODE_FCOE_LINK_DIAG_STATE,
req_len, LPFC_SLI4_MBX_EMBED);
- if (alloc_len != req_len) {
- rc = -ENOMEM;
+ if (alloc_len != req_len)
goto link_diag_test_exit;
- }
+
run_link_diag_test = &pmboxq->u.mqe.un.link_diag_test;
bf_set(lpfc_mbx_run_diag_test_link_num, &run_link_diag_test->u.req,
phba->sli4_hba.lnk_info.lnk_no);
--
2.5.0
Re: [PATCH] sg: recheck MMAP_IO request length with lock held
On 2017-08-16 12:48 AM, Todd Poynor wrote:
Commit 1bc0eb044615 ("scsi: sg: protect accesses to 'reserved' page
array") adds needed concurrency protection for the "reserve" buffer.
Some checks that are initially made outside the lock are replicated once
the lock is taken to ensure the checks and resulting decisions are made
using consistent state.
The check that a request with flag SG_FLAG_MMAP_IO set fits in the
reserve buffer also needs to be performed again under the lock to
ensure the reserve buffer length compared against matches the value in
effect when the request is linked to the reserve buffer. An -ENOMEM
should be returned in this case, instead of switching over to an
indirect buffer as for non-MMAP_IO requests.
Signed-off-by: Todd Poynor
Acked-by: Douglas Gilbert
Thanks.
---
drivers/scsi/sg.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d7ff71e0c85c..3a44b4bc872b 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1735,9 +1735,12 @@ sg_start_req(Sg_request *srp, unsigned char *cmd)
!sfp->res_in_use) {
sfp->res_in_use = 1;
sg_link_reserve(sfp, srp, dxfer_len);
- } else if ((hp->flags & SG_FLAG_MMAP_IO) && sfp->res_in_use) {
+ } else if (hp->flags & SG_FLAG_MMAP_IO) {
+ res = -EBUSY; /* sfp->res_in_use == 1 */
+ if (dxfer_len > rsv_schp->bufflen)
+ res = -ENOMEM;
mutex_unlock(&sfp->f_mutex);
- return -EBUSY;
+ return res;
} else {
res = sg_build_indirect(req_schp, sfp, dxfer_len);
if (res) {
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> ==
> WARNING: possible circular locking dependency detected
> 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> --
> fsck.ext4/148 is trying to acquire lock:
> (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
>
> but now in release context of a crosslock acquired at the following:
> ((complete)&wait#2){+.+.}, at: [] blk_execute_rq+0xbb/0xda
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 ((complete)&wait#2){+.+.}:
>lock_acquire+0x176/0x19e
>__wait_for_common+0x50/0x1e3
>blk_execute_rq+0xbb/0xda
>scsi_execute+0xc3/0x17d [scsi_mod]
>sd_revalidate_disk+0x112/0x1549 [sd_mod]
>rescan_partitions+0x48/0x2c4
>__blkdev_get+0x14b/0x37c
>blkdev_get+0x191/0x2c0
>device_add_disk+0x2b4/0x3e5
>sd_probe_async+0xf8/0x17e [sd_mod]
>async_run_entry_fn+0x34/0xe0
>process_one_work+0x2af/0x4d1
>worker_thread+0x19a/0x24f
>kthread+0x133/0x13b
>ret_from_fork+0x27/0x40
>
> -> #0 (&bdev->bd_mutex){+.+.}:
>__blkdev_put+0x33/0x190
>blkdev_close+0x24/0x27
>__fput+0xee/0x18a
>task_work_run+0x79/0xa0
>prepare_exit_to_usermode+0x9b/0xb5
>
> other info that might help us debug this:
> Possible unsafe locking scenario by crosslock:
>CPU0CPU1
>
> lock(&bdev->bd_mutex);
> lock((complete)&wait#2);
>lock(&bdev->bd_mutex);
>unlock((complete)&wait#2);
>
> *** DEADLOCK ***
> 4 locks held by fsck.ext4/148:
> #0: (&bdev->bd_mutex){+.+.}, at: []
> __blkdev_put+0x33/0x190
> #1: (rcu_read_lock){}, at: []
> rcu_lock_acquire+0x0/0x20
> #2: (&(&host->lock)->rlock){-.-.}, at: []
> ata_scsi_queuecmd+0x23/0x74 [libata]
> #3: (&x->wait#14){-...}, at: [] complete+0x18/0x50
>
> stack backtrace:
> CPU: 1 PID: 148 Comm: fsck.ext4 Not tainted
> 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746
> Call Trace:
> dump_stack+0x67/0x8e
> print_circular_bug+0x2a1/0x2af
> ? zap_class+0xc5/0xc5
> check_prev_add+0x76/0x20d
> ? __lock_acquire+0xc27/0xcc8
> lock_commit_crosslock+0x327/0x35e
> complete+0x24/0x50
> scsi_end_request+0x8d/0x176 [scsi_mod]
> scsi_io_completion+0x1be/0x423 [scsi_mod]
> __blk_mq_complete_request+0x112/0x131
> ata_scsi_simulate+0x212/0x218 [libata]
> __ata_scsi_queuecmd+0x1be/0x1de [libata]
> ata_scsi_queuecmd+0x41/0x74 [libata]
> scsi_dispatch_cmd+0x194/0x2af [scsi_mod]
> scsi_queue_rq+0x1e0/0x26f [scsi_mod]
> blk_mq_dispatch_rq_list+0x193/0x2a7
> ? _raw_spin_unlock+0x2e/0x40
> blk_mq_sched_dispatch_requests+0x132/0x176
> __blk_mq_run_hw_queue+0x59/0xc5
> __blk_mq_delay_run_hw_queue+0x5f/0xc1
> blk_mq_flush_plug_list+0xfc/0x10b
> blk_flush_plug_list+0xc6/0x1eb
> blk_finish_plug+0x25/0x32
> generic_writepages+0x56/0x63
> do_writepages+0x36/0x70
> __filemap_fdatawrite_range+0x59/0x5f
> filemap_write_and_wait+0x19/0x4f
> __blkdev_put+0x5f/0x190
> blkdev_close+0x24/0x27
> __fput+0xee/0x18a
> task_work_run+0x79/0xa0
> prepare_exit_to_usermode+0x9b/0xb5
> entry_SYSCALL_64_fastpath+0xab/0xad
Byungchul, did you add the crosslock checks to lockdep? Can you have a look at
the above report? That report namely doesn't make sense to me.
Bart.
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > ==
> > WARNING: possible circular locking dependency detected
> > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > --
> > fsck.ext4/148 is trying to acquire lock:
> > (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> >
> > but now in release context of a crosslock acquired at the following:
> > ((complete)&wait#2){+.+.}, at: []
> > blk_execute_rq+0xbb/0xda
> >
> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 ((complete)&wait#2){+.+.}:
> >lock_acquire+0x176/0x19e
> >__wait_for_common+0x50/0x1e3
> >blk_execute_rq+0xbb/0xda
> >scsi_execute+0xc3/0x17d [scsi_mod]
> >sd_revalidate_disk+0x112/0x1549 [sd_mod]
> >rescan_partitions+0x48/0x2c4
> >__blkdev_get+0x14b/0x37c
> >blkdev_get+0x191/0x2c0
> >device_add_disk+0x2b4/0x3e5
> >sd_probe_async+0xf8/0x17e [sd_mod]
> >async_run_entry_fn+0x34/0xe0
> >process_one_work+0x2af/0x4d1
> >worker_thread+0x19a/0x24f
> >kthread+0x133/0x13b
> >ret_from_fork+0x27/0x40
> >
> > -> #0 (&bdev->bd_mutex){+.+.}:
> >__blkdev_put+0x33/0x190
> >blkdev_close+0x24/0x27
> >__fput+0xee/0x18a
> >task_work_run+0x79/0xa0
> >prepare_exit_to_usermode+0x9b/0xb5
> >
> > other info that might help us debug this:
> > Possible unsafe locking scenario by crosslock:
> >CPU0CPU1
> >
> > lock(&bdev->bd_mutex);
> > lock((complete)&wait#2);
> >lock(&bdev->bd_mutex);
> >unlock((complete)&wait#2);
> >
> > *** DEADLOCK ***
> > 4 locks held by fsck.ext4/148:
> > #0: (&bdev->bd_mutex){+.+.}, at: []
> > __blkdev_put+0x33/0x190
> > #1: (rcu_read_lock){}, at: []
> > rcu_lock_acquire+0x0/0x20
> > #2: (&(&host->lock)->rlock){-.-.}, at: []
> > ata_scsi_queuecmd+0x23/0x74 [libata]
> > #3: (&x->wait#14){-...}, at: [] complete+0x18/0x50
> >
> > stack backtrace:
> > CPU: 1 PID: 148 Comm: fsck.ext4 Not tainted
> > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746
> > Call Trace:
> > dump_stack+0x67/0x8e
> > print_circular_bug+0x2a1/0x2af
> > ? zap_class+0xc5/0xc5
> > check_prev_add+0x76/0x20d
> > ? __lock_acquire+0xc27/0xcc8
> > lock_commit_crosslock+0x327/0x35e
> > complete+0x24/0x50
> > scsi_end_request+0x8d/0x176 [scsi_mod]
> > scsi_io_completion+0x1be/0x423 [scsi_mod]
> > __blk_mq_complete_request+0x112/0x131
> > ata_scsi_simulate+0x212/0x218 [libata]
> > __ata_scsi_queuecmd+0x1be/0x1de [libata]
> > ata_scsi_queuecmd+0x41/0x74 [libata]
> > scsi_dispatch_cmd+0x194/0x2af [scsi_mod]
> > scsi_queue_rq+0x1e0/0x26f [scsi_mod]
> > blk_mq_dispatch_rq_list+0x193/0x2a7
> > ? _raw_spin_unlock+0x2e/0x40
> > blk_mq_sched_dispatch_requests+0x132/0x176
> > __blk_mq_run_hw_queue+0x59/0xc5
> > __blk_mq_delay_run_hw_queue+0x5f/0xc1
> > blk_mq_flush_plug_list+0xfc/0x10b
> > blk_flush_plug_list+0xc6/0x1eb
> > blk_finish_plug+0x25/0x32
> > generic_writepages+0x56/0x63
> > do_writepages+0x36/0x70
> > __filemap_fdatawrite_range+0x59/0x5f
> > filemap_write_and_wait+0x19/0x4f
> > __blkdev_put+0x5f/0x190
> > blkdev_close+0x24/0x27
> > __fput+0xee/0x18a
> > task_work_run+0x79/0xa0
> > prepare_exit_to_usermode+0x9b/0xb5
> > entry_SYSCALL_64_fastpath+0xab/0xad
>
> Byungchul, did you add the crosslock checks to lockdep? Can you have a look at
> the above report? That report namely doesn't make sense to me.
The report is talking about the following lockup:
A work in a worker A task work on exit to user
-- ---
mutex_lock(&bdev->bd_mutex)
mutext_lock(&bdev->bd_mutex)
blk_execute_rq()
wait_for_completion_io_timeout(&A)
complete(&A)
Is this impossible?
To Peterz,
Anyway I wanted to avoid lockdep reports in the case using a timeout
interface. Do you think it's still worth reporting the kind of lockup?
I'm ok if you do.
Re: [PATCH] sg: recheck MMAP_IO request length with lock held
Todd,
> Commit 1bc0eb044615 ("scsi: sg: protect accesses to 'reserved' page
> array") adds needed concurrency protection for the "reserve" buffer.
> Some checks that are initially made outside the lock are replicated once
> the lock is taken to ensure the checks and resulting decisions are made
> using consistent state.
>
> The check that a request with flag SG_FLAG_MMAP_IO set fits in the
> reserve buffer also needs to be performed again under the lock to
> ensure the reserve buffer length compared against matches the value in
> effect when the request is linked to the reserve buffer. An -ENOMEM
> should be returned in this case, instead of switching over to an
> indirect buffer as for non-MMAP_IO requests.
Applied to 4.14/scsi-queue, thank you!
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] sg: protect against races between mmap() and SG_SET_RESERVED_SIZE
On 2017-08-16 01:41 AM, Todd Poynor wrote:
Take f_mutex around mmap() processing to protect against races with
the SG_SET_RESERVED_SIZE ioctl. Ensure the reserve buffer length
remains consistent during the mapping operation, and set the
"mmap called" flag to prevent further changes to the reserved buffer
size as an atomic operation with the mapping.
Signed-off-by: Todd Poynor
Acked-by: Douglas Gilbert
Thanks.
---
drivers/scsi/sg.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 3a44b4bc872b..a20718e9f1f4 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1233,6 +1233,7 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
unsigned long req_sz, len, sa;
Sg_scatter_hold *rsv_schp;
int k, length;
+int ret = 0;
if ((!filp) || (!vma) || (!(sfp = (Sg_fd *) filp->private_data)))
return -ENXIO;
@@ -1243,8 +1244,11 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
if (vma->vm_pgoff)
return -EINVAL; /* want no offset */
rsv_schp = &sfp->reserve;
- if (req_sz > rsv_schp->bufflen)
- return -ENOMEM; /* cannot map more than reserved buffer */
+ mutex_lock(&sfp->f_mutex);
+ if (req_sz > rsv_schp->bufflen) {
+ ret = -ENOMEM; /* cannot map more than reserved buffer */
+ goto out;
+ }
sa = vma->vm_start;
length = 1 << (PAGE_SHIFT + rsv_schp->page_order);
@@ -1258,7 +1262,9 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
vma->vm_private_data = sfp;
vma->vm_ops = &sg_mmap_vm_ops;
- return 0;
+out:
+ mutex_unlock(&sfp->f_mutex);
+ return ret;
}
static void
Re: [PATCH] scsi: sg: off by one in sg_ioctl()
On 2017-08-17 03:09 AM, Dan Carpenter wrote:
If "val" is SG_MAX_QUEUE then we are one element beyond the end of the
"rinfo" array so the > should be >=.
Fixes: 109bade9c625 ("scsi: sg: use standard lists for sg_requests")
Signed-off-by: Dan Carpenter
Acked-by: Douglas Gilbert
Thanks.
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d7ff71e0c85c..84e782d8e7c3 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1021,7 +1021,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned
long arg)
read_lock_irqsave(&sfp->rq_list_lock, iflags);
val = 0;
list_for_each_entry(srp, &sfp->rq_list, entry) {
- if (val > SG_MAX_QUEUE)
+ if (val >= SG_MAX_QUEUE)
break;
memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
rinfo[val].req_state = srp->done + 1;
Re: [PATCH v4 00/14] mpt3sas driver NVMe support:
Suganath, > mpt3sas: SGL to PRP Translation for I/Os to NVMe devices I'm still confused about this patch. - I don't understand why you go through all these hoops to decide whether to use PRPs or IEEE scatterlists. If the firmware translation is slow, why even bother with the SG format in the first place? Set the max I/O size to match MDTS and you're done. - What's the benefit of using SG for regular I/O commands? - If the unmap translation in firmware is slow, why don't you translate WRITE SAME/w UNMAP set to DSM DEALLOCATE without requiring applications to do encapsulated passthrough? Also make sure you attribute your patches correctly (From: root ). And you don't need that long CC: list. Just send the patch series to linux-scsi@vger.kernel.org. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] sg: protect against races between mmap() and SG_SET_RESERVED_SIZE
Todd, > Take f_mutex around mmap() processing to protect against races with > the SG_SET_RESERVED_SIZE ioctl. Ensure the reserve buffer length > remains consistent during the mapping operation, and set the > "mmap called" flag to prevent further changes to the reserved buffer > size as an atomic operation with the mapping. Applied to 4.14/scsi-queue (with a slight whitespace fix). Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: sg: off by one in sg_ioctl()
Dan, > If "val" is SG_MAX_QUEUE then we are one element beyond the end of the > "rinfo" array so the > should be >=. Applied to 4.13/scsi-fixes. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On (08/23/17 09:03), Byungchul Park wrote: [..] aha, ok > The report is talking about the following lockup: > > A work in a worker A task work on exit to user > -- --- > mutex_lock(&bdev->bd_mutex) >mutext_lock(&bdev->bd_mutex) > blk_execute_rq() >wait_for_completion_io_timeout(&A) >complete(&A) > > Is this impossible? I was really confused how this "unlock" may lead to a deadlock > > > other info that might help us debug this: > > > Possible unsafe locking scenario by crosslock: > > >CPU0CPU1 > > > > > > lock(&bdev->bd_mutex); > > > lock((complete)&wait#2); > > >lock(&bdev->bd_mutex); > > >unlock((complete)&wait#2); any chance the report can be improved? mention timeout, etc? // well, if this functionality will stay. p.s. Bart Van Assche, thanks for Cc-ing Park Byungchul, I was really sure I didn't enabled the cross-release, but apparently I was wrong: CONFIG_LOCKDEP_CROSSRELEASE=y CONFIG_LOCKDEP_COMPLETIONS=y -ss
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Wed, Aug 23, 2017 at 11:36:49AM +0900, Sergey Senozhatsky wrote: > On (08/23/17 09:03), Byungchul Park wrote: > [..] > > aha, ok > > > The report is talking about the following lockup: > > > > A work in a worker A task work on exit to user > > -- --- > > mutex_lock(&bdev->bd_mutex) > >mutext_lock(&bdev->bd_mutex) > > blk_execute_rq() > >wait_for_completion_io_timeout(&A) > >complete(&A) > > > > Is this impossible? > > I was really confused how this "unlock" may lead to a deadlock Hi Sergey, Right. It should be enhanced. > > > > > other info that might help us debug this: > > > > Possible unsafe locking scenario by crosslock: > > > >CPU0CPU1 > > > > > > > > lock(&bdev->bd_mutex); > > > > lock((complete)&wait#2); > > > >lock(&bdev->bd_mutex); > > > >unlock((complete)&wait#2); > > > any chance the report can be improved? mention timeout, etc? > // well, if this functionality will stay. > > > p.s. > Bart Van Assche, thanks for Cc-ing Park Byungchul, I was really > sure I didn't enabled the cross-release, but apparently I was wrong: > CONFIG_LOCKDEP_CROSSRELEASE=y > CONFIG_LOCKDEP_COMPLETIONS=y > > -ss
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
Hi Byungchul,
On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > ==
> > > WARNING: possible circular locking dependency detected
> > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > > --
> > > fsck.ext4/148 is trying to acquire lock:
> > > (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> > >
> > > but now in release context of a crosslock acquired at the following:
> > > ((complete)&wait#2){+.+.}, at: []
> > > blk_execute_rq+0xbb/0xda
> > >
> > > which lock already depends on the new lock.
> > >
I felt this message really misleading, because the deadlock is detected
at the commit time of "((complete)&wait#2)" rather than the acquisition
time of "(&bdev->bd_mutex)", so I made the following improvement.
Thoughts?
Regards,
Boqun
--->8
From: Boqun Feng
Date: Wed, 23 Aug 2017 10:18:30 +0800
Subject: [PATCH] lockdep: Improve the readibility of crossrelease related
splats
When a crossrelease related deadlock is detected in a commit, the
current implemention makes splats like:
> fsck.ext4/148 is trying to acquire lock:
> (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
>
> but now in release context of a crosslock acquired at the following:
> ((complete)&wait#2){+.+.}, at: [] blk_execute_rq+0xbb/0xda
>
> which lock already depends on the new lock.
> ...
However, it could be misleading because the current task has got the
lock already, and in fact the deadlock is detected when it is doing the
commit of the crossrelease lock. So make the splats more accurate to
describe the deadlock case.
Signed-off-by: Boqun Feng
---
kernel/locking/lockdep.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 66011c9f5df3..642fb5362507 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1195,17 +1195,23 @@ print_circular_bug_header(struct lock_list *entry,
unsigned int depth,
pr_warn("WARNING: possible circular locking dependency detected\n");
print_kernel_ident();
pr_warn("--\n");
- pr_warn("%s/%d is trying to acquire lock:\n",
- curr->comm, task_pid_nr(curr));
- print_lock(check_src);
- if (cross_lock(check_tgt->instance))
- pr_warn("\nbut now in release context of a crosslock acquired
at the following:\n");
- else
+ if (cross_lock(check_tgt->instance)) {
+ pr_warn("%s/%d is committing a crossrelease lock:\n",
+ curr->comm, task_pid_nr(curr));
+ print_lock(check_tgt);
+ pr_warn("\n, with the following lock held:\n");
+ print_lock(check_src);
+ pr_warn("\non which lock the crossrelease lock already
depends.\n\n");
+ } else {
+ pr_warn("%s/%d is trying to acquire lock:\n",
+ curr->comm, task_pid_nr(curr));
+ print_lock(check_src);
pr_warn("\nbut task is already holding lock:\n");
+ print_lock(check_tgt);
+ pr_warn("\nwhich lock already depends on the new lock.\n\n");
+ }
- print_lock(check_tgt);
- pr_warn("\nwhich lock already depends on the new lock.\n\n");
pr_warn("\nthe existing dependency chain (in reverse order) is:\n");
print_circular_bug_entry(entry, depth);
--
2.14.1
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Wed, Aug 23, 2017 at 11:49:51AM +0800, Boqun Feng wrote:
> Hi Byungchul,
>
> On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> > On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > > ==
> > > > WARNING: possible circular locking dependency detected
> > > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > > > --
> > > > fsck.ext4/148 is trying to acquire lock:
> > > > (&bdev->bd_mutex){+.+.}, at: []
> > > > __blkdev_put+0x33/0x190
> > > >
> > > > but now in release context of a crosslock acquired at the following:
> > > > ((complete)&wait#2){+.+.}, at: []
> > > > blk_execute_rq+0xbb/0xda
> > > >
> > > > which lock already depends on the new lock.
> > > >
>
> I felt this message really misleading, because the deadlock is detected
> at the commit time of "((complete)&wait#2)" rather than the acquisition
> time of "(&bdev->bd_mutex)", so I made the following improvement.
>
> Thoughts?
>
> Regards,
> Boqun
>
While I'm on this one, I think we should also add a case in @check_src
is a cross lock, i.e. we detect cross deadlock at the acquisition time
of the cross lock. How about the following?
Regards,
Boqun
--->8
From: Boqun Feng
Date: Wed, 23 Aug 2017 12:12:16 +0800
Subject: [PATCH] lockdep: Print proper scenario if cross deadlock detected at
acquisition time
For a potential deadlock about CROSSRELEASE as follow:
P1 P2
=== =
lock(A)
lock(X)
lock(A)
commit(X)
A: normal lock, X: cross lock
, we could detect it at two places:
1. commit time:
We have run P1 first, and have dependency A --> X in graph, and
then we run P2, and find the deadlock.
2. acquisition time:
We have run P2 first, and have dependency A --> X, in
graph(because another P3 may run previously and is acquiring for
lock X), and then we run P1 and find the deadlock.
In current print_circular_lock_scenario(), for 1) we could print the
right scenario and note that's a deadlock related to CROSSRELEASE,
however for 2) we print the scenario as a normal lockdep deadlock.
It's better to print a proper scenario related to CROSSRELEASE to help
users find their bugs more easily, so improve this.
Signed-off-by: Boqun Feng
---
kernel/locking/lockdep.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 642fb5362507..a3709e15f609 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
__print_lock_name(target);
printk(KERN_CONT ");\n");
printk("\n *** DEADLOCK ***\n\n");
+ } else if (cross_lock(src->instance)) {
+ printk(" Possible unsafe locking scenario by crosslock:\n\n");
+ printk(" CPU0CPU1\n");
+ printk(" \n");
+ printk(" lock(");
+ __print_lock_name(target);
+ printk(KERN_CONT ");\n");
+ printk(" lock(");
+ __print_lock_name(source);
+ printk(KERN_CONT ");\n");
+ printk(" lock(");
+ __print_lock_name(parent == source ? target : parent);
+ printk(KERN_CONT ");\n");
+ printk(" unlock(");
+ __print_lock_name(source);
+ printk(KERN_CONT ");\n");
+ printk("\n *** DEADLOCK ***\n\n");
} else {
printk(" Possible unsafe locking scenario:\n\n");
printk(" CPU0CPU1\n");
--
2.14.1
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Wed, Aug 23, 2017 at 11:49:51AM +0800, Boqun Feng wrote:
> Hi Byungchul,
>
> On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> > On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > > ==
> > > > WARNING: possible circular locking dependency detected
> > > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > > > --
> > > > fsck.ext4/148 is trying to acquire lock:
> > > > (&bdev->bd_mutex){+.+.}, at: []
> > > > __blkdev_put+0x33/0x190
> > > >
> > > > but now in release context of a crosslock acquired at the following:
> > > > ((complete)&wait#2){+.+.}, at: []
> > > > blk_execute_rq+0xbb/0xda
> > > >
> > > > which lock already depends on the new lock.
> > > >
>
> I felt this message really misleading, because the deadlock is detected
> at the commit time of "((complete)&wait#2)" rather than the acquisition
> time of "(&bdev->bd_mutex)", so I made the following improvement.
>
> Thoughts?
>
> Regards,
> Boqun
>
> --->8
> From: Boqun Feng
> Date: Wed, 23 Aug 2017 10:18:30 +0800
> Subject: [PATCH] lockdep: Improve the readibility of crossrelease related
> splats
>
> When a crossrelease related deadlock is detected in a commit, the
> current implemention makes splats like:
>
> > fsck.ext4/148 is trying to acquire lock:
> > (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> >
> > but now in release context of a crosslock acquired at the following:
> > ((complete)&wait#2){+.+.}, at: []
> > blk_execute_rq+0xbb/0xda
> >
> > which lock already depends on the new lock.
> > ...
>
> However, it could be misleading because the current task has got the
> lock already, and in fact the deadlock is detected when it is doing the
> commit of the crossrelease lock. So make the splats more accurate to
> describe the deadlock case.
>
> Signed-off-by: Boqun Feng
> ---
> kernel/locking/lockdep.c | 22 ++
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 66011c9f5df3..642fb5362507 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1195,17 +1195,23 @@ print_circular_bug_header(struct lock_list *entry,
> unsigned int depth,
> pr_warn("WARNING: possible circular locking dependency detected\n");
> print_kernel_ident();
> pr_warn("--\n");
> - pr_warn("%s/%d is trying to acquire lock:\n",
> - curr->comm, task_pid_nr(curr));
> - print_lock(check_src);
>
> - if (cross_lock(check_tgt->instance))
> - pr_warn("\nbut now in release context of a crosslock acquired
> at the following:\n");
> - else
> + if (cross_lock(check_tgt->instance)) {
> + pr_warn("%s/%d is committing a crossrelease lock:\n",
> + curr->comm, task_pid_nr(curr));
I think it would be better to print something in term of acquisition,
since the following print_lock() will print infromation of acquisition.
> + print_lock(check_tgt);
> + pr_warn("\n, with the following lock held:\n");
The lock does not have to be held at the commit.
> + print_lock(check_src);
> + pr_warn("\non which lock the crossrelease lock already
> depends.\n\n");
> + } else {
> + pr_warn("%s/%d is trying to acquire lock:\n",
> + curr->comm, task_pid_nr(curr));
> + print_lock(check_src);
> pr_warn("\nbut task is already holding lock:\n");
> + print_lock(check_tgt);
> + pr_warn("\nwhich lock already depends on the new lock.\n\n");
> + }
>
> - print_lock(check_tgt);
> - pr_warn("\nwhich lock already depends on the new lock.\n\n");
> pr_warn("\nthe existing dependency chain (in reverse order) is:\n");
>
> print_circular_bug_entry(entry, depth);
> --
> 2.14.1
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On (08/23/17 12:38), Boqun Feng wrote:
[..]
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 642fb5362507..a3709e15f609 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
> __print_lock_name(target);
> printk(KERN_CONT ");\n");
KERN_CONT and "\n" should not be together. "\n" flushes the cont
buffer immediately.
-ss
> printk("\n *** DEADLOCK ***\n\n");
> + } else if (cross_lock(src->instance)) {
> + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> + printk(" CPU0CPU1\n");
> + printk(" \n");
> + printk(" lock(");
> + __print_lock_name(target);
> + printk(KERN_CONT ");\n");
> + printk(" lock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk(" lock(");
> + __print_lock_name(parent == source ? target : parent);
> + printk(KERN_CONT ");\n");
> + printk(" unlock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk("\n *** DEADLOCK ***\n\n");
> } else {
> printk(" Possible unsafe locking scenario:\n\n");
> printk(" CPU0CPU1\n");
> --
> 2.14.1
>
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Wed, Aug 23, 2017 at 01:46:17PM +0900, Byungchul Park wrote:
> On Wed, Aug 23, 2017 at 11:49:51AM +0800, Boqun Feng wrote:
> > Hi Byungchul,
> >
> > On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> > > On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > > > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > > > ==
> > > > > WARNING: possible circular locking dependency detected
> > > > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not
> > > > > tainted
> > > > > --
> > > > > fsck.ext4/148 is trying to acquire lock:
> > > > > (&bdev->bd_mutex){+.+.}, at: []
> > > > > __blkdev_put+0x33/0x190
> > > > >
> > > > > but now in release context of a crosslock acquired at the following:
> > > > > ((complete)&wait#2){+.+.}, at: []
> > > > > blk_execute_rq+0xbb/0xda
> > > > >
> > > > > which lock already depends on the new lock.
> > > > >
> >
> > I felt this message really misleading, because the deadlock is detected
> > at the commit time of "((complete)&wait#2)" rather than the acquisition
> > time of "(&bdev->bd_mutex)", so I made the following improvement.
> >
> > Thoughts?
> >
> > Regards,
> > Boqun
> >
> > --->8
> > From: Boqun Feng
> > Date: Wed, 23 Aug 2017 10:18:30 +0800
> > Subject: [PATCH] lockdep: Improve the readibility of crossrelease related
> > splats
> >
> > When a crossrelease related deadlock is detected in a commit, the
> > current implemention makes splats like:
> >
> > > fsck.ext4/148 is trying to acquire lock:
> > > (&bdev->bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> > >
> > > but now in release context of a crosslock acquired at the following:
> > > ((complete)&wait#2){+.+.}, at: []
> > > blk_execute_rq+0xbb/0xda
> > >
> > > which lock already depends on the new lock.
> > > ...
> >
> > However, it could be misleading because the current task has got the
> > lock already, and in fact the deadlock is detected when it is doing the
> > commit of the crossrelease lock. So make the splats more accurate to
> > describe the deadlock case.
> >
> > Signed-off-by: Boqun Feng
> > ---
> > kernel/locking/lockdep.c | 22 ++
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 66011c9f5df3..642fb5362507 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1195,17 +1195,23 @@ print_circular_bug_header(struct lock_list *entry,
> > unsigned int depth,
> > pr_warn("WARNING: possible circular locking dependency detected\n");
> > print_kernel_ident();
> > pr_warn("--\n");
> > - pr_warn("%s/%d is trying to acquire lock:\n",
> > - curr->comm, task_pid_nr(curr));
> > - print_lock(check_src);
> >
> > - if (cross_lock(check_tgt->instance))
> > - pr_warn("\nbut now in release context of a crosslock acquired
> > at the following:\n");
> > - else
> > + if (cross_lock(check_tgt->instance)) {
> > + pr_warn("%s/%d is committing a crossrelease lock:\n",
> > + curr->comm, task_pid_nr(curr));
>
> I think it would be better to print something in term of acquisition,
> since the following print_lock() will print infromation of acquisition.
>
Well, that print_lock() will print the cross lock acquisition
information at other contexts, but the current thread is doing the
commit. So I think the information would be a little misleading. I will
add "aacquired at" to indicate the lock information is for acquisition.
> > + print_lock(check_tgt);
> > + pr_warn("\n, with the following lock held:\n");
>
> The lock does not have to be held at the commit.
>
Ah.. right.
How about this:
pr_warn("%s/%d is committing a crossrelease lock acquired
at:\n",
curr->comm, task_pid_nr(curr));
print_lock(check_tgt);
pr_warn("\n, after having the followin
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Wed, Aug 23, 2017 at 01:46:48PM +0900, Sergey Senozhatsky wrote:
> On (08/23/17 12:38), Boqun Feng wrote:
> [..]
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 642fb5362507..a3709e15f609 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
> > __print_lock_name(target);
> > printk(KERN_CONT ");\n");
>
> KERN_CONT and "\n" should not be together. "\n" flushes the cont
> buffer immediately.
>
Hmm.. Not quite familiar with printk() stuffs, but I could see several
usages of printk(KERN_CONT "...\n") in kernel.
Did a bit research myself, and I now think the inappropriate use is to
use a KERN_CONT printk *after* another printk ending with a "\n". Am I
missing some recent changes or rules of KERN_CONT?
Regards,
Boqun
> -ss
>
> > printk("\n *** DEADLOCK ***\n\n");
> > + } else if (cross_lock(src->instance)) {
> > + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> > + printk(" CPU0CPU1\n");
> > + printk(" \n");
> > + printk(" lock(");
> > + __print_lock_name(target);
> > + printk(KERN_CONT ");\n");
> > + printk(" lock(");
> > + __print_lock_name(source);
> > + printk(KERN_CONT ");\n");
> > + printk(" lock(");
> > + __print_lock_name(parent == source ? target : parent);
> > + printk(KERN_CONT ");\n");
> > + printk(" unlock(");
> > + __print_lock_name(source);
> > + printk(KERN_CONT ");\n");
> > + printk("\n *** DEADLOCK ***\n\n");
> > } else {
> > printk(" Possible unsafe locking scenario:\n\n");
> > printk(" CPU0CPU1\n");
> > --
> > 2.14.1
> >
signature.asc
Description: PGP signature
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On (08/23/17 13:35), Boqun Feng wrote: [..] > > > printk(KERN_CONT ");\n"); > > > > KERN_CONT and "\n" should not be together. "\n" flushes the cont > > buffer immediately. > > > > Hmm.. Not quite familiar with printk() stuffs, but I could see several > usages of printk(KERN_CONT "...\n") in kernel. > > Did a bit research myself, and I now think the inappropriate use is to > use a KERN_CONT printk *after* another printk ending with a "\n". Am I > missing some recent changes or rules of KERN_CONT? has been this way for quite some time (if not always). LOG_NEWLINE results in cont_flush(), which log_store() the content of KERN_CONT buffer. if we see that supplied message has no \n then we store it in a dedicated buffer (cont buffer) if (!(lflags & LOG_NEWLINE)) return cont_add(); return log_store(); we flush that buffer (move its content to the kernel log buffer) when we receive a message with a \n or when printk() from another task/context interrupts the current cont line and, thus, forces us to flush. -ss
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On Wed, Aug 23, 2017 at 12:38:13PM +0800, Boqun Feng wrote:
> From: Boqun Feng
> Date: Wed, 23 Aug 2017 12:12:16 +0800
> Subject: [PATCH] lockdep: Print proper scenario if cross deadlock detected at
> acquisition time
>
> For a potential deadlock about CROSSRELEASE as follow:
>
> P1 P2
> === =
> lock(A)
> lock(X)
> lock(A)
> commit(X)
>
> A: normal lock, X: cross lock
>
> , we could detect it at two places:
>
> 1. commit time:
>
> We have run P1 first, and have dependency A --> X in graph, and
> then we run P2, and find the deadlock.
>
> 2. acquisition time:
>
> We have run P2 first, and have dependency A --> X, in
X -> A
> graph(because another P3 may run previously and is acquiring for
".. another P3 may have run previously and was holding .."
^
Additionally, not only P3 but also P2 like:
lock(A)
lock(X)
lock(X) // I mean it's at _P2_
lock(A)
commit(X)
> lock X), and then we run P1 and find the deadlock.
>
> In current print_circular_lock_scenario(), for 1) we could print the
> right scenario and note that's a deadlock related to CROSSRELEASE,
> however for 2) we print the scenario as a normal lockdep deadlock.
>
> It's better to print a proper scenario related to CROSSRELEASE to help
> users find their bugs more easily, so improve this.
>
> Signed-off-by: Boqun Feng
> ---
> kernel/locking/lockdep.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 642fb5362507..a3709e15f609 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
> __print_lock_name(target);
> printk(KERN_CONT ");\n");
> printk("\n *** DEADLOCK ***\n\n");
> + } else if (cross_lock(src->instance)) {
> + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> + printk(" CPU0CPU1\n");
> + printk(" \n");
> + printk(" lock(");
> + __print_lock_name(target);
> + printk(KERN_CONT ");\n");
> + printk(" lock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk(" lock(");
> + __print_lock_name(parent == source ? target : parent);
> + printk(KERN_CONT ");\n");
> + printk(" unlock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk("\n *** DEADLOCK ***\n\n");
> } else {
> printk(" Possible unsafe locking scenario:\n\n");
> printk(" CPU0CPU1\n");
I need time to be sure if it's correct.
Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]
On (08/23/17 13:35), Boqun Feng wrote:
> > KERN_CONT and "\n" should not be together. "\n" flushes the cont
> > buffer immediately.
> >
>
> Hmm.. Not quite familiar with printk() stuffs, but I could see several
> usages of printk(KERN_CONT "...\n") in kernel.
>
> Did a bit research myself, and I now think the inappropriate use is to
> use a KERN_CONT printk *after* another printk ending with a "\n".
ah... I didn't check __print_lock_name(): it leaves unflushed cont buffer
upon the return. sorry, your code is correct.
-ss
> > > printk("\n *** DEADLOCK ***\n\n");
> > > + } else if (cross_lock(src->instance)) {
> > > + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> > > + printk(" CPU0CPU1\n");
> > > + printk(" \n");
> > > + printk(" lock(");
> > > + __print_lock_name(target);
> > > + printk(KERN_CONT ");\n");
> > > + printk(" lock(");
> > > + __print_lock_name(source);
> > > + printk(KERN_CONT ");\n");
> > > + printk(" lock(");
> > > + __print_lock_name(parent == source ? target : parent);
> > > + printk(KERN_CONT ");\n");
> > > + printk(" unlock(");
> > > + __print_lock_name(source);
> > > + printk(KERN_CONT ");\n");
> > > + printk("\n *** DEADLOCK ***\n\n");
> > > } else {
> > > printk(" Possible unsafe locking scenario:\n\n");
> > > printk(" CPU0CPU1\n");
> > > --
> > > 2.14.1
> > >
[GIT PULL] SCSI fixes for 4.13-rc6
Six minor and error leg fixes, plus one major change: the reversion of
scsi-mq as the default. We're doing the latter temporarily (with a
backport to stable) to give us time to fix all the issues that turned
up with this default before trying again.
The patch is available here:
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes
The short changelog is:
Christoph Hellwig (1):
Revert "scsi: default to scsi-mq"
Damien Le Moal (1):
scsi: sd_zbc: Write unlock zone from sd_uninit_cmnd()
Raghava Aditya Renukunta (1):
scsi: aacraid: Fix out of bounds in aac_get_name_resp
Varun Prakash (2):
scsi: cxgb4i: call neigh_event_send() to update MAC address
scsi: csiostor: fail probe if fw does not support FCoE
weiping zhang (1):
scsi: megaraid_sas: fix error handle in megasas_probe_one
And the diffstat:
drivers/scsi/Kconfig | 11 +++
drivers/scsi/aacraid/aachba.c | 9 +++--
drivers/scsi/aacraid/aacraid.h| 2 +-
drivers/scsi/csiostor/csio_hw.c | 4 +++-
drivers/scsi/csiostor/csio_init.c | 12
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c| 3 +++
drivers/scsi/megaraid/megaraid_sas_base.c | 2 +-
drivers/scsi/scsi.c | 4
drivers/scsi/sd.c | 3 +++
drivers/scsi/sd_zbc.c | 9 +
include/scsi/scsi_cmnd.h | 1 +
11 files changed, 47 insertions(+), 13 deletions(-)
With full diff below.
James
---
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index f4538d7a3016..d145e0d90227 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -47,6 +47,17 @@ config SCSI_NETLINK
default n
depends on NET
+config SCSI_MQ_DEFAULT
+ bool "SCSI: use blk-mq I/O path by default"
+ depends on SCSI
+ ---help---
+ This option enables the new blk-mq based I/O path for SCSI
+ devices by default. With the option the scsi_mod.use_blk_mq
+ module/boot option defaults to Y, without it to N, but it can
+ still be overridden either way.
+
+ If unsure say N.
+
config SCSI_PROC_FS
bool "legacy /proc/scsi/ support"
depends on SCSI && PROC_FS
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 4591113c49de..a1a2c71e1626 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -549,7 +549,9 @@ static void get_container_name_callback(void *context,
struct fib * fibptr)
if ((le32_to_cpu(get_name_reply->status) == CT_OK)
&& (get_name_reply->data[0] != '\0')) {
char *sp = get_name_reply->data;
- sp[sizeof(((struct aac_get_name_resp *)NULL)->data)] = '\0';
+ int data_size = FIELD_SIZEOF(struct aac_get_name_resp, data);
+
+ sp[data_size - 1] = '\0';
while (*sp == ' ')
++sp;
if (*sp) {
@@ -579,12 +581,15 @@ static void get_container_name_callback(void *context,
struct fib * fibptr)
static int aac_get_container_name(struct scsi_cmnd * scsicmd)
{
int status;
+ int data_size;
struct aac_get_name *dinfo;
struct fib * cmd_fibcontext;
struct aac_dev * dev;
dev = (struct aac_dev *)scsicmd->device->host->hostdata;
+ data_size = FIELD_SIZEOF(struct aac_get_name_resp, data);
+
cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
aac_fib_init(cmd_fibcontext);
@@ -593,7 +598,7 @@ static int aac_get_container_name(struct scsi_cmnd *
scsicmd)
dinfo->command = cpu_to_le32(VM_ContainerConfig);
dinfo->type = cpu_to_le32(CT_READ_NAME);
dinfo->cid = cpu_to_le32(scmd_id(scsicmd));
- dinfo->count = cpu_to_le32(sizeof(((struct aac_get_name_resp
*)NULL)->data));
+ dinfo->count = cpu_to_le32(data_size - 1);
status = aac_fib_send(ContainerCommand,
cmd_fibcontext,
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index d31a9bc2ba69..ee2667e20e42 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -2274,7 +2274,7 @@ struct aac_get_name_resp {
__le32 parm3;
__le32 parm4;
__le32 parm5;
- u8 data[16];
+ u8 data[17];
};
#define CT_CID_TO_32BITS_UID 165
diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
index 2029ad225121..5be0086142ca 100644
--- a/drivers/scsi/csiostor/csio_hw.c
+++ b/drivers/scsi/csiostor/csio_hw.c
@@ -3845,8 +3845,10 @@ csio_hw_start(struct csio_hw *hw)
if (csio_is_hw_ready(hw))
return 0;
- else
+ else if (csio_match_state(hw, csio_hws_uninit))
return -EINVAL;
+ else
+ return -ENODEV;
}
int
diff --git a/drivers/scsi/csiostor/csio_init.c
b/drivers/
Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)
Ok. If the stable maintainers are ok with your small fix I'm not going to complain too loudly. But I'm always worried about stable trees divering too much from mainline.
