Re: Please help me understand ->writepage. Was Re: segfault mdadm --write-behind, 2.6.14-mm2 (was: Re: RAID1 ramdisk patch)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Monday November 21, [email protected] wrote:
> Neil Brown <[email protected]> wrote:
> >
> > Help ???
> 
> Indeed.  tmpfs is crackpottery.

Ok, that explains a lot... :-)

> > 
> >  Any advice would be most welcome!
> 
> Skip the writepage if !mapping_cap_writeback_dirty(page->mapping), I guess.
> Or, if appropriate, just sync the file.  Use filemap_fdatawrite() or even
> refactor do_fsync() and use most of that.

Uhm, what would you think of testing mapping_cap_writeback_dirty in
write_one_page??  If you don't like it, I can take it into write_page.

> 
> Also, write_page() doesn't need to run set_page_dirty(); ->commit_write()
> will do that.

Ok.... but I think I'm dropping prepare_write / commit_write.

> 
> Several kmap()s in there which can become kmap_atomic().

I've made them all kmap_atomic.

> 
> bitmap_init_from_disk() might be leaking bitmap->filemap on kmalloc-failed
> error path.

It looks that way, but actually not.  bitmap_create requires that
bitmap_destroy always be called afterwards, even on an error.  Not the
best interface I'd agree...

> 
> bitmap->filemap_attr can be allocated with kzalloc() now.
Yes, thanks.

So Sander, could you try this patch for main against reiser4?  It
seems to work on ext3 and tmpfs and has some chance of not mucking up
on reiser4.

Thanks,
NeilBrown


===File /home/src/mm/.patches/applied/014MdBitmapFix========
Status: devel

Hopefully make md/bitmaps work on files other than ext3



Signed-off-by: Neil Brown <[email protected]>

### Diffstat output
 ./drivers/md/bitmap.c |   64 +++++++++++++++++++-------------------------------
 ./mm/page-writeback.c |    4 +++
 2 files changed, 29 insertions(+), 39 deletions(-)

diff ./drivers/md/bitmap.c~current~ ./drivers/md/bitmap.c
--- ./drivers/md/bitmap.c~current~	2005-11-22 14:06:53.000000000 +1100
+++ ./drivers/md/bitmap.c	2005-11-22 14:07:05.000000000 +1100
@@ -310,7 +310,6 @@ static int write_sb_page(mddev_t *mddev,
  */
 static int write_page(struct bitmap *bitmap, struct page *page, int wait)
 {
-	int ret = -ENOMEM;
 
 	if (bitmap->file == NULL)
 		return write_sb_page(bitmap->mddev, bitmap->offset, page, wait);
@@ -326,15 +325,6 @@ static int write_page(struct bitmap *bit
 		}
 	}
 
-	ret = page->mapping->a_ops->prepare_write(bitmap->file, page, 0, PAGE_SIZE);
-	if (!ret)
-		ret = page->mapping->a_ops->commit_write(bitmap->file, page, 0,
-			PAGE_SIZE);
-	if (ret) {
-		unlock_page(page);
-		return ret;
-	}
-
 	set_page_dirty(page); /* force it to be written out */
 
 	if (!wait) {
@@ -406,11 +396,11 @@ int bitmap_update_sb(struct bitmap *bitm
 		return 0;
 	}
 	spin_unlock_irqrestore(&bitmap->lock, flags);
-	sb = (bitmap_super_t *)kmap(bitmap->sb_page);
+	sb = (bitmap_super_t *)kmap_atomic(bitmap->sb_page, KM_USER0);
 	sb->events = cpu_to_le64(bitmap->mddev->events);
 	if (!bitmap->mddev->degraded)
 		sb->events_cleared = cpu_to_le64(bitmap->mddev->events);
-	kunmap(bitmap->sb_page);
+	kunmap_atomic(bitmap->sb_page, KM_USER0);
 	return write_page(bitmap, bitmap->sb_page, 1);
 }
 
@@ -421,7 +411,7 @@ void bitmap_print_sb(struct bitmap *bitm
 
 	if (!bitmap || !bitmap->sb_page)
 		return;
-	sb = (bitmap_super_t *)kmap(bitmap->sb_page);
+	sb = (bitmap_super_t *)kmap_atomic(bitmap->sb_page, KM_USER0);
 	printk(KERN_DEBUG "%s: bitmap file superblock:\n", bmname(bitmap));
 	printk(KERN_DEBUG "         magic: %08x\n", le32_to_cpu(sb->magic));
 	printk(KERN_DEBUG "       version: %d\n", le32_to_cpu(sb->version));
@@ -440,7 +430,7 @@ void bitmap_print_sb(struct bitmap *bitm
 	printk(KERN_DEBUG "     sync size: %llu KB\n",
 			(unsigned long long)le64_to_cpu(sb->sync_size)/2);
 	printk(KERN_DEBUG "max write behind: %d\n", le32_to_cpu(sb->write_behind));
-	kunmap(bitmap->sb_page);
+	kunmap_atomic(bitmap->sb_page, KM_USER0);
 }
 
 /* read the superblock from the bitmap file and initialize some bitmap fields */
@@ -466,7 +456,7 @@ static int bitmap_read_sb(struct bitmap 
 		return err;
 	}
 
-	sb = (bitmap_super_t *)kmap(bitmap->sb_page);
+	sb = (bitmap_super_t *)kmap_atomic(bitmap->sb_page, KM_USER0);
 
 	if (bytes_read < sizeof(*sb)) { /* short read */
 		printk(KERN_INFO "%s: bitmap file superblock truncated\n",
@@ -535,7 +525,7 @@ success:
 		bitmap->events_cleared = bitmap->mddev->events;
 	err = 0;
 out:
-	kunmap(bitmap->sb_page);
+	kunmap_atomic(bitmap->sb_page, KM_USER0);
 	if (err)
 		bitmap_print_sb(bitmap);
 	return err;
@@ -560,7 +550,7 @@ static void bitmap_mask_state(struct bit
 	}
 	page_cache_get(bitmap->sb_page);
 	spin_unlock_irqrestore(&bitmap->lock, flags);
-	sb = (bitmap_super_t *)kmap(bitmap->sb_page);
+	sb = (bitmap_super_t *)kmap_atomic(bitmap->sb_page, KM_USER0);
 	switch (op) {
 		case MASK_SET: sb->state |= bits;
 				break;
@@ -568,7 +558,7 @@ static void bitmap_mask_state(struct bit
 				break;
 		default: BUG();
 	}
-	kunmap(bitmap->sb_page);
+	kunmap_atomic(bitmap->sb_page, KM_USER0);
 	page_cache_release(bitmap->sb_page);
 }
 
@@ -621,8 +611,7 @@ static void bitmap_file_unmap(struct bit
 	spin_unlock_irqrestore(&bitmap->lock, flags);
 
 	while (pages--)
-		if (map[pages]->index != 0) /* 0 is sb_page, release it below */
-			page_cache_release(map[pages]);
+		page_cache_release(map[pages]);
 	kfree(map);
 	kfree(attr);
 
@@ -771,7 +760,7 @@ static void bitmap_file_set_bit(struct b
 		set_bit(bit, kaddr);
 	else
 		ext2_set_bit(bit, kaddr);
-	kunmap_atomic(kaddr, KM_USER0);
+	kunmap_atomic(page, KM_USER0);
 	PRINTK("set file bit %lu page %lu\n", bit, page->index);
 
 	/* record page number so it gets flushed to disk when unplug occurs */
@@ -854,6 +843,7 @@ static int bitmap_init_from_disk(struct 
 	unsigned long bytes, offset, dummy;
 	int outofdate;
 	int ret = -ENOSPC;
+	void *paddr;
 
 	chunks = bitmap->chunks;
 	file = bitmap->file;
@@ -887,12 +877,10 @@ static int bitmap_init_from_disk(struct 
 	if (!bitmap->filemap)
 		goto out;
 
-	bitmap->filemap_attr = kmalloc(sizeof(long) * num_pages, GFP_KERNEL);
+	bitmap->filemap_attr = kzalloc(sizeof(long) * num_pages, GFP_KERNEL);
 	if (!bitmap->filemap_attr)
 		goto out;
 
-	memset(bitmap->filemap_attr, 0, sizeof(long) * num_pages);
-
 	oldindex = ~0L;
 
 	for (i = 0; i < chunks; i++) {
@@ -901,8 +889,6 @@ static int bitmap_init_from_disk(struct 
 		bit = file_page_offset(i);
 		if (index != oldindex) { /* this is a new page, read it in */
 			/* unmap the old page, we're done with it */
-			if (oldpage != NULL)
-				kunmap(oldpage);
 			if (index == 0) {
 				/*
 				 * if we're here then the superblock page
@@ -910,6 +896,7 @@ static int bitmap_init_from_disk(struct 
 				 * we've already read it in, so just use it
 				 */
 				page = bitmap->sb_page;
+				page_cache_get(page);
 				offset = sizeof(bitmap_super_t);
 			} else if (file) {
 				page = read_page(file, index, &dummy);
@@ -925,18 +912,18 @@ static int bitmap_init_from_disk(struct 
 
 			oldindex = index;
 			oldpage = page;
-			kmap(page);
 
 			if (outofdate) {
 				/*
 				 * if bitmap is out of date, dirty the
 			 	 * whole page and write it out
 				 */
-				memset(page_address(page) + offset, 0xff,
+				paddr = kmap_atomic(page, KM_USER0);
+				memset(paddr + offset, 0xff,
 				       PAGE_SIZE - offset);
+				kunmap_atomic(page, KM_USER0);
 				ret = write_page(bitmap, page, 1);
 				if (ret) {
-					kunmap(page);
 					/* release, page not in filemap yet */
 					page_cache_release(page);
 					goto out;
@@ -945,10 +932,12 @@ static int bitmap_init_from_disk(struct 
 
 			bitmap->filemap[bitmap->file_pages++] = page;
 		}
+		paddr = kmap_atomic(page, KM_USER0);
 		if (bitmap->flags & BITMAP_HOSTENDIAN)
-			b = test_bit(bit, page_address(page));
+			b = test_bit(bit, paddr);
 		else
-			b = ext2_test_bit(bit, page_address(page));
+			b = ext2_test_bit(bit, paddr);
+		kunmap_atomic(page, KM_USER0);
 		if (b) {
 			/* if the disk bit is set, set the memory bit */
 			bitmap_set_memory_bits(bitmap, i << CHUNK_BLOCK_SHIFT(bitmap),
@@ -963,9 +952,6 @@ static int bitmap_init_from_disk(struct 
 	ret = 0;
 	bitmap_mask_state(bitmap, BITMAP_STALE, MASK_UNSET);
 
-	if (page) /* unmap the last page */
-		kunmap(page);
-
 	if (bit_cnt) { /* Kick recovery if any bits were set */
 		set_bit(MD_RECOVERY_NEEDED, &bitmap->mddev->recovery);
 		md_wakeup_thread(bitmap->mddev->thread);
@@ -1021,6 +1007,7 @@ int bitmap_daemon_work(struct bitmap *bi
 	int err = 0;
 	int blocks;
 	int attr;
+	void *paddr;
 
 	if (bitmap == NULL)
 		return 0;
@@ -1077,14 +1064,12 @@ int bitmap_daemon_work(struct bitmap *bi
 					set_page_attr(bitmap, lastpage, BITMAP_PAGE_NEEDWRITE);
 					spin_unlock_irqrestore(&bitmap->lock, flags);
 				}
-				kunmap(lastpage);
 				page_cache_release(lastpage);
 				if (err)
 					bitmap_file_kick(bitmap);
 			} else
 				spin_unlock_irqrestore(&bitmap->lock, flags);
 			lastpage = page;
-			kmap(page);
 /*
 			printk("bitmap clean at page %lu\n", j);
 */
@@ -1107,10 +1092,12 @@ int bitmap_daemon_work(struct bitmap *bi
 						  -1);
 
 				/* clear the bit */
+				paddr = kmap_atomic(page, KM_USER0);
 				if (bitmap->flags & BITMAP_HOSTENDIAN)
-					clear_bit(file_page_offset(j), page_address(page));
+					clear_bit(file_page_offset(j), paddr);
 				else
-					ext2_clear_bit(file_page_offset(j), page_address(page));
+					ext2_clear_bit(file_page_offset(j), paddr);
+				kunmap_atomic(page, KM_USER0);
 			}
 		}
 		spin_unlock_irqrestore(&bitmap->lock, flags);
@@ -1118,7 +1105,6 @@ int bitmap_daemon_work(struct bitmap *bi
 
 	/* now sync the final page */
 	if (lastpage != NULL) {
-		kunmap(lastpage);
 		spin_lock_irqsave(&bitmap->lock, flags);
 		if (get_page_attr(bitmap, lastpage) &BITMAP_PAGE_NEEDWRITE) {
 			clear_page_attr(bitmap, lastpage, BITMAP_PAGE_NEEDWRITE);

diff ./mm/page-writeback.c~current~ ./mm/page-writeback.c
--- ./mm/page-writeback.c~current~	2005-11-22 14:06:53.000000000 +1100
+++ ./mm/page-writeback.c	2005-11-22 14:07:05.000000000 +1100
@@ -583,6 +583,10 @@ int write_one_page(struct page *page, in
 	};
 
 	BUG_ON(!PageLocked(page));
+	if (!mapping_cap_writeback_dirty(mapping)) {
+		unlock_page(page);
+		return ret;
+	}
 
 	if (wait)
 		wait_on_page_writeback(page);
============================================================
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux