Messages in this thread Patches in this message | | | Date | Fri, 2 Jun 2006 21:14:30 +0200 | From | Olaf Hering <> | Subject | Re: [PATCH] cramfs corruption after BLKFLSBUF on loop device |
| |
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, }; /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |