Skip to contentSkip to conversation

SetPageDirty in shmem_nopage

PATCH Christoph Rohland 2 participants 3 messages Jan 14, 2001

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Hi Linus,

While playing with the shmem read/write support I realised that the accounting for shmem is broken:

Since we do not mark the page dirty at allocation time the vm can drop it at any time as long as it is not written to. But shmem never adjusts its accounting to that and will happily increase the use counter for both the inode and the fs.

The appended patch fixes this by recalculating the inodes size in nopage and writepage. With this change i_blocks is still an upper bound of the real usage, but should be near the real value in normal cases. At least it does not grow any more beyond the inodes size.

Any idea to handle this better? Else I think this should be applied.

Greetings
Christoph

mm/shmem.c +38 -19
diff -uNr 2.4.0-nosymlink/mm/shmem.c 2.4.0-nosymlink-calc/mm/shmem.c
@@ -117,11 +117,43 @@
 	return 0;
 }
 
+/*
+ * shmem_recalc_inode - recalculate the size of an inode
+ *
+ * @inode: inode to recalc
+ *
+ * We have to calculate the free blocks since the mm can drop pages
+ * behind our back
+ *
+ * But we know that normally
+ * inodes->i_blocks == inode->i_mapping->nrpages + info->swapped
+ *
+ * So the mm freed 
+ * inodes->i_blocks - (inode->i_mapping->nrpages + info->swapped)
+ *
+ * It has to be called with the spinlock held.
+ */
+
+static void shmem_recalc_inode(struct inode * inode)
+{
+	unsigned long freed;
+
+	freed = inode->i_blocks -
+		(inode->i_mapping->nrpages + inode->u.shmem_i.swapped);
+	if (freed){
+		struct shmem_sb_info * info = &inode->i_sb->u.shmem_sb;
+		inode->i_blocks -= freed;
+		spin_lock (&info->stat_lock);
+		info->free_blocks += freed;
+		spin_unlock (&info->stat_lock);
+	}
+}
+
 static void shmem_truncate (struct inode * inode)
 {
 	int clear_base;
 	unsigned long start;
-	unsigned long mmfreed, freed = 0;
+	unsigned long freed = 0;
 	swp_entry_t **base, **ptr;
 	struct shmem_inode_info * info = &inode->u.shmem_i;
 
@@ -154,26 +186,9 @@
 	info->i_indirect = 0;
 
 out:
-
-	/*
-	 * We have to calculate the free blocks since we do not know
-	 * how many pages the mm discarded
-	 *
-	 * But we know that normally
-	 * inodes->i_blocks == inode->i_mapping->nrpages + info->swapped
-	 *
-	 * So the mm freed 
-	 * inodes->i_blocks - (inode->i_mapping->nrpages + info->swapped)
-	 */
-
-	mmfreed = inode->i_blocks - (inode->i_mapping->nrpages + info->swapped);
 	info->swapped -= freed;
-	inode->i_blocks -= freed + mmfreed;
+	shmem_recalc_inode(inode);
 	spin_unlock (&info->lock);
-
-	spin_lock (&inode->i_sb->u.shmem_sb.stat_lock);
-	inode->i_sb->u.shmem_sb.free_blocks += freed + mmfreed;
-	spin_unlock (&inode->i_sb->u.shmem_sb.stat_lock);
 }
 
 static void shmem_delete_inode(struct inode * inode)
@@ -208,6 +223,7 @@
 		return 1;
 
 	spin_lock(&info->lock);
+	shmem_recalc_inode(page->mapping->host);
 	entry = shmem_swp_entry (info, page->index);
 	if (!entry)	/* this had been allocted on page allocation */
 		BUG();
@@ -269,6 +285,9 @@
 	entry = shmem_swp_entry (info, idx);
 	if (!entry)
 		goto oom;
+	spin_lock (&info->lock);
+	shmem_recalc_inode(inode);
+	spin_unlock (&info->lock);
 	if (entry->val) {
 		unsigned long flags;
 

-
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/

#1 lore raw
Quoted 5 lines · Christoph Rohland

Since we do not mark the page dirty at allocation time the vm can drop it at any time as long as it is not written to. But shmem never adjusts its accounting to that and will happily increase the use counter for both the inode and the fs.

Why do you increment the use counter at all in nopage?

There's something wrong here. You shouldn't need to calculate any of this. The VM layer already keeps track of how many pages are associated with a mapping in "mapping->nr_pages". Why do you maintain extra counters that do not give you anything at all?

You should count how many swap cache entries you have allocated for this inode, and nothing more - the VM keeps track of everything else for you already. It looks like this code is all historical baggage from when the shm code didn't use the VM page cache? I'd rather remove it than try to edit it, no?

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/

#2 lore raw
Quoted 2 lines · Linus Torvalds

On 14 Jan 2001, Christoph Rohland wrote:
Why do you increment the use counter at all in nopage?

First to be able to limit the overall number of pages used by the filesystem and second to have the right value for the number of blocks in [f]stat.

Show me a way to get the overall number of vm pages in the fs and I drop it in a minute.

Quoted 2 lines

It looks like this code is all historical baggage from when the shm code didn't use the VM page cache?

No, it was introduced with the changes to use the page cache.

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/