Testing some othe rwork, totally unrelated, we added a might_sleep() to
mntput(). It turned up this:
Debug: sleeping function called from invalid context at
include/linux/mount.h:82
in_atomic():1, irqs_disabled():0
Call Trace:
[<c012378f>] __might_sleep+0xab/0xcb
[<c018787c>] d_path+0x171/0x272
[<c018ff50>] seq_path+0x4b/0xf4
[<c0165b12>] swap_show+0x3d/0x110
[<c018f87d>] seq_read+0xa8/0x31b
[<c015aa83>] do_brk+0x14f/0x21c
[<c0168fae>] vfs_read+0xaf/0x119
[<c0169224>] sys_read+0x3f/0x5d
[<c010b4e1>] sysenter_past_esp+0x52/0x71
* /proc/swaps uses seq_file code, calling seq_path() with swaplock held
* seq_path() calls d_path()
* d_path() calls mntput() which might_sleep()
Is this worth trying to solve?
--
Tim Hockin
Sun Microsystems, Linux Software Engineering
[email protected]
All opinions are my own, not Sun's
On Wed, Apr 28, 2004 at 04:24:58PM -0700, Tim Hockin wrote:
> * /proc/swaps uses seq_file code, calling seq_path() with swaplock held
> * seq_path() calls d_path()
> * d_path() calls mntput() which might_sleep()
>
> Is this worth trying to solve?
Hrm... Yes, it is - we could have chroot(2) called from another thread
while traversing the path to current root and have e.g. umount -l trigger
final umount when d_path() is finished.
Note that we have another blocking function called there anyway - dput()
will happily block under similar conditions (s/umount -l/rm/).
Lovely... So we need yet another semaphore in swapfile.c (we can't turn
swaplock into semaphore *and* can't reuse swaps_bdev_sem, since dput()
et.al. are not just blocking but can cause any IO and memory allocations).
I'll try to put together something not too revolting; will post the patch
in a followup...
On Thu, Apr 29, 2004 at 01:53:35AM +0100, [email protected] wrote:
> On Wed, Apr 28, 2004 at 04:24:58PM -0700, Tim Hockin wrote:
> > * /proc/swaps uses seq_file code, calling seq_path() with swaplock held
> > * seq_path() calls d_path()
> > * d_path() calls mntput() which might_sleep()
> >
> > Is this worth trying to solve?
>
> Hrm... Yes, it is - we could have chroot(2) called from another thread
> while traversing the path to current root and have e.g. umount -l trigger
> final umount when d_path() is finished.
>
> Note that we have another blocking function called there anyway - dput()
> will happily block under similar conditions (s/umount -l/rm/).
>
> Lovely... So we need yet another semaphore in swapfile.c (we can't turn
> swaplock into semaphore *and* can't reuse swaps_bdev_sem, since dput()
> et.al. are not just blocking but can cause any IO and memory allocations).
>
> I'll try to put together something not too revolting; will post the patch
> in a followup...
OK, here comes. New semaphore protecting insertions/removals in the
set of swap components + switch of ->start()/->stop() to the same
semaphore [fixes deadlocks] + trivial cleanup of ->next().
See if it works for you...
diff -urN RC6-rc3/mm/swapfile.c RC6-rc3-current/mm/swapfile.c
--- RC6-rc3/mm/swapfile.c Tue Apr 27 23:17:39 2004
+++ RC6-rc3-current/mm/swapfile.c Wed Apr 28 21:34:30 2004
@@ -45,6 +45,8 @@
struct swap_info_struct swap_info[MAX_SWAPFILES];
+static DECLARE_MUTEX(swapon_sem);
+
/*
* Array of backing blockdevs, for swap_unplug_fn. We need this because the
* bdev->unplug_fn can sleep and we cannot hold swap_list_lock while calling
@@ -1158,6 +1160,7 @@
swap_list_unlock();
goto out_dput;
}
+ down(&swapon_sem);
down(&swap_bdevs_sem);
swap_list_lock();
swap_device_lock(p);
@@ -1172,6 +1175,7 @@
swap_list_unlock();
remove_swap_bdev(p->bdev);
up(&swap_bdevs_sem);
+ up(&swapon_sem);
vfree(swap_map);
if (S_ISBLK(mapping->host->i_mode)) {
struct block_device *bdev = I_BDEV(mapping->host);
@@ -1197,7 +1201,7 @@
int i;
loff_t l = *pos;
- swap_list_lock();
+ down(&swapon_sem);
for (i = 0; i < nr_swapfiles; i++, ptr++) {
if (!(ptr->flags & SWP_USED) || !ptr->swap_map)
@@ -1212,9 +1216,9 @@
static void *swap_next(struct seq_file *swap, void *v, loff_t *pos)
{
struct swap_info_struct *ptr = v;
- void *endptr = (void *) swap_info + nr_swapfiles * sizeof(struct swap_info_struct);
+ struct swap_info_struct *endptr = swap_info + nr_swapfiles;
- for (++ptr; ptr < (struct swap_info_struct *) endptr; ptr++) {
+ for (++ptr; ptr < endptr; ptr++) {
if (!(ptr->flags & SWP_USED) || !ptr->swap_map)
continue;
++*pos;
@@ -1226,7 +1230,7 @@
static void swap_stop(struct seq_file *swap, void *v)
{
- swap_list_unlock();
+ up(&swapon_sem);
}
static int swap_show(struct seq_file *swap, void *v)
@@ -1513,6 +1517,7 @@
if (error)
goto bad_swap;
+ down(&swapon_sem);
down(&swap_bdevs_sem);
swap_list_lock();
swap_device_lock(p);
@@ -1541,6 +1546,7 @@
swap_list_unlock();
install_swap_bdev(p->bdev);
up(&swap_bdevs_sem);
+ up(&swapon_sem);
error = 0;
goto out;
bad_swap:
On Thu, Apr 29, 2004 at 03:03:09AM +0100, [email protected] wrote:
> OK, here comes. New semaphore protecting insertions/removals in the
> set of swap components + switch of ->start()/->stop() to the same
> semaphore [fixes deadlocks] + trivial cleanup of ->next().
>
> See if it works for you...
Well, it stops bitching about might_sleep(), so it solves the obvious
problems. I'm not in a position to comment about the other complexities of
swapfile.c, so I'll take it on faith that you got it right. :)
Thanks
Tim