* [PATCH] dm_create_session() calls create_proc_entry() with spinlock held
@ 2007-02-16 6:33 Kevin Jamieson
2007-02-19 0:30 ` Vlad Apostolov
0 siblings, 1 reply; 2+ messages in thread
From: Kevin Jamieson @ 2007-02-16 6:33 UTC (permalink / raw)
To: xfs
dm_create_session() calls create_proc_read_entry() with the dm_session_lock
spinlock held. But create_proc_entry() calls kmalloc(GFP_KERNEL), which can
sleep, so this isn't safe.
We have observed this to cause deadlocks with multiple processes creating/using
DMAPI sessions concurrently (this was with SLES10's 2.6.16.21-0.8 kernel, but
the DMAPI code in question is the same in CVS HEAD):
BUG: spinlock cpu recursion on CPU#1, a1/3235
lock: f930c49c, .magic: dead4ead, .owner: a2/22546, .owner_cpu: 1
Stack traceback for pid 6144
0xdfc98270 6144 2748 1 0 R 0xdfc98450 a1
EBP EIP Function (args)
0xf0f5be24 0xc010d89f delay_tsc+0xc (0xf0f5be44)
0xf0f5be2c 0xc01cd928 __delay+0xc (0x1, 0x18, 0xf0f5be90, 0xf0f5be8c)
0xc01ceb85 _raw_spin_lock+0x79
0xf0f5be4c 0xc02afd93 _spin_lock+0x8 (0x1, 0x18, 0xf0f5be8c, 0x1)
0xf0f5be64 0xf9307302 [dmapi]dm_find_session_and_lock+0x1a (0x2220a, 0x1,
0x8fb5b198, 0x6f6b8864, 0xe)
0xf0f5bea0 0xf93062eb [dmapi]dm_app_get_tdp+0x87 (0xffffffff, 0x2, 0x1,
0xf0f5beb8, 0x3000)
0xf0f5bec0 0xf9302cf6 [dmapi]dm_read_invis_rvp+0x17 (0xffffffff,
0x27c4000, 0x0, 0x4000, 0x0)
0xf0f5bf48 0xf93014b9 [dmapi]dmapi_ioctl+0x4ab (0xab0be160, 0x28,
0xfffffff7, 0xf787f9c0, 0xab0be160)
0xf0f5bf64 0xc01700d3 do_ioctl+0x4f (0xaf345000, 0xf6f58240, 0x1b, 0x2,
0xaf3459ac)
0xf0f5bf98 0xc0170346 vfs_ioctl+0x25a (0xab0be160, 0x1, 0x1b, 0x0,
0xb7825bf0)
0xf0f5bfb4 0xc01703a9 sys_ioctl+0x50
0xc0103d1b sysenter_past_esp+0x54
Stack traceback for pid 3254
0xdfec0720 3254 2748 1 1 R 0xdfec0900 *a1
EBP EIP Function (args)
0xf6f71e40 0xc01ce862 spin_bug (0xf66cbf20, 0xffffffda, 0xf6f71ec0,
0xf6f71ebc)
0xc01ceb58 _raw_spin_lock+0x4c
0xf6f71e48 0xc02afd93 _spin_lock+0x8 (0x1, 0xffffffda, 0x0, 0xb32f01d0)
0xf6f71e60 0xf9307302 [dmapi]dm_find_session_and_lock+0x1a (0x0, 0x1, 0x0,
0x0, 0x3)
0xf6f71ed0 0xf9307538 [dmapi]dm_get_events+0x24 (0xfff, 0xad2e2ae8,
0xb32f02c8, 0x1, 0x0)
0xf6f71f48 0xf9301274 [dmapi]dmapi_ioctl+0x266 (0xb32f01d0, 0x12,
0xfffffff7, 0xf787f9c0, 0xb32f01d0)
0xf6f71f64 0xc01700d3 do_ioctl+0x4f (0x0, 0x0, 0x1b, 0xf66d31cc,
0x8517980)
0xf6f71f98 0xc0170346 vfs_ioctl+0x25a (0xb32f01d0, 0x1, 0x1b, 0xad2f7400,
0xb32f02c4)
0xf6f71fb4 0xc01703a9 sys_ioctl+0x50
0xc0103d1b sysenter_past_esp+0x54
Stack traceback for pid 12452
0xdfb047e0 12452 12451 0 1 R 0xdfb049c0 a2
EBP EIP Function (args)
0xf14cbcbc 0xc02ae254 schedule+0xb84 (0xdffff180)
0xf14cbcc8 0xc02ae33b cond_resched+0x36 (0x0, 0x1, 0xf14cbe53)
0xf14cbcdc 0xc015d07a __kmalloc+0x4a (0x1, 0x8124, 0xa, 0x8, 0xf14cbe48)
0xf14cbd04 0xc018ef93 proc_create+0x8c (0x1, 0xf784d440, 0xf14cbe34,
0xd53b5de8)
0xf14cbd1c 0xc018f045 create_proc_entry+0x5f (0xf14cbe34, 0xf9309030,
0xd53b5de8, 0x7, 0x61637942)
0xf14cbedc 0xf9307c14 [dmapi]dm_create_session+0x234 (0x0, 0x0, 0x80db388,
0xb7fccff4, 0x82fa604)
0xf14cbf48 0xf93010b0 [dmapi]dmapi_ioctl+0xa2 (0xbfd1fb00, 0x3,
0xfffffff7, 0xf1399540, 0xbfd1fb00)
0xf14cbf64 0xc01700d3 do_ioctl+0x4f (0xf54cf080, 0xf782cee4, 0x3,
0xc02b0c16, 0x0)
0xf14cbf98 0xc0170346 vfs_ioctl+0x25a (0xbfd1fb00, 0x0, 0x3, 0x82fa600,
0x2)
0xf14cbfb4 0xc01703a9 sys_ioctl+0x50
0xc0103d1b sysenter_past_esp+0x54
It doesn't appear necessary for create/remove_proc_entry() to be called with the
dm_session_lock spinlock held. The following patch moves these calls outside of
the spinlock.
Signed-off-by: Kevin Jamieson <kjamieson@bycast.com>
Index: fs/dmapi/dmapi_session.c
===================================================================
RCS file: /cvs/linux-2.6-xfs/fs/dmapi/dmapi_session.c,v
retrieving revision 1.31
diff -u -r1.31 dmapi_session.c
--- fs/dmapi/dmapi_session.c 12 Jan 2007 15:07:58 -0000 1.31
+++ fs/dmapi/dmapi_session.c 16 Feb 2007 06:14:08 -0000
@@ -519,13 +519,14 @@
sv_init(&s->sn_readerq, SV_DEFAULT, "dmreadq");
sv_init(&s->sn_writerq, SV_DEFAULT, "dmwritq");
spinlock_init(&s->sn_qlock, "sn_qlock");
- lc = mutex_spinlock(&dm_session_lock);
} else {
lc = mutex_spinlock(&dm_session_lock);
if ((error = dm_find_session(old, &s)) != 0) {
mutex_spinunlock(&dm_session_lock, lc);
return(error);
}
+ unlink_session(s);
+ mutex_spinunlock(&dm_session_lock, lc);
#ifdef CONFIG_PROC_FS
{
char buf[100];
@@ -533,12 +534,13 @@
remove_proc_entry(buf, NULL);
}
#endif
- unlink_session(s);
}
memcpy(s->sn_info, sessinfo, len);
s->sn_info[len-1] = 0; /* if not NULL, then now 'tis */
s->sn_sessid = sid;
+ lc = mutex_spinlock(&dm_session_lock);
link_session(s);
+ mutex_spinunlock(&dm_session_lock, lc);
#ifdef CONFIG_PROC_FS
{
char buf[100];
@@ -549,7 +551,6 @@
entry->owner = THIS_MODULE;
}
#endif
- mutex_spinunlock(&dm_session_lock, lc);
return(0);
}
@@ -583,6 +584,12 @@
mutex_spinunlock(&dm_session_lock, lc);
return(-EBUSY);
}
+
+ /* The session is not in use. Dequeue it from the session chain. */
+
+ unlink_session(s);
+ nested_spinunlock(&s->sn_qlock);
+ mutex_spinunlock(&dm_session_lock, lc);
#ifdef CONFIG_PROC_FS
{
@@ -592,12 +599,6 @@
}
#endif
- /* The session is not in use. Dequeue it from the session chain. */
-
- unlink_session(s);
- nested_spinunlock(&s->sn_qlock);
- mutex_spinunlock(&dm_session_lock, lc);
-
/* Now clear the sessions's disposition registration, and then destroy
the session structure.
*/
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] dm_create_session() calls create_proc_entry() with spinlock held
2007-02-16 6:33 [PATCH] dm_create_session() calls create_proc_entry() with spinlock held Kevin Jamieson
@ 2007-02-19 0:30 ` Vlad Apostolov
0 siblings, 0 replies; 2+ messages in thread
From: Vlad Apostolov @ 2007-02-19 0:30 UTC (permalink / raw)
To: kjamieson; +Cc: xfs
Hi Kevin,
The patch looks good to me and I will merge it the dmapi tree. One think that
should be kept in mind is that there was a race problem in remove_proc_entry()
fixed in kernel 2.6.15-rt2-sr3.
See http://www-gatago.com/linux/kernel/14664637.html
I don't know if this problem has been noticed and was the reason for the old
dmapi implementation but for kernel versions later than 2.6.15-rt2-sr3 your
patch makes perfect sense.
Thanks and regards,
Vlad
Kevin Jamieson wrote:
> dm_create_session() calls create_proc_read_entry() with the
> dm_session_lock
> spinlock held. But create_proc_entry() calls kmalloc(GFP_KERNEL),
> which can
> sleep, so this isn't safe.
>
> We have observed this to cause deadlocks with multiple processes
> creating/using
> DMAPI sessions concurrently (this was with SLES10's 2.6.16.21-0.8
> kernel, but
> the DMAPI code in question is the same in CVS HEAD):
>
> BUG: spinlock cpu recursion on CPU#1, a1/3235
> lock: f930c49c, .magic: dead4ead, .owner: a2/22546, .owner_cpu: 1
>
> Stack traceback for pid 6144
> 0xdfc98270 6144 2748 1 0 R 0xdfc98450 a1
> EBP EIP Function (args)
> 0xf0f5be24 0xc010d89f delay_tsc+0xc (0xf0f5be44)
> 0xf0f5be2c 0xc01cd928 __delay+0xc (0x1, 0x18, 0xf0f5be90, 0xf0f5be8c)
> 0xc01ceb85 _raw_spin_lock+0x79
> 0xf0f5be4c 0xc02afd93 _spin_lock+0x8 (0x1, 0x18, 0xf0f5be8c, 0x1)
> 0xf0f5be64 0xf9307302 [dmapi]dm_find_session_and_lock+0x1a (0x2220a, 0x1,
> 0x8fb5b198, 0x6f6b8864, 0xe)
> 0xf0f5bea0 0xf93062eb [dmapi]dm_app_get_tdp+0x87 (0xffffffff, 0x2, 0x1,
> 0xf0f5beb8, 0x3000)
> 0xf0f5bec0 0xf9302cf6 [dmapi]dm_read_invis_rvp+0x17 (0xffffffff,
> 0x27c4000, 0x0, 0x4000, 0x0)
> 0xf0f5bf48 0xf93014b9 [dmapi]dmapi_ioctl+0x4ab (0xab0be160, 0x28,
> 0xfffffff7, 0xf787f9c0, 0xab0be160)
> 0xf0f5bf64 0xc01700d3 do_ioctl+0x4f (0xaf345000, 0xf6f58240, 0x1b, 0x2,
> 0xaf3459ac)
> 0xf0f5bf98 0xc0170346 vfs_ioctl+0x25a (0xab0be160, 0x1, 0x1b, 0x0,
> 0xb7825bf0)
> 0xf0f5bfb4 0xc01703a9 sys_ioctl+0x50
> 0xc0103d1b sysenter_past_esp+0x54
>
> Stack traceback for pid 3254
> 0xdfec0720 3254 2748 1 1 R 0xdfec0900 *a1
> EBP EIP Function (args)
> 0xf6f71e40 0xc01ce862 spin_bug (0xf66cbf20, 0xffffffda, 0xf6f71ec0,
> 0xf6f71ebc)
> 0xc01ceb58 _raw_spin_lock+0x4c
> 0xf6f71e48 0xc02afd93 _spin_lock+0x8 (0x1, 0xffffffda, 0x0, 0xb32f01d0)
> 0xf6f71e60 0xf9307302 [dmapi]dm_find_session_and_lock+0x1a (0x0, 0x1,
> 0x0,
> 0x0, 0x3)
> 0xf6f71ed0 0xf9307538 [dmapi]dm_get_events+0x24 (0xfff, 0xad2e2ae8,
> 0xb32f02c8, 0x1, 0x0)
> 0xf6f71f48 0xf9301274 [dmapi]dmapi_ioctl+0x266 (0xb32f01d0, 0x12,
> 0xfffffff7, 0xf787f9c0, 0xb32f01d0)
> 0xf6f71f64 0xc01700d3 do_ioctl+0x4f (0x0, 0x0, 0x1b, 0xf66d31cc,
> 0x8517980)
> 0xf6f71f98 0xc0170346 vfs_ioctl+0x25a (0xb32f01d0, 0x1, 0x1b, 0xad2f7400,
> 0xb32f02c4)
> 0xf6f71fb4 0xc01703a9 sys_ioctl+0x50
> 0xc0103d1b sysenter_past_esp+0x54
>
> Stack traceback for pid 12452
> 0xdfb047e0 12452 12451 0 1 R 0xdfb049c0 a2
> EBP EIP Function (args)
> 0xf14cbcbc 0xc02ae254 schedule+0xb84 (0xdffff180)
> 0xf14cbcc8 0xc02ae33b cond_resched+0x36 (0x0, 0x1, 0xf14cbe53)
> 0xf14cbcdc 0xc015d07a __kmalloc+0x4a (0x1, 0x8124, 0xa, 0x8, 0xf14cbe48)
> 0xf14cbd04 0xc018ef93 proc_create+0x8c (0x1, 0xf784d440, 0xf14cbe34,
> 0xd53b5de8)
> 0xf14cbd1c 0xc018f045 create_proc_entry+0x5f (0xf14cbe34, 0xf9309030,
> 0xd53b5de8, 0x7, 0x61637942)
> 0xf14cbedc 0xf9307c14 [dmapi]dm_create_session+0x234 (0x0, 0x0,
> 0x80db388,
> 0xb7fccff4, 0x82fa604)
> 0xf14cbf48 0xf93010b0 [dmapi]dmapi_ioctl+0xa2 (0xbfd1fb00, 0x3,
> 0xfffffff7, 0xf1399540, 0xbfd1fb00)
> 0xf14cbf64 0xc01700d3 do_ioctl+0x4f (0xf54cf080, 0xf782cee4, 0x3,
> 0xc02b0c16, 0x0)
> 0xf14cbf98 0xc0170346 vfs_ioctl+0x25a (0xbfd1fb00, 0x0, 0x3, 0x82fa600,
> 0x2)
> 0xf14cbfb4 0xc01703a9 sys_ioctl+0x50
> 0xc0103d1b sysenter_past_esp+0x54
>
>
> It doesn't appear necessary for create/remove_proc_entry() to be
> called with the
> dm_session_lock spinlock held. The following patch moves these calls
> outside of
> the spinlock.
>
>
> Signed-off-by: Kevin Jamieson <kjamieson@bycast.com>
> Index: fs/dmapi/dmapi_session.c
> ===================================================================
> RCS file: /cvs/linux-2.6-xfs/fs/dmapi/dmapi_session.c,v
> retrieving revision 1.31
> diff -u -r1.31 dmapi_session.c
> --- fs/dmapi/dmapi_session.c 12 Jan 2007 15:07:58 -0000 1.31
> +++ fs/dmapi/dmapi_session.c 16 Feb 2007 06:14:08 -0000
> @@ -519,13 +519,14 @@
> sv_init(&s->sn_readerq, SV_DEFAULT, "dmreadq");
> sv_init(&s->sn_writerq, SV_DEFAULT, "dmwritq");
> spinlock_init(&s->sn_qlock, "sn_qlock");
> - lc = mutex_spinlock(&dm_session_lock);
> } else {
> lc = mutex_spinlock(&dm_session_lock);
> if ((error = dm_find_session(old, &s)) != 0) {
> mutex_spinunlock(&dm_session_lock, lc);
> return(error);
> }
> + unlink_session(s);
> + mutex_spinunlock(&dm_session_lock, lc);
> #ifdef CONFIG_PROC_FS
> {
> char buf[100];
> @@ -533,12 +534,13 @@
> remove_proc_entry(buf, NULL);
> }
> #endif
> - unlink_session(s);
> }
> memcpy(s->sn_info, sessinfo, len);
> s->sn_info[len-1] = 0; /* if not NULL, then now 'tis */
> s->sn_sessid = sid;
> + lc = mutex_spinlock(&dm_session_lock);
> link_session(s);
> + mutex_spinunlock(&dm_session_lock, lc);
> #ifdef CONFIG_PROC_FS
> {
> char buf[100];
> @@ -549,7 +551,6 @@
> entry->owner = THIS_MODULE;
> }
> #endif
> - mutex_spinunlock(&dm_session_lock, lc);
> return(0);
> }
>
> @@ -583,6 +584,12 @@
> mutex_spinunlock(&dm_session_lock, lc);
> return(-EBUSY);
> }
> +
> + /* The session is not in use. Dequeue it from the session chain. */
> +
> + unlink_session(s);
> + nested_spinunlock(&s->sn_qlock);
> + mutex_spinunlock(&dm_session_lock, lc);
>
> #ifdef CONFIG_PROC_FS
> {
> @@ -592,12 +599,6 @@
> }
> #endif
>
> - /* The session is not in use. Dequeue it from the session chain. */
> -
> - unlink_session(s);
> - nested_spinunlock(&s->sn_qlock);
> - mutex_spinunlock(&dm_session_lock, lc);
> -
> /* Now clear the sessions's disposition registration, and then
> destroy
> the session structure.
> */
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2007-02-19 0:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-02-16 6:33 [PATCH] dm_create_session() calls create_proc_entry() with spinlock held Kevin Jamieson
2007-02-19 0:30 ` Vlad Apostolov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox