shmem_unuse race fix

Hi Linus,

The following patch (against clean test13-pre4) removes the races in shmem_unuse. I changed inode.c to not lock the inode if there is no write_inode function. So I can grab the inode while holding the spinlock.

It also optimises the shmem_ftruncate behaviour.

BTW: The generic swapoff path itself has still races if a process is paging in a page which is just freed on swap by try_to_unuse. It gives 'VM: bad swap entries' and worse. But this is not shmem specific. Marcelo would you like to look into this?

Greetings
Christoph

@@ -168,13 +168,6 @@
 		__wait_on_inode(inode);
 }
 
-
-static inline void write_inode(struct inode *inode, int sync)
-{
-	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode)
-		inode->i_sb->s_op->write_inode(inode, sync);
-}
-
 static inline void __iget(struct inode * inode)
 {
 	if (atomic_read(&inode->i_count)) {

@@ -202,16 +195,20 @@
 		list_add(&inode->i_list, atomic_read(&inode->i_count)
 							? &inode_in_use
 							: &inode_unused);
-		/* Set I_LOCK, reset I_DIRTY */
-		inode->i_state |= I_LOCK;
-		inode->i_state &= ~I_DIRTY;
-		spin_unlock(&inode_lock);
+		if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode) {
+                        /* Set I_LOCK, reset I_DIRTY */
+                        inode->i_state |= I_LOCK;
+                        inode->i_state &= ~I_DIRTY;
+                        spin_unlock(&inode_lock);
 
-		write_inode(inode, sync);
+                        inode->i_sb->s_op->write_inode(inode, sync);
 
-		spin_lock(&inode_lock);
-		inode->i_state &= ~I_LOCK;
-		wake_up(&inode->i_wait);
+                        spin_lock(&inode_lock);
+                        inode->i_state &= ~I_LOCK;
+                        wake_up(&inode->i_wait);
+                        return;
+                }
+                inode->i_state &= ~I_DIRTY;
 	}
 }
 

diff -uNr 4-13-4/include/linux/shmem_fs.h c/include/linux/shmem_fs.h
@@ -19,6 +19,7 @@
 
 struct shmem_inode_info {
 	spinlock_t	lock;
+	unsigned long	max_index;
 	swp_entry_t	i_direct[SHMEM_NR_DIRECT]; /* for the first blocks */
 	swp_entry_t   **i_indirect; /* doubly indirect blocks */
 	unsigned long	swapped;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Sure.

Look at this comment on try_to_unuse():

             swap_device_lock(si);
             for (i = 1; i < si->max ; i++) {
                 if (si->swap_map[i] > 0 && si->swap_map[i] != SWAP_MAP_BAD) {
                                /*
                                 * Prevent swaphandle from being completely
                                 * unused by swap_free while we are trying
                                 * to read in the page - this prevents warning
                                 * messages from rw_swap_page_base.
                                 */
                                if (si->swap_map[i] != SWAP_MAP_MAX)
                                        si->swap_map[i]++;
                                swap_device_unlock(si);
                                goto found_entry;

...

I think that incrementing the swap entry count will not allow swap from removing the swap entry (as the comment says)

What I'm missing here?

Thanks

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

I think the culprit is somewhere else. The error occurs in nopage of a process, not in swapoff.

Looking at the following in try_to_unuse:

	found_entry:
		entry = SWP_ENTRY(type, i);

		/* Get a page for the entry, using the existing swap
                   cache page if there is one.  Otherwise, get a clean
                   page and read the swap into it. */
		page = read_swap_cache(entry);
		if (!page) {
			swap_free(entry);
  			return -ENOMEM;
		}
		if (PageSwapCache(page))
			delete_from_swap_cache(page);
		read_lock(&tasklist_lock);
		for_each_task(p)
			unuse_process(p->mm, entry, page);
		read_unlock(&tasklist_lock);
		shmem_unuse(entry, page);

and in unuse_pte:

	if (pte_present(pte)) {
		/* If this entry is swap-cached, then page must already
                   hold the right address for any copies in physical
                   memory */
		if (pte_page(pte) != page)
			return;
		/* We will be removing the swap cache in a moment, so... */
		ptep_mkdirty(dir);
		return;
	}

I see at least a wrong comment. There is no swap cache any more... And another thought is: why can we remove the swap cache entry before finding the process? What prevent reallocation of the swap cache entry by a page fault?

BTW you can reproduce the problem on a UP system quite easily by trashing and always disablingænabling two swap partitions round robin.

Greetings
Christoph

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

I think swapoff() should be fixed..

I moved the

	if (PageSwapCache(page))
		delete_from_swap_cache(page);

thing to _before_ the "unuse_process()" and "shmem_unuse()" code, because I wanted to avoid a race on the PageDirty bit that way. However, that opens up another race, the one you see with "nopage".

Woul dyou mind testing this alternate fix instead:

- add the lines

	repeat:
		ClearPageDirty(page);

to just before the "read_lock(&tasklist_lock);" in try_to_unuse(). We can obviously mark the page clean, because we _are_ going to get rid of it, and in the meantime we have a hold on it by virtue of having raised the page count in "read_swap_cache()".

- move the "delete_from_swap_cache()" call back to _after_ the unuse() calls.

- but just before deleting the entry, we add a new test:

	if (PageDirty(page))
		goto repeat;

this all should mean that if something moves the dirty bit from a page table to the backing store, we will notice, and just re-do the VM scan, which will mark the page table entry dirty again. And because we delete it from the swap cache late, we aren't severing the link with "nopage" handling.

Christoph, how does this sound to you?

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Does not work, but is the right direction I think.

First we need the following patch since otherwise we use a swap entry without having the count increased:

@@ -93,8 +93,8 @@
                entry.val = page->index;
                if (pte_dirty(pte))
                        SetPageDirty(page);
-set_swap_pte:
                swap_duplicate(entry);
+set_swap_pte:
                set_pte(page_table, swp_entry_to_pte(entry));
 drop_pte:
                UnlockPage(page);

@@ -185,7 +185,7 @@
         * we have the swap cache set up to associate the
         * page with that swap entry.
         */
-       entry = get_swap_page();
+       entry = __get_swap_page(2);
        if (!entry.val)
                goto out_unlock_restore; /* No swap space left */
 

Second there look at this in handle_pte_fault:

		/*
		 * If it truly wasn't present, we know that kswapd
		 * and the PTE updates will not touch it later. So
		 * drop the lock.
		 */
		spin_unlock(&mm->page_table_lock);
		if (pte_none(entry))
			return do_no_page(mm, vma, address, write_access, pte);
		return do_swap_page(mm, vma, address, pte, pte_to_swp_entry(entry), write_access);

The comment is wrong. try_to_unuse will touch it. This stumbles over a bad swap entry after try_to_unuse complaining about an undead swap entry.

If I retry in try_to_unuse it goes into an infinite loop since it deadlocks with this.

Ideas?
Christoph

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

No, that shouldn't be needed.

Look at the code-path: the kernel has the page locked, so nothing will de-allocate the swap entry - so it's perfectly ok to increase it later. I dislike the "magic" __get_swap_page(2) thing - we might make get_swap_page() itself _always_ return a swap entry with count two (one fot eh swap cache, one for the user), or we should keep it the way it was (where we explicitly increment it for the user).

Ok. How about making try_to_unuse() just get the VM semaphore instead of the page table lock?

Then try_to_unuse() would follow all the right rules, and the above problem wouldn't exist..

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

I am not sure that page locking is done very strictly in the swap cache. I had to fiddle around that sometimes in shmem.c.

The patch actually is getting me the 'right error': Undead swap entries in swapoff instead of bad swap entries in nopage. The latter means there is a swapentry noted in the page table which was legally removed. And that's definitely wrong.

Yes, I agree. I was too lazy to check the other uses of get_swap_page for this patchlet. But at least shmem.c uses the same and I think it's logical. We always need a swap cache and a user reference.

If this is right we should do this. There is no need to care about contention in swapoff. It's definitely not the common path. But we have to be careful about deadlocks....

Greetings
Christoph

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/