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

From: Neil Brown
Date: Mon Nov 21 2005 - 22:13:21 EST

On Monday November 21, akpm@xxxxxxxx wrote:
> Neil Brown <neilb@xxxxxxx> 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 <neilb@xxxxxxx>

### 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/