Re: [PATCH] cramfs corruption after BLKFLSBUF on loop device

[Olaf Hering <olh@suse.de>]

 On Fri, Jun 02, Andrew Morton wrote:

> I'd suggest you run SetPagePrivate() and SetPageChecked() on the locked
> page and implement a_ops.releasepage(), which will fail if PageChecked(),
> and will succeed otherwise:
> 
> /*
>  * cramfs_releasepage() will fail if cramfs_read() set PG_checked.  This
>  * is so that invalidate_inode_pages() cannot zap the page while
>  * cramfs_read() is trying to get at its contents.
>  */
> cramfs_releasepage(...)
> {
> 	if (PageChecked(page))
> 		return 0;
> 	return 1;
> }

This function is not triggered in my testing.

> cramfs_read(...)
> {
> 	lock_page(page);
> 	SetPagePrivate(page);
> 	SetPageChecked(page);
> 	read_mapping_page(...);
> 	lock_page(page);
> 	if (page->mapping == NULL) {
> 		/* truncate got there first */
> 		unlock_page(page);
> 		bale();
> 	}
> 	memcpy();
> 	ClearPageChecked(page);
> 	ClearPagePrivate(page);
> 	unlock_page(page);
> }

recursive recursion? Where is page supposed to come from? Did you mean
to call all this with a page returned from read_mapping_page(), and
call read_mapping_page() twice?

My version seems to leak memory somehow, Inactive and slab buffer_head
keeps growing. I guess something is missing in the picture.

Doesnt read_cache_page lock the page already?
read_cache_page
  __read_cache_page
    add_to_page_cache_lru
      add_to_page_cache
        SetPageLocked

--- /tmp/meminfo-1149269715     2006-06-02 13:35:15.439683381 -0400
+++ x   2006-06-02 13:53:20.379683381 -0400
@@ -1,23 +1,23 @@
 MemTotal:      3952952 kB
-MemFree:       3395708 kB
+MemFree:       2966276 kB
 Buffers:             0 kB
-Cached:         328960 kB
+Cached:         326904 kB
 SwapCached:          0 kB
-Active:         366884 kB
-Inactive:       111496 kB
+Active:         381720 kB
+Inactive:       518188 kB
 HighTotal:           0 kB
 HighFree:            0 kB
 LowTotal:      3952952 kB
-LowFree:       3395708 kB
+LowFree:       2966276 kB
 SwapTotal:           0 kB
 SwapFree:            0 kB
 Dirty:               0 kB
 Writeback:           0 kB
-Mapped:          56320 kB
-Slab:            60608 kB
+Mapped:          60440 kB
+Slab:            68704 kB
 CommitLimit:   1976476 kB
-Committed_AS:   155304 kB
-PageTables:       1216 kB
+Committed_AS:   158780 kB
+PageTables:       1112 kB
 VmallocTotal: 8589934592 kB
 VmallocUsed:      2452 kB
 VmallocChunk: 8589932068 kB


diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 9efcc3a..1266b8e 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -141,6 +141,35 @@ static unsigned buffer_blocknr[READ_BUFF
 static struct super_block * buffer_dev[READ_BUFFERS];
 static int next_buffer;
 
+static void cramfs_read_putpage(struct page *page)
+{
+	ClearPageChecked(page);
+	ClearPagePrivate(page);
+	unlock_page(page);
+	page_cache_release(page);
+}
+
+/* return a page in PageUptodate state, BLKFLSBUF may have flushed the page */
+static struct page *cramfs_read_getpage(struct address_space *m, unsigned int n)
+{
+	struct page *page;
+	int readagain = 5;
+retry:
+	page = read_cache_page(m, n, (filler_t *)m->a_ops->readpage, NULL);
+	if (IS_ERR(page))
+		return NULL;
+	lock_page(page);
+	SetPagePrivate(page);
+	SetPageChecked(page);
+	if (PageUptodate(page))
+		return page;
+	cramfs_read_putpage(page);
+	printk("cramfs_read_getpage %d %p\n", 5 - readagain + 1, page);
+	if (readagain--)
+		goto retry;
+	return NULL;
+}
+
 /*
  * Returns a pointer to a buffer containing at least LEN bytes of
  * filesystem starting at byte offset OFFSET into the filesystem.
@@ -148,8 +177,8 @@ static int next_buffer;
 static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned int len)
 {
 	struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
-	struct page *pages[BLKS_PER_BUF];
-	unsigned i, blocknr, buffer, unread;
+	struct page *page;
+	unsigned i, blocknr, buffer;
 	unsigned long devsize;
 	char *data;
 
@@ -175,48 +204,22 @@ static void *cramfs_read(struct super_bl
 
 	devsize = mapping->host->i_size >> PAGE_CACHE_SHIFT;
 
-	/* Ok, read in BLKS_PER_BUF pages completely first. */
-	unread = 0;
-	for (i = 0; i < BLKS_PER_BUF; i++) {
-		struct page *page = NULL;
-
-		if (blocknr + i < devsize) {
-			page = read_cache_page(mapping, blocknr + i,
-				(filler_t *)mapping->a_ops->readpage,
-				NULL);
-			/* synchronous error? */
-			if (IS_ERR(page))
-				page = NULL;
-		}
-		pages[i] = page;
-	}
-
-	for (i = 0; i < BLKS_PER_BUF; i++) {
-		struct page *page = pages[i];
-		if (page) {
-			wait_on_page_locked(page);
-			if (!PageUptodate(page)) {
-				/* asynchronous error */
-				page_cache_release(page);
-				pages[i] = NULL;
-			}
-		}
-	}
-
 	buffer = next_buffer;
 	next_buffer = NEXT_BUFFER(buffer);
 	buffer_blocknr[buffer] = blocknr;
 	buffer_dev[buffer] = sb;
-
 	data = read_buffers[buffer];
+
 	for (i = 0; i < BLKS_PER_BUF; i++) {
-		struct page *page = pages[i];
-		if (page) {
-			memcpy(data, kmap(page), PAGE_CACHE_SIZE);
-			kunmap(page);
-			page_cache_release(page);
-		} else
-			memset(data, 0, PAGE_CACHE_SIZE);
+		if (blocknr + i < devsize) {
+			page = cramfs_read_getpage(mapping, blocknr + i);
+			if (page) {
+				memcpy(data, kmap_atomic(page, KM_USER0), PAGE_CACHE_SIZE);
+				kunmap_atomic(page, KM_USER0);
+				cramfs_read_putpage(page);
+			} else
+				memset(data, 0, PAGE_CACHE_SIZE);
+		}
 		data += PAGE_CACHE_SIZE;
 	}
 	return read_buffers[buffer] + offset;
@@ -501,8 +504,19 @@ static int cramfs_readpage(struct file *
 	return 0;
 }
 
+static int cramfs_releasepage(struct page *page, gfp_t mask)
+{
+	int ret = -ENOENT;
+	if (page)
+		ret = !PageChecked(page);
+	printk("cramfs_releasepage called for page %p checked %d\n", page, ret);
+	WARN_ON(1);
+	return ret;
+}
+
 static struct address_space_operations cramfs_aops = {
-	.readpage = cramfs_readpage
+	.readpage = cramfs_readpage,
+	.releasepage = cramfs_releasepage,
 };
 
 /*