Warning on memory offline (and possible in usual migration?)

Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

In 2.6.25-rc8-mm2.

I saw below warning at memory offlining. please help.
(ia64/NUMA box with ext3 file system. Almost all memory are free memory.)
==

localhost.localdomain login: ------------[ cut here ]------------
WARNING: at fs/buffer.c:720 __set_page_dirty+0x330/0x360()
Modules linked in: ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge ipv6 ipmi_watchdog mptctl ipmi_devintf ipmi_si ipmi_msghandler vfat fat dm_multipath parport_pc lp parport sg tg3 e100 mii button shpchp dm_snapshot dm_zero dm_mirror dm_log dm_mod usb_storage mptspi mptscsih scsi_transport_spi mptbase sd_mod scsi_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd [last unloaded: ipmi_watchdog]

Call Trace:
 [<a000000100015220>] show_stack+0x80/0xa0
                                sp=e000012027b9fae0 bsp=e000012027b99398
 [<a000000100015270>] dump_stack+0x30/0x60
                                sp=e000012027b9fcb0 bsp=e000012027b99380
 [<a000000100089ed0>] warn_on_slowpath+0x90/0xe0
                                sp=e000012027b9fcb0 bsp=e000012027b99358
 [<a0000001001f8b10>] __set_page_dirty+0x330/0x360
                                sp=e000012027b9fda0 bsp=e000012027b99328
 [<a0000001001ffb90>] __set_page_dirty_buffers+0xd0/0x280
                                sp=e000012027b9fda0 bsp=e000012027b992f8
 [<a00000010012fec0>] set_page_dirty+0xc0/0x260
                                sp=e000012027b9fda0 bsp=e000012027b992d0
 [<a000000100195670>] migrate_page_copy+0x5d0/0x5e0
                                sp=e000012027b9fda0 bsp=e000012027b992a8
 [<a000000100197840>] buffer_migrate_page+0x2e0/0x3c0
                                sp=e000012027b9fda0 bsp=e000012027b99260
 [<a000000100195eb0>] migrate_pages+0x770/0xe00
                                sp=e000012027b9fda0 bsp=e000012027b991a8
 [<a000000100191250>] offline_pages+0x6f0/0xa20
                                sp=e000012027b9fdf0 bsp=e000012027b99118
 [<a00000010006b1f0>] remove_memory+0x30/0x60
                                sp=e000012027b9fe20 bsp=e000012027b990f0
 [<a000000100482c50>] memory_block_change_state+0x390/0x400
                                sp=e000012027b9fe20 bsp=e000012027b990a0
 [<a000000100483720>] store_mem_state+0x1e0/0x200
                                sp=e000012027b9fe20 bsp=e000012027b99068
 [<a0000001004718a0>] sysdev_store+0x60/0xa0
                                sp=e000012027b9fe20 bsp=e000012027b99030
 [<a000000100246100>] sysfs_write_file+0x220/0x300
                                sp=e000012027b9fe20 bsp=e000012027b98fd0
 [<a00000010019e2e0>] vfs_write+0x1a0/0x320
                                sp=e000012027b9fe20 bsp=e000012027b98f80
 [<a00000010019edf0>] sys_write+0x70/0xe0
                                sp=e000012027b9fe20 bsp=e000012027b98f08
 [<a00000010000a570>] ia64_trace_syscall+0xd0/0x110
                                sp=e000012027b9fe30 bsp=e000012027b98f08
 [<a000000000010720>] __start_ivt_text+0xffffffff00010720/0x400
                                sp=e000012027ba0000 bsp=e000012027b98f08
==

This comes from fs/buffer.c
==
static int __set_page_dirty(struct page *page,
                struct address_space *mapping, int warn)
{
        if (unlikely(!mapping))
                return !TestSetPageDirty(page);

        if (TestSetPageDirty(page))
                return 0;

        write_lock_irq(&mapping->tree_lock);
        if (page->mapping) {    /* Race with truncate? */
                WARN_ON_ONCE(warn && !PageUptodate(page)); ---------(*)

                if (mapping_cap_account_dirty(mapping)) {
                        __inc_zone_page_state(page, NR_FILE_DIRTY);
                        __inc_bdi_stat(mapping->backing_dev_info,
                                        BDI_RECLAIMABLE);
                        task_io_account_write(PAGE_CACHE_SIZE);
                }
                radix_tree_tag_set(&mapping->page_tree,
                                page_index(page), PAGECACHE_TAG_DIRTY);
        }
        write_unlock_irq(&mapping->tree_lock);
        __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

        return 1;
}
==
Then, "page" is not Uptodate when it reaches (*).

But, migrate_page() call path is
==
   buffer_migrate_page()
	-> lock all buffers on old pages.
	-> move buffers to newpage.
	-> migrate_page_copy(page, newpage)
		-> set_page_dirty().
	-> unlock all buffers().
==
static void migrate_page_copy(struct page *newpage, struct page *page)
{
        copy_highpage(newpage, page);
<snip>
        if (PageUptodate(page))
                SetPageUptodate(newpage);
<snip>
        if (PageDirty(page)) {
                clear_page_dirty_for_io(page);
                set_page_dirty(newpage);------------------------(**)
        }

==
Then, Uptodate() is copied before set_page_dirty(). 
So, "page" is not Uptodate and Dirty when it reaches (**)

"newpage" has buffers but not dirty and Uptodate().

>From patch comment, http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=787d2214c19bcc9b6ac48af0ce098277a801eded

"It is a bug to set a page dirty if it is not uptodate unless it has
 buffers."

__set_page_dirty() should be following ?
=
if (TestSetPageDirty(page))
	return 0;
if (PagePrivate(page))
	return 0;
if (page->mapping) {
	...
   }
=

Thanks,
-Kame





--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Mon, 14 Apr 2008, KAMEZAWA Hiroyuki wrote:

> Then, "page" is not Uptodate when it reaches (*).

Yes. Strange situation.

> But, migrate_page() call path is
> ==
>    buffer_migrate_page()
> 	-> lock all buffers on old pages.
> 	-> move buffers to newpage.
> 	-> migrate_page_copy(page, newpage)
> 		-> set_page_dirty().
> 	-> unlock all buffers().
> ==
> static void migrate_page_copy(struct page *newpage, struct page *page)
> {
>         copy_highpage(newpage, page);
> <snip>
>         if (PageUptodate(page))
>                 SetPageUptodate(newpage);

Hmmm... I guess BUG_ON(!PageUptodate) would be better here?

> <snip>
>         if (PageDirty(page)) {
>                 clear_page_dirty_for_io(page);
>                 set_page_dirty(newpage);------------------------(**)
>         }
> 
> ==
> Then, Uptodate() is copied before set_page_dirty(). 
> So, "page" is not Uptodate and Dirty when it reaches (**)

The page will be marked uptodate before we reach ** so its okay in 
general. If a page is not uptodate then we should not be getting here.

An !uptodate page is not migratable. Maybe we need to add better checking?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Mon, 14 Apr 2008 10:46:47 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> > <snip>
> >         if (PageDirty(page)) {
> >                 clear_page_dirty_for_io(page);
> >                 set_page_dirty(newpage);------------------------(**)
> >         }
> > 
> > ==
> > Then, Uptodate() is copied before set_page_dirty(). 
> > So, "page" is not Uptodate and Dirty when it reaches (**)
> 
> The page will be marked uptodate before we reach ** so its okay in 
> general. If a page is not uptodate then we should not be getting here.
> 
> An !uptodate page is not migratable. Maybe we need to add better checking?
> 
adding check is good but...

I found I can reproduce this. I'd like to chase this a bit more.

following is just a report.
==
a page which caused trouble was
- Dirty, Private, Locked (locked because of migration)
- a file cache of ext3. (maybe)

When I added following check,
==
@@ -648,6 +649,10 @@ static int unmap_and_move(new_page_t get
                        goto move_newpage;
                lock_page(page);
        }
+       /* All caches should be Uptodate before migration.*/
+       if (page_mapping(page) && !PageUptodate(page))
+               goto unlock;
+

        if (PageWriteback(page)) {
                if (!force)
==
A page offlining never ends until I run "echo 3 > /proc/sys/vm/drop_caches".

Once I use drop_caches, memory offlining works very well.
(even under some workload.) If the code I added is bad, plz blame me.

Thanks,
-Kame

 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Tue, 15 Apr 2008, KAMEZAWA Hiroyuki wrote:

> > An !uptodate page is not migratable. Maybe we need to add better checking?
> > 
> adding check is good but...
> 
> I found I can reproduce this. I'd like to chase this a bit more.
> 
> following is just a report.
> ==
> a page which caused trouble was
> - Dirty, Private, Locked (locked because of migration)
> - a file cache of ext3. (maybe)
> 
> When I added following check,
> ==
> @@ -648,6 +649,10 @@ static int unmap_and_move(new_page_t get
>                         goto move_newpage;
>                 lock_page(page);
>         }
> +       /* All caches should be Uptodate before migration.*/
> +       if (page_mapping(page) && !PageUptodate(page))
> +               goto unlock;
> +
> 
>         if (PageWriteback(page)) {
>                 if (!force)
> ==
> A page offlining never ends until I run "echo 3 > /proc/sys/vm/drop_caches".

Page migration should stop after 10 retries though? You need to set the RC 
to another value than -EAGAIN in order to avoid the retries.

if (page_mapping(page) && !PageUptodate(page)) {
	rc = -EBUSY;
	goto unlock;
}

will stop retries.

> Once I use drop_caches, memory offlining works very well.
> (even under some workload.) If the code I added is bad, plz blame me.

The retries during page migration may hold off the completion of bringing 
up a page up to date since the PageLock is repeatedly acquired. So either 
pass more pages in one go to migrate_pages() or pause once in awhile?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Tue, 15 Apr 2008 12:31:00 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:
> Page migration should stop after 10 retries though? You need to set the RC 
> to another value than -EAGAIN in order to avoid the retries.
> 
> if (page_mapping(page) && !PageUptodate(page)) {
> 	rc = -EBUSY;
> 	goto unlock;
> }
> 
> will stop retries.
> 
will try.

> > Once I use drop_caches, memory offlining works very well.
> > (even under some workload.) If the code I added is bad, plz blame me.
> 
> The retries during page migration may hold off the completion of bringing 
> up a page up to date since the PageLock is repeatedly acquired. So either 
> pass more pages in one go to migrate_pages() or pause once in awhile?
> 
yes, *may*.  But page offlining can be stopped by Ctrl-C.

What I experienced was.
==
%echo offline > /sys/device/system/memoryXXXX/state
...wait for a minute
Ctrl-C
% sync
% sync
% echo offline > /sys/device/system/memoryXXXX/state
...wait for a minute
% echo 3 > /proc/sys/vm/drop_caches
% echo offline > /sys/device/system/memoryXXXX/state
success.
==

I'll see what happens wish -EBUSY but maybe no help...

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 16 Apr 2008 09:23:34 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> What I experienced was.
> ==
> %echo offline > /sys/device/system/memoryXXXX/state
> ...wait for a minute
> Ctrl-C
> % sync
> % sync
> % echo offline > /sys/device/system/memoryXXXX/state
> ...wait for a minute
> % echo 3 > /proc/sys/vm/drop_caches
> % echo offline > /sys/device/system/memoryXXXX/state
> success.
> ==
> 
> I'll see what happens wish -EBUSY but maybe no help...
> 
Adding -EBUSY was no help. some pages seems never to be Uptodate...

== prinkt result  ==
not-up-to-date a000400144d83768 60000000000801
not-up-to-date a000400144d83d98 60000000000801
not-up-to-date a000400144d840b0 60000000000801
not-up-to-date a000400144d842a8 60000000000801
not-up-to-date a000400144d8a638 60000000000801
not-up-to-date a000400144d8f780 60000000000801
not-up-to-date a000400144d901a0 60000000000801
not-up-to-date a000400144d915e0 60000000000801
not-up-to-date a000400144d937a0 60000000000801
not-up-to-date a000400144d95d50 60000000000801
not-up-to-date a000400144d97028 60000000000801
not-up-to-date a000400144d97e38 60000000000801
not-up-to-date a000400144d99398 60000000000801
not-up-to-date a000400144d995d8 60000000000801
not-up-to-date a000400144df4e98 60000000000801
not-up-to-date a000400144df5ee8 60000000000801
not-up-to-date a000400144df7010 60000000000801
not-up-to-date a000400144df7400 60000000000801
== repeated.

Hmm...

Thanks,
-Kame


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 16 Apr 2008 11:23:41 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Wed, 16 Apr 2008 09:23:34 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > What I experienced was.
> > ==
> > %echo offline > /sys/device/system/memoryXXXX/state
> > ...wait for a minute
> > Ctrl-C
> > % sync
> > % sync
> > % echo offline > /sys/device/system/memoryXXXX/state
> > ...wait for a minute
> > % echo 3 > /proc/sys/vm/drop_caches
> > % echo offline > /sys/device/system/memoryXXXX/state
> > success.
> > ==
> > 
> > I'll see what happens wish -EBUSY but maybe no help...
> > 
> Adding -EBUSY was no help. some pages seems never to be Uptodate...
> 
BTW, a bit off-topic.
I found I can't do memory offline when I use SLAB not SLUB, 
This is because some not-migratable page are in ZONE_MOVABLE.

here is zonestat.
==
Node 1, zone  Movable
  pages free     193181
        min      190
        low      237
        high     285
        scanned  0 (a: 0 i: 0)
        spanned  196608
        present  195744
    nr_free_pages 193181
    nr_inactive  102
    nr_active    2154
    nr_anon_pages 1851
    nr_mapped    365
    nr_file_pages 407
    nr_dirty     0
    nr_writeback 0
    nr_slab_reclaimable 101
    nr_slab_unreclaimable 920
    nr_page_table_pages 0
    nr_unstable  0
    nr_bounce    0
    nr_vmscan_write 0
    numa_hit     0
    numa_miss    77029
    numa_foreign 0
    numa_interleave 0
    numa_local   0
    numa_other   76119
    pgpgin       910
        protection: (0, 0, 0)
==
Hmm, I guess SLAB's gfp_mask handling is wrong .....

Thanks,
-Kame










--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 16 Apr 2008 12:10:03 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> BTW, a bit off-topic.
> I found I can't do memory offline when I use SLAB not SLUB, 
> This is because some not-migratable page are in ZONE_MOVABLE.
> 
> here is zonestat.
> ==
> Node 1, zone  Movable
>   pages free     193181
>         min      190
>         low      237
>         high     285
>         scanned  0 (a: 0 i: 0)
>         spanned  196608
>         present  195744
>     nr_free_pages 193181
>     nr_inactive  102
>     nr_active    2154
>     nr_anon_pages 1851
>     nr_mapped    365
>     nr_file_pages 407
>     nr_dirty     0
>     nr_writeback 0
>     nr_slab_reclaimable 101
>     nr_slab_unreclaimable 920
>     nr_page_table_pages 0
>     nr_unstable  0
>     nr_bounce    0
>     nr_vmscan_write 0
>     numa_hit     0
>     numa_miss    77029
>     numa_foreign 0
>     numa_interleave 0
>     numa_local   0
>     numa_other   76119
>     pgpgin       910
>         protection: (0, 0, 0)
> ==
> Hmm, I guess SLAB's gfp_mask handling is wrong .....
> 
This was because GFP_THISNODE is buggy. I'll send a patch soon.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Mon, 14 Apr 2008 10:46:47 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> On Mon, 14 Apr 2008, KAMEZAWA Hiroyuki wrote:
> 
> > Then, "page" is not Uptodate when it reaches (*).
> 
> The page will be marked uptodate before we reach ** so its okay in
> general. If a page is not uptodate then we should not be getting here.
> 
> An !uptodate page is not migratable. Maybe we need to add better checking?
> 
> 
With tons of printk, I think I found when it happens.

Assume I use ia64/PAGE_SIZE=16k and ext3's blocksize=4k.
A page has 4 buffer_heads.

Assume that a page is not Uptodate before issuing write_begin()

At the end of writing to ext3, the kernel reaches here.
==
static int __block_commit_write(struct inode *inode, struct page *page,
                unsigned from, unsigned to)
{
    int patrial=0;

    if (!All_buffers_to_this_page_is_uptodate)
	partial = 1
    if (!partial)
        SetPageUptodate(page)
}
==
To set a page as Uptodate, all buffers must be uptodate.

But *all* buffers to this page is not necessary to be uptodate, here. 
Then, the page can be not-up-to-date after commit-write.

At page offlining, all buffers on the page seems to be marked as Uptodate
(by printk) but the page itself isn't. This seems strange.

But I don't found who set Uptodate to the buffers. 
And why page isn't up-to-date while all buffers are marked as up-to-date.

still chasing.

Thanks,
-Kame







--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Wed, 16 Apr 2008, KAMEZAWA Hiroyuki wrote:

> BTW, a bit off-topic.
> I found I can't do memory offline when I use SLAB not SLUB, 

Ah. SLAB depends on GFP_THISNODE to force the page allocator to allocate 
on a certain and since we broke that it does strange things.

SLUB lets the page allocator figure out which node to use. GFP_THISNODE is 
only used if the caller specifies it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Andrew Morton]

On Wed, 16 Apr 2008 20:00:36 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Mon, 14 Apr 2008 10:46:47 -0700 (PDT)
> Christoph Lameter <clameter@sgi.com> wrote:
> 
> > On Mon, 14 Apr 2008, KAMEZAWA Hiroyuki wrote:
> > 
> > > Then, "page" is not Uptodate when it reaches (*).
> > 
> > The page will be marked uptodate before we reach ** so its okay in
> > general. If a page is not uptodate then we should not be getting here.
> > 
> > An !uptodate page is not migratable. Maybe we need to add better checking?
> > 
> > 
> With tons of printk, I think I found when it happens.
> 
> Assume I use ia64/PAGE_SIZE=16k and ext3's blocksize=4k.
> A page has 4 buffer_heads.
> 
> Assume that a page is not Uptodate before issuing write_begin()
> 
> At the end of writing to ext3, the kernel reaches here.
> ==
> static int __block_commit_write(struct inode *inode, struct page *page,
>                 unsigned from, unsigned to)
> {
>     int patrial=0;
> 
>     if (!All_buffers_to_this_page_is_uptodate)
> 	partial = 1
>     if (!partial)
>         SetPageUptodate(page)
> }
> ==
> To set a page as Uptodate, all buffers must be uptodate.
> 
> But *all* buffers to this page is not necessary to be uptodate, here. 
> Then, the page can be not-up-to-date after commit-write.
> 
> At page offlining, all buffers on the page seems to be marked as Uptodate
> (by printk) but the page itself isn't. This seems strange.
> 
> But I don't found who set Uptodate to the buffers. 
> And why page isn't up-to-date while all buffers are marked as up-to-date.

That would imply that someone brought a buffer uptodate and didn't mark the
page uptodate.  That can happen if a read reads the buffer from disk or
memsets all of it.  Or if a write memsets all of it, or does
copy_from_user() into all of it.

> still chasing.

umm..

If you had some code which does

	pread(fd, buf, 1, 0);
	pread(fd, buf, 1, 4096);
	pread(fd, buf, 1, 8192);
	pread(fd, buf, 1, 12288);

then I'd expect that each read would read a single buffer so we end up with
four uptodate buffers, but nobody brings the entire page uptodate.

readahead will hide this most of the time by reading entire pages, but if
for some reason readahead has collapsed the window to zero then it could
happen.

I'd expect that you could reproduce this by disabling readahead with
fadvise(POSIX_FADV_RANDOM) and then issuing the above four reads.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 16 Apr 2008 11:36:42 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> > To set a page as Uptodate, all buffers must be uptodate.
> > 
> > But *all* buffers to this page is not necessary to be uptodate, here. 
> > Then, the page can be not-up-to-date after commit-write.
> > 
> > At page offlining, all buffers on the page seems to be marked as Uptodate
> > (by printk) but the page itself isn't. This seems strange.
> > 
> > But I don't found who set Uptodate to the buffers. 
> > And why page isn't up-to-date while all buffers are marked as up-to-date.
> 
> That would imply that someone brought a buffer uptodate and didn't mark the
> page uptodate.  That can happen if a read reads the buffer from disk or
> memsets all of it.  Or if a write memsets all of it, or does
> copy_from_user() into all of it.
> 
ok, I'll pay attention to codes for read.

> > still chasing.
> 
> umm..
> 
> If you had some code which does
> 
> 	pread(fd, buf, 1, 0);
> 	pread(fd, buf, 1, 4096);
> 	pread(fd, buf, 1, 8192);
> 	pread(fd, buf, 1, 12288);
> 
> then I'd expect that each read would read a single buffer so we end up with
> four uptodate buffers, but nobody brings the entire page uptodate.
> 
> readahead will hide this most of the time by reading entire pages, but if
> for some reason readahead has collapsed the window to zero then it could
> happen.
> 
> I'd expect that you could reproduce this by disabling readahead with
> fadvise(POSIX_FADV_RANDOM) and then issuing the above four reads.
> 
Thank you for advice. I'll try.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (possible in migration ?)

[KAMEZAWA Hiroyuki]

On Thu, 17 Apr 2008 09:19:30 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > I'd expect that you could reproduce this by disabling readahead with
> > fadvise(POSIX_FADV_RANDOM) and then issuing the above four reads.
> > 
> Thank you for advice. I'll try.
> 
(Added lkml to CC:)

What happens:
  When I do memory offline on ia64/NUMA box, __set_page_dirty_buffers() printed
  out WARNINGS because the page under migration is not up-to-date.

Following is my investigation.

Assume 16k pages / 4 buffers of 4096bytes block (ext3).
4 buffers on a page of ext3.

At page offlining, we can find a page which is not up-to-date.
But all buffers of the page seems up-to-date.

buffers on a page by prink().
    buffer 0, block_nr= some vaule, state= BH_uptodate | BH_Req| BH_Mapped
    buffer 1, block_nr= -1,         state= BH_uptodate
    buffer 2, block_nr= -1,         state= BH_uptodate
    buffer 3, block_nr= -1,         state= BH_uptodate

It seems no I/O for 3 buffers. It's because the page is the last page of inode
and blocks for buffer[1,2,3] is not assgined.
(maybe BH_uptodate is set by block_write_full_page().

Adding below check can hide the warning....but I can't say this is correct.
Can we set this page dirty silently in this case ? 

===
+
+static int check_fragment_page(struct page *page, struct address_space *mapping
)
+{
+       struct inode *inode = mapping->host;
+       unsigned long lastblock, coverblock;
+
+       if (!page_has_buffers(page))
+               return 0;
+
+       lastblock = (i_size_read(inode) - 1) >> inode->i_blkbits;
+       coverblock = (page->index + 1) << (PAGE_SHIFT - inode->i_blkbits);
+
+       return coverblock > lastblock;
+}
+
+
+
 static int __set_page_dirty(struct page *page,
                struct address_space *mapping, int warn)
 {
@@ -717,7 +734,9 @@ static int __set_page_dirty(struct page

        write_lock_irq(&mapping->tree_lock);
        if (page->mapping) {    /* Race with truncate? */
-               WARN_ON_ONCE(warn && !PageUptodate(page));
+               WARN_ON_ONCE(warn
+                            && !PageUptodate(page)
+                            && !check_fragment_page(page, mapping));

                if (mapping_cap_account_dirty(mapping)) {
                        __inc_zone_page_state(page, NR_FILE_DIRTY);
==

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (possible in migration ?)

[Andrew Morton]

On Thu, 17 Apr 2008 15:38:18 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Thu, 17 Apr 2008 09:19:30 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > > I'd expect that you could reproduce this by disabling readahead with
> > > fadvise(POSIX_FADV_RANDOM) and then issuing the above four reads.
> > > 
> > Thank you for advice. I'll try.
> > 
> (Added lkml to CC:)
> 
> What happens:
>   When I do memory offline on ia64/NUMA box, __set_page_dirty_buffers() printed
>   out WARNINGS because the page under migration is not up-to-date.

The warning is in __set_page_dirty().

> Following is my investigation.
> 
> Assume 16k pages / 4 buffers of 4096bytes block (ext3).
> 4 buffers on a page of ext3.
> 
> At page offlining, we can find a page which is not up-to-date.
> But all buffers of the page seems up-to-date.
> 
> buffers on a page by prink().
>     buffer 0, block_nr= some vaule, state= BH_uptodate | BH_Req| BH_Mapped
>     buffer 1, block_nr= -1,         state= BH_uptodate
>     buffer 2, block_nr= -1,         state= BH_uptodate
>     buffer 3, block_nr= -1,         state= BH_uptodate
> 
> It seems no I/O for 3 buffers. It's because the page is the last page of inode
> and blocks for buffer[1,2,3] is not assgined.
> (maybe BH_uptodate is set by block_write_full_page().
> 
> Adding below check can hide the warning....but I can't say this is correct.
> Can we set this page dirty silently in this case ? 
> 
> ===
> +
> +static int check_fragment_page(struct page *page, struct address_space *mapping
> )
> +{
> +       struct inode *inode = mapping->host;
> +       unsigned long lastblock, coverblock;
> +
> +       if (!page_has_buffers(page))
> +               return 0;
> +
> +       lastblock = (i_size_read(inode) - 1) >> inode->i_blkbits;
> +       coverblock = (page->index + 1) << (PAGE_SHIFT - inode->i_blkbits);
> +
> +       return coverblock > lastblock;
> +}
> +
> +
> +
>  static int __set_page_dirty(struct page *page,
>                 struct address_space *mapping, int warn)
>  {
> @@ -717,7 +734,9 @@ static int __set_page_dirty(struct page
> 
>         write_lock_irq(&mapping->tree_lock);
>         if (page->mapping) {    /* Race with truncate? */
> -               WARN_ON_ONCE(warn && !PageUptodate(page));
> +               WARN_ON_ONCE(warn
> +                            && !PageUptodate(page)
> +                            && !check_fragment_page(page, mapping));
> 
>                 if (mapping_cap_account_dirty(mapping)) {
>                         __inc_zone_page_state(page, NR_FILE_DIRTY);
> ==

The warning is just wrong, I think.  We don't nowmally hit it because
write() will use mark_buffer_dirty() which supresses the warning and mmaped
pages are uptodate.

Nick?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (possible in migration ?)

[KAMEZAWA Hiroyuki]

On Wed, 16 Apr 2008 23:43:03 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 17 Apr 2008 15:38:18 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > On Thu, 17 Apr 2008 09:19:30 +0900
> > KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > > > I'd expect that you could reproduce this by disabling readahead with
> > > > fadvise(POSIX_FADV_RANDOM) and then issuing the above four reads.
> > > > 
> > > Thank you for advice. I'll try.
> > > 
> > (Added lkml to CC:)
> > 
> > What happens:
> >   When I do memory offline on ia64/NUMA box, __set_page_dirty_buffers() printed
> >   out WARNINGS because the page under migration is not up-to-date.
> 
> The warning is in __set_page_dirty().
> 
Sorry, __set_page_dirty() in fs/buffer.c 

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Wed, Apr 16, 2008 at 11:36:42AM -0700, Andrew Morton wrote:
> On Wed, 16 Apr 2008 20:00:36 +0900
> > With tons of printk, I think I found when it happens.
> > 
> > Assume I use ia64/PAGE_SIZE=16k and ext3's blocksize=4k.
> > A page has 4 buffer_heads.
> > 
> > Assume that a page is not Uptodate before issuing write_begin()
> > 
> > At the end of writing to ext3, the kernel reaches here.
> > ==
> > static int __block_commit_write(struct inode *inode, struct page *page,
> >                 unsigned from, unsigned to)
> > {
> >     int patrial=0;
> > 
> >     if (!All_buffers_to_this_page_is_uptodate)
> > 	partial = 1
> >     if (!partial)
> >         SetPageUptodate(page)
> > }
> > ==
> > To set a page as Uptodate, all buffers must be uptodate.
> > 
> > But *all* buffers to this page is not necessary to be uptodate, here. 
> > Then, the page can be not-up-to-date after commit-write.
> > 
> > At page offlining, all buffers on the page seems to be marked as Uptodate
> > (by printk) but the page itself isn't. This seems strange.
> > 
> > But I don't found who set Uptodate to the buffers. 
> > And why page isn't up-to-date while all buffers are marked as up-to-date.
> 
> That would imply that someone brought a buffer uptodate and didn't mark the
> page uptodate.  That can happen if a read reads the buffer from disk or
> memsets all of it.  Or if a write memsets all of it, or does
> copy_from_user() into all of it.
> 
> > still chasing.
> 
> umm..
> 
> If you had some code which does
> 
> 	pread(fd, buf, 1, 0);
> 	pread(fd, buf, 1, 4096);
> 	pread(fd, buf, 1, 8192);
> 	pread(fd, buf, 1, 12288);
> 
> then I'd expect that each read would read a single buffer so we end up with
> four uptodate buffers, but nobody brings the entire page uptodate.

The generic read path AFAIK doesn't do partial buffers.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Mon, Apr 14, 2008 at 02:58:06PM +0900, KAMEZAWA Hiroyuki wrote:
> In 2.6.25-rc8-mm2.
> 
> I saw below warning at memory offlining. please help.
> (ia64/NUMA box with ext3 file system. Almost all memory are free memory.)
> ==
> 
> localhost.localdomain login: ------------[ cut here ]------------
> WARNING: at fs/buffer.c:720 __set_page_dirty+0x330/0x360()
> Modules linked in: ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge ipv6 ipmi_watchdog mptctl ipmi_devintf ipmi_si ipmi_msghandler vfat fat dm_multipath parport_pc lp parport sg tg3 e100 mii button shpchp dm_snapshot dm_zero dm_mirror dm_log dm_mod usb_storage mptspi mptscsih scsi_transport_spi mptbase sd_mod scsi_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd [last unloaded: ipmi_watchdog]
> 
> Call Trace:
>  [<a000000100015220>] show_stack+0x80/0xa0
>                                 sp=e000012027b9fae0 bsp=e000012027b99398
>  [<a000000100015270>] dump_stack+0x30/0x60
>                                 sp=e000012027b9fcb0 bsp=e000012027b99380
>  [<a000000100089ed0>] warn_on_slowpath+0x90/0xe0
>                                 sp=e000012027b9fcb0 bsp=e000012027b99358
>  [<a0000001001f8b10>] __set_page_dirty+0x330/0x360
>                                 sp=e000012027b9fda0 bsp=e000012027b99328
>  [<a0000001001ffb90>] __set_page_dirty_buffers+0xd0/0x280
>                                 sp=e000012027b9fda0 bsp=e000012027b992f8
>  [<a00000010012fec0>] set_page_dirty+0xc0/0x260
>                                 sp=e000012027b9fda0 bsp=e000012027b992d0
>  [<a000000100195670>] migrate_page_copy+0x5d0/0x5e0
>                                 sp=e000012027b9fda0 bsp=e000012027b992a8
>  [<a000000100197840>] buffer_migrate_page+0x2e0/0x3c0
>                                 sp=e000012027b9fda0 bsp=e000012027b99260
>  [<a000000100195eb0>] migrate_pages+0x770/0xe00
>                                 sp=e000012027b9fda0 bsp=e000012027b991a8
>  [<a000000100191250>] offline_pages+0x6f0/0xa20
>                                 sp=e000012027b9fdf0 bsp=e000012027b99118
>  [<a00000010006b1f0>] remove_memory+0x30/0x60
>                                 sp=e000012027b9fe20 bsp=e000012027b990f0
>  [<a000000100482c50>] memory_block_change_state+0x390/0x400
>                                 sp=e000012027b9fe20 bsp=e000012027b990a0
>  [<a000000100483720>] store_mem_state+0x1e0/0x200
>                                 sp=e000012027b9fe20 bsp=e000012027b99068
>  [<a0000001004718a0>] sysdev_store+0x60/0xa0
>                                 sp=e000012027b9fe20 bsp=e000012027b99030
>  [<a000000100246100>] sysfs_write_file+0x220/0x300
>                                 sp=e000012027b9fe20 bsp=e000012027b98fd0
>  [<a00000010019e2e0>] vfs_write+0x1a0/0x320
>                                 sp=e000012027b9fe20 bsp=e000012027b98f80
>  [<a00000010019edf0>] sys_write+0x70/0xe0
>                                 sp=e000012027b9fe20 bsp=e000012027b98f08
>  [<a00000010000a570>] ia64_trace_syscall+0xd0/0x110
>                                 sp=e000012027b9fe30 bsp=e000012027b98f08
>  [<a000000000010720>] __start_ivt_text+0xffffffff00010720/0x400
>                                 sp=e000012027ba0000 bsp=e000012027b98f08
> ==
> 
> This comes from fs/buffer.c
> ==
> static int __set_page_dirty(struct page *page,
>                 struct address_space *mapping, int warn)
> {
>         if (unlikely(!mapping))
>                 return !TestSetPageDirty(page);
> 
>         if (TestSetPageDirty(page))
>                 return 0;
> 
>         write_lock_irq(&mapping->tree_lock);
>         if (page->mapping) {    /* Race with truncate? */
>                 WARN_ON_ONCE(warn && !PageUptodate(page)); ---------(*)
> 
>                 if (mapping_cap_account_dirty(mapping)) {
>                         __inc_zone_page_state(page, NR_FILE_DIRTY);
>                         __inc_bdi_stat(mapping->backing_dev_info,
>                                         BDI_RECLAIMABLE);
>                         task_io_account_write(PAGE_CACHE_SIZE);
>                 }
>                 radix_tree_tag_set(&mapping->page_tree,
>                                 page_index(page), PAGECACHE_TAG_DIRTY);
>         }
>         write_unlock_irq(&mapping->tree_lock);
>         __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> 
>         return 1;
> }
> ==
> Then, "page" is not Uptodate when it reaches (*).
> 
> But, migrate_page() call path is
> ==
>    buffer_migrate_page()
> 	-> lock all buffers on old pages.
> 	-> move buffers to newpage.
> 	-> migrate_page_copy(page, newpage)
> 		-> set_page_dirty().
> 	-> unlock all buffers().
> ==
> static void migrate_page_copy(struct page *newpage, struct page *page)
> {
>         copy_highpage(newpage, page);
> <snip>
>         if (PageUptodate(page))
>                 SetPageUptodate(newpage);
> <snip>
>         if (PageDirty(page)) {
>                 clear_page_dirty_for_io(page);
>                 set_page_dirty(newpage);------------------------(**)
>         }

As far as I can see, migration just wants to transfer the PG_dirty state
to newpage, not actually "mark newpage as entirely dirty". So this should
be something like redirty_page_for_writepage here (without the wbc part,
__set_page_dirty_nobuffers should be sufficient as we are mm/ and
presumably know what we're doing).


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Mon, Apr 14, 2008 at 10:46:47AM -0700, Christoph Lameter wrote:
> On Mon, 14 Apr 2008, KAMEZAWA Hiroyuki wrote:
> 
> > Then, "page" is not Uptodate when it reaches (*).
> 
> Yes. Strange situation.
> 
> > But, migrate_page() call path is
> > ==
> >    buffer_migrate_page()
> > 	-> lock all buffers on old pages.
> > 	-> move buffers to newpage.
> > 	-> migrate_page_copy(page, newpage)
> > 		-> set_page_dirty().
> > 	-> unlock all buffers().
> > ==
> > static void migrate_page_copy(struct page *newpage, struct page *page)
> > {
> >         copy_highpage(newpage, page);
> > <snip>
> >         if (PageUptodate(page))
> >                 SetPageUptodate(newpage);
> 
> Hmmm... I guess BUG_ON(!PageUptodate) would be better here?
> 
> > <snip>
> >         if (PageDirty(page)) {
> >                 clear_page_dirty_for_io(page);
> >                 set_page_dirty(newpage);------------------------(**)
> >         }
> > 
> > ==
> > Then, Uptodate() is copied before set_page_dirty(). 
> > So, "page" is not Uptodate and Dirty when it reaches (**)
> 
> The page will be marked uptodate before we reach ** so its okay in 
> general. If a page is not uptodate then we should not be getting here.
> 
> An !uptodate page is not migratable. Maybe we need to add better checking?

Why is an !uptodate page not migrateable, and where are you testing to
prevent that?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Tue, 22 Apr 2008 06:52:05 +0200
Nick Piggin <npiggin@suse.de> wrote:
> > I guess BUG_ON(!PageUptodate) would be better here?
> > 
> > > <snip>
> > >         if (PageDirty(page)) {
> > >                 clear_page_dirty_for_io(page);
> > >                 set_page_dirty(newpage);------------------------(**)
> > >         }
> > > 
> > > ==
> > > Then, Uptodate() is copied before set_page_dirty(). 
> > > So, "page" is not Uptodate and Dirty when it reaches (**)
> > 
> > The page will be marked uptodate before we reach ** so its okay in 
> > general. If a page is not uptodate then we should not be getting here.
> > 
> > An !uptodate page is not migratable. Maybe we need to add better checking?
> 
> Why is an !uptodate page not migrateable, and where are you testing to
> prevent that?
> 

I'm sorry if I don't understand correctly, in usual, can I consider
!PageUptodate() page is under I/O or some unstable state ?
If so, migration is danger.
And set_page_dirty() shown WARNING when I migrated a dirty and not-up-to-date
page. To avoid WARNING, a not-up-to-date page should not be migratable.


I tested PageUptodate() in
==
static int unmap_and_move(new_page_t get_new_page, unsigned long private,
                        struct page *page, int force)
{
    <snip>
        if (TestSetPageLocked(page)) {
                if (!force)
                        goto move_newpage;
                lock_page(page);
        }
      (*) here
....
}
==
and the page never becomes Uptodate.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Tue, Apr 22, 2008 at 04:56:08PM +0900, KAMEZAWA Hiroyuki wrote:
> On Tue, 22 Apr 2008 06:52:05 +0200
> Nick Piggin <npiggin@suse.de> wrote:
> > > I guess BUG_ON(!PageUptodate) would be better here?
> > > 
> > > > <snip>
> > > >         if (PageDirty(page)) {
> > > >                 clear_page_dirty_for_io(page);
> > > >                 set_page_dirty(newpage);------------------------(**)
> > > >         }
> > > > 
> > > > ==
> > > > Then, Uptodate() is copied before set_page_dirty(). 
> > > > So, "page" is not Uptodate and Dirty when it reaches (**)
> > > 
> > > The page will be marked uptodate before we reach ** so its okay in 
> > > general. If a page is not uptodate then we should not be getting here.
> > > 
> > > An !uptodate page is not migratable. Maybe we need to add better checking?
> > 
> > Why is an !uptodate page not migrateable, and where are you testing to
> > prevent that?
> > 
> 
> I'm sorry if I don't understand correctly, in usual, can I consider
> !PageUptodate() page is under I/O or some unstable state ?

No, it need not be under IO or in some unstable state. Christoph just
said that migration can't handle !uptodate pages, and I'm very
curious as to why not, and what is in place to prevent that from
happening.


> If so, migration is danger.
> And set_page_dirty() shown WARNING when I migrated a dirty and not-up-to-date
> page. To avoid WARNING, a not-up-to-date page should not be migratable.
> 
> 
> I tested PageUptodate() in
> ==
> static int unmap_and_move(new_page_t get_new_page, unsigned long private,
>                         struct page *page, int force)
> {
>     <snip>
>         if (TestSetPageLocked(page)) {
>                 if (!force)
>                         goto move_newpage;
>                 lock_page(page);
>         }
>       (*) here
> ....
> }
> ==
> and the page never becomes Uptodate.
> 
> Thanks,
> -Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Tue, 22 Apr 2008 11:43:52 +0200
Nick Piggin <npiggin@suse.de> wrote:
> > > > > <snip>
> > > > >         if (PageDirty(page)) {
> > > > >                 clear_page_dirty_for_io(page);
> > > > >                 set_page_dirty(newpage);------------------------(**)
> > > > >         }
> > > > > 
> > > > > ==
> > > > > Then, Uptodate() is copied before set_page_dirty(). 
> > > > > So, "page" is not Uptodate and Dirty when it reaches (**)
> > > > 
> > > > The page will be marked uptodate before we reach ** so its okay in 
> > > > general. If a page is not uptodate then we should not be getting here.
> > > > 
> > > > An !uptodate page is not migratable. Maybe we need to add better checking?
> > > 
> > > Why is an !uptodate page not migrateable, and where are you testing to
> > > prevent that?
> > > 
> > 
> > I'm sorry if I don't understand correctly, in usual, can I consider
> > !PageUptodate() page is under I/O or some unstable state ?
> 
> No, it need not be under IO or in some unstable state. Christoph just
> said that migration can't handle !uptodate pages, and I'm very
> curious as to why not, and what is in place to prevent that from
> happening.
> 
I myself have no idea about "why". I thought that page_lock() can
keep us from migrating a page under I/O before I found this WARNING.

Thanks
-Kame


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Tue, 22 Apr 2008, Nick Piggin wrote:

> No, it need not be under IO or in some unstable state. Christoph just
> said that migration can't handle !uptodate pages, and I'm very
> curious as to why not, and what is in place to prevent that from
> happening.

We just assumed that the page was in an unstable state since it was under 
I/O. Maybe you can give us the correct definition?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Tue, Apr 22, 2008 at 12:16:07PM -0700, Christoph Lameter wrote:
> On Tue, 22 Apr 2008, Nick Piggin wrote:
> 
> > No, it need not be under IO or in some unstable state. Christoph just
> > said that migration can't handle !uptodate pages, and I'm very
> > curious as to why not, and what is in place to prevent that from
> > happening.
> 
> We just assumed that the page was in an unstable state since it was under 
> I/O.

A !uptodate page isn't necessarily under IO. But even if you are assuming
it is in an unstable state, I don't see any code that would prevent it
from trying to migrate an !uptodate page.

Anyway, here is my proposed (uncompiled, untested) fix. Score 1 for my
buffer invariant checks if I'm right ;)

---

KAMEZAWA Hiroyuki found a warning message in the buffer dirtying code that
is coming from page migration caller.

WARNING: at fs/buffer.c:720 __set_page_dirty+0x330/0x360()
Call Trace:
 [<a000000100015220>] show_stack+0x80/0xa0
 [<a000000100015270>] dump_stack+0x30/0x60
 [<a000000100089ed0>] warn_on_slowpath+0x90/0xe0
 [<a0000001001f8b10>] __set_page_dirty+0x330/0x360
 [<a0000001001ffb90>] __set_page_dirty_buffers+0xd0/0x280
 [<a00000010012fec0>] set_page_dirty+0xc0/0x260
 [<a000000100195670>] migrate_page_copy+0x5d0/0x5e0
 [<a000000100197840>] buffer_migrate_page+0x2e0/0x3c0
 [<a000000100195eb0>] migrate_pages+0x770/0xe00

What was happening is that migrate_page_copy wants to transfer the PG_dirty
bit from old page to new page, so what it would do is set_page_dirty(newpage).
However set_page_dirty() is used to set the entire page dirty, wheras in
this case, only part of the page was dirty, and it also was not uptodate.

Marking the whole page dirty with set_page_dirty would lead to corruption or
unresolvable conditions -- a dirty && !uptodate page and dirty && !uptodate
buffers.

Possibly we could just ClearPageDirty(oldpage); SetPageDirty(newpage);
however in the interests of keeping the change minimal...

Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/mm/migrate.c
===================================================================
--- linux-2.6.orig/mm/migrate.c
+++ linux-2.6/mm/migrate.c
@@ -383,7 +383,14 @@ static void migrate_page_copy(struct pag
 
 	if (PageDirty(page)) {
 		clear_page_dirty_for_io(page);
-		set_page_dirty(newpage);
+		/*
+		 * Want to mark the page and the radix tree as dirty, and
+		 * redo the accounting that clear_page_dirty_for_io undid,
+		 * but we can't use set_page_dirty because that function
+		 * is actually a signal that all of the page has become dirty.
+		 * Wheras only part of our page may be dirty.
+		 */
+		__set_page_dirty_nobuffers(newpage);
  	}
 
 #ifdef CONFIG_SWAP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 23 Apr 2008 02:48:04 +0200
Nick Piggin <npiggin@suse.de> wrote:

> On Tue, Apr 22, 2008 at 12:16:07PM -0700, Christoph Lameter wrote:
> > On Tue, 22 Apr 2008, Nick Piggin wrote:
> > 
> > > No, it need not be under IO or in some unstable state. Christoph just
> > > said that migration can't handle !uptodate pages, and I'm very
> > > curious as to why not, and what is in place to prevent that from
> > > happening.
> > 
> > We just assumed that the page was in an unstable state since it was under 
> > I/O.
> 
> A !uptodate page isn't necessarily under IO. But even if you are assuming
> it is in an unstable state, I don't see any code that would prevent it
> from trying to migrate an !uptodate page.
> 
> Anyway, here is my proposed (uncompiled, untested) fix. Score 1 for my
> buffer invariant checks if I'm right ;)
> 
Thank you!, I'll test this.

Regards,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 23 Apr 2008 02:48:04 +0200
Nick Piggin <npiggin@suse.de> wrote:
> KAMEZAWA Hiroyuki found a warning message in the buffer dirtying code that
> is coming from page migration caller.
> 
> WARNING: at fs/buffer.c:720 __set_page_dirty+0x330/0x360()
> Call Trace:
>  [<a000000100015220>] show_stack+0x80/0xa0
>  [<a000000100015270>] dump_stack+0x30/0x60
>  [<a000000100089ed0>] warn_on_slowpath+0x90/0xe0
>  [<a0000001001f8b10>] __set_page_dirty+0x330/0x360
>  [<a0000001001ffb90>] __set_page_dirty_buffers+0xd0/0x280
>  [<a00000010012fec0>] set_page_dirty+0xc0/0x260
>  [<a000000100195670>] migrate_page_copy+0x5d0/0x5e0
>  [<a000000100197840>] buffer_migrate_page+0x2e0/0x3c0
>  [<a000000100195eb0>] migrate_pages+0x770/0xe00
> 
> What was happening is that migrate_page_copy wants to transfer the PG_dirty
> bit from old page to new page, so what it would do is set_page_dirty(newpage).
> However set_page_dirty() is used to set the entire page dirty, wheras in
> this case, only part of the page was dirty, and it also was not uptodate.
> 
> Marking the whole page dirty with set_page_dirty would lead to corruption or
> unresolvable conditions -- a dirty && !uptodate page and dirty && !uptodate
> buffers.
> 
> Possibly we could just ClearPageDirty(oldpage); SetPageDirty(newpage);
> however in the interests of keeping the change minimal...
> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>

Tested and seems to work well. thank you!

BTW, can I ask a question for understanding this change ?

==this check==
 WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));

in __set_page_dirty_nobuffers() seems to check "the page should have buffer or
be up-to-date when it calls this function."

When it comes to __set_page_dirty() (in fs/buffer.c)
== this check==
 WARN_ON_ONCE(warn && !PageUptodate(page));

is used and doesn't see page has buffers or not.
What's difference between two functions's condition for WARNING ?

-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Wed, Apr 23, 2008 at 11:41:07AM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 23 Apr 2008 02:48:04 +0200
> Nick Piggin <npiggin@suse.de> wrote:
> > KAMEZAWA Hiroyuki found a warning message in the buffer dirtying code that
> > is coming from page migration caller.
> > 
> > WARNING: at fs/buffer.c:720 __set_page_dirty+0x330/0x360()
> > Call Trace:
> >  [<a000000100015220>] show_stack+0x80/0xa0
> >  [<a000000100015270>] dump_stack+0x30/0x60
> >  [<a000000100089ed0>] warn_on_slowpath+0x90/0xe0
> >  [<a0000001001f8b10>] __set_page_dirty+0x330/0x360
> >  [<a0000001001ffb90>] __set_page_dirty_buffers+0xd0/0x280
> >  [<a00000010012fec0>] set_page_dirty+0xc0/0x260
> >  [<a000000100195670>] migrate_page_copy+0x5d0/0x5e0
> >  [<a000000100197840>] buffer_migrate_page+0x2e0/0x3c0
> >  [<a000000100195eb0>] migrate_pages+0x770/0xe00
> > 
> > What was happening is that migrate_page_copy wants to transfer the PG_dirty
> > bit from old page to new page, so what it would do is set_page_dirty(newpage).
> > However set_page_dirty() is used to set the entire page dirty, wheras in
> > this case, only part of the page was dirty, and it also was not uptodate.
> > 
> > Marking the whole page dirty with set_page_dirty would lead to corruption or
> > unresolvable conditions -- a dirty && !uptodate page and dirty && !uptodate
> > buffers.
> > 
> > Possibly we could just ClearPageDirty(oldpage); SetPageDirty(newpage);
> > however in the interests of keeping the change minimal...
> > 
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> 
> Tested and seems to work well. thank you!

Thanks very much!

 
> BTW, can I ask a question for understanding this change ?
> 
> ==this check==
>  WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
> 
> in __set_page_dirty_nobuffers() seems to check "the page should have buffer or
> be up-to-date when it calls this function."
> 
> When it comes to __set_page_dirty() (in fs/buffer.c)
> == this check==
>  WARN_ON_ONCE(warn && !PageUptodate(page));
> 
> is used and doesn't see page has buffers or not.
> What's difference between two functions's condition for WARNING ?

Yes, __set_page_dirty_nobuffers confusingly can also be called for pages
with buffers. In the case that the page has buffers (or any other private
metadata), then __set_page_dirty_nobuffers does not have enough information
to know whether the page should be uptodate before being marked dirty.

In the __set_page_dirty case in fs/buffer.c, we _do_ know that the page
has buffers and that it would be wrong to have a situation where the
page is !uptodate at this point.

Is that clear? Or have I explained it poorly?

Thanks,
Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 23 Apr 2008 04:53:58 +0200
Nick Piggin <npiggin@suse.de> wrote:

> > BTW, can I ask a question for understanding this change ?
> > 
> > ==this check==
> >  WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
> > 
> > in __set_page_dirty_nobuffers() seems to check "the page should have buffer or
> > be up-to-date when it calls this function."
> > 
> > When it comes to __set_page_dirty() (in fs/buffer.c)
> > == this check==
> >  WARN_ON_ONCE(warn && !PageUptodate(page));
> > 
> > is used and doesn't see page has buffers or not.
> > What's difference between two functions's condition for WARNING ?
> 
> Yes, __set_page_dirty_nobuffers confusingly can also be called for pages
> with buffers. In the case that the page has buffers (or any other private
> metadata), then __set_page_dirty_nobuffers does not have enough information
> to know whether the page should be uptodate before being marked dirty.
> 
> In the __set_page_dirty case in fs/buffer.c, we _do_ know that the page
> has buffers and that it would be wrong to have a situation where the
> page is !uptodate at this point.
> 
> Is that clear? Or have I explained it poorly?
> 

Hmm...does that comes from difference of the purpose of the functions ?

Is this correct ?
==
set_page_dirty_buffers() (in fs/buffer.c) makes a page and _all_ buffers on it
dirty. So, a page *must* be up-to-date when it calls set_page_dirty_buffers().
This is used for mapped pages or some callers which requires the whole
page containes valid data.

In set_page_dirty_nobuffers()case , it just makes a page to be dirty. We can't
see whether a page is really up-to-date or not when PagePrivate(page) &&
!PageUptodate(page). This is used for a page which contains some data
to be written out. (part of buffers contains data.)

==

Thank you.

-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Wed, Apr 23, 2008 at 12:44:25PM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 23 Apr 2008 04:53:58 +0200
> Nick Piggin <npiggin@suse.de> wrote:
> 
> > > BTW, can I ask a question for understanding this change ?
> > > 
> > > ==this check==
> > >  WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
> > > 
> > > in __set_page_dirty_nobuffers() seems to check "the page should have buffer or
> > > be up-to-date when it calls this function."
> > > 
> > > When it comes to __set_page_dirty() (in fs/buffer.c)
> > > == this check==
> > >  WARN_ON_ONCE(warn && !PageUptodate(page));
> > > 
> > > is used and doesn't see page has buffers or not.
> > > What's difference between two functions's condition for WARNING ?
> > 
> > Yes, __set_page_dirty_nobuffers confusingly can also be called for pages
> > with buffers. In the case that the page has buffers (or any other private
> > metadata), then __set_page_dirty_nobuffers does not have enough information
> > to know whether the page should be uptodate before being marked dirty.
> > 
> > In the __set_page_dirty case in fs/buffer.c, we _do_ know that the page
> > has buffers and that it would be wrong to have a situation where the
> > page is !uptodate at this point.
> > 
> > Is that clear? Or have I explained it poorly?
> > 
> 
> Hmm...does that comes from difference of the purpose of the functions ?

Yes, well sometimes __set_page_dirty_nobuffers is actually called into
for a page which does have buffers or some private data (eg. via
redirty_page_for_writepage). If it was only called for pages that really
have no buffers, it could simply be WARN_ON(!PageUptodate(page))

 
> Is this correct ?
> ==
> set_page_dirty_buffers() (in fs/buffer.c) makes a page and _all_ buffers on it
> dirty. So, a page *must* be up-to-date when it calls set_page_dirty_buffers().
> This is used for mapped pages or some callers which requires the whole
> page containes valid data.
> 
> In set_page_dirty_nobuffers()case , it just makes a page to be dirty. We can't
> see whether a page is really up-to-date or not when PagePrivate(page) &&
> !PageUptodate(page). This is used for a page which contains some data
> to be written out. (part of buffers contains data.)
> 
> ==

Yes I think you have it correct. 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Wed, 23 Apr 2008, Nick Piggin wrote:

> In the __set_page_dirty case in fs/buffer.c, we _do_ know that the page
> has buffers and that it would be wrong to have a situation where the
> page is !uptodate at this point.
> 
> Is that clear? Or have I explained it poorly?

In other words __set_page_dirty sets the page dirty and only warns
if the page has no buffers and is not uptodate.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Wed, 23 Apr 2008, KAMEZAWA Hiroyuki wrote:

> In set_page_dirty_nobuffers()case , it just makes a page to be dirty. We can't
> see whether a page is really up-to-date or not when PagePrivate(page) &&
> !PageUptodate(page). This is used for a page which contains some data
> to be written out. (part of buffers contains data.)

So its safe to migrate a !Uptodate page if it contains buffers? Note that 
the migration code reattaches the buffer to the new page in 
buffer_migrate_page().



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 23 Apr 2008 17:28:09 +0200
Nick Piggin <npiggin@suse.de> wrote:
> > Is this correct ?
> > ==
> > set_page_dirty_buffers() (in fs/buffer.c) makes a page and _all_ buffers on it
> > dirty. So, a page *must* be up-to-date when it calls set_page_dirty_buffers().
> > This is used for mapped pages or some callers which requires the whole
> > page containes valid data.
> > 
> > In set_page_dirty_nobuffers()case , it just makes a page to be dirty. We can't
> > see whether a page is really up-to-date or not when PagePrivate(page) &&
> > !PageUptodate(page). This is used for a page which contains some data
> > to be written out. (part of buffers contains data.)
> > 
> > ==
> 
> Yes I think you have it correct. 
> 

Thank you for kindly explanation.

Regards,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 23 Apr 2008 10:50:33 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> On Wed, 23 Apr 2008, KAMEZAWA Hiroyuki wrote:
> 
> > In set_page_dirty_nobuffers()case , it just makes a page to be dirty. We can't
> > see whether a page is really up-to-date or not when PagePrivate(page) &&
> > !PageUptodate(page). This is used for a page which contains some data
> > to be written out. (part of buffers contains data.)
> 
> So its safe to migrate a !Uptodate page if it contains buffers? Note that 
> the migration code reattaches the buffer to the new page in 
> buffer_migrate_page().
> 
I think it's safe because it reattaches buffers as you explained.

under migration
1. a page is locked.
2. buffers are reattached.
3. a PG_writeback page are not migrated.

So, it seems safe.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Wed, Apr 23, 2008 at 10:47:04AM -0700, Christoph Lameter wrote:
> On Wed, 23 Apr 2008, Nick Piggin wrote:
> 
> > In the __set_page_dirty case in fs/buffer.c, we _do_ know that the page
> > has buffers and that it would be wrong to have a situation where the
> > page is !uptodate at this point.
> > 
> > Is that clear? Or have I explained it poorly?
> 
> In other words __set_page_dirty sets the page dirty and only warns
> if the page has no buffers and is not uptodate.

That's what __set_page_dirty_nobuffers does.

__set_page_dirty, when called from __set_page_dirty_buffers, will warn
even if the page does have buffers. That's because from that path we
know all our buffers are dirty, which implies they all must be uptodate,
which implies the page must be uptodate.

When called form mark_buffer_dirty, it does not warn because we may
have some buffers still not marked dirty in the page.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Thu, 24 Apr 2008, KAMEZAWA Hiroyuki wrote:

> > So its safe to migrate a !Uptodate page if it contains buffers? Note that 
> > the migration code reattaches the buffer to the new page in 
> > buffer_migrate_page().
> > 
> I think it's safe because it reattaches buffers as you explained.
> 
> under migration
> 1. a page is locked.
> 2. buffers are reattached.
> 3. a PG_writeback page are not migrated.
> 
> So, it seems safe.

Concurrent DMA reads cannot occur while we migrate? What holds off 
potential I/O to the page? The page lock? Or some buffer lock?

Not that the "buffers" are segments of the page that is migrated.
If concurrent transfers occur while we copy the page then we may see data 
corruption.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Thu, 24 Apr 2008 12:11:07 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> On Thu, 24 Apr 2008, KAMEZAWA Hiroyuki wrote:
> 
> > > So its safe to migrate a !Uptodate page if it contains buffers? Note that 
> > > the migration code reattaches the buffer to the new page in 
> > > buffer_migrate_page().
> > > 
> > I think it's safe because it reattaches buffers as you explained.
> > 
> > under migration
> > 1. a page is locked.
> > 2. buffers are reattached.
> > 3. a PG_writeback page are not migrated.
> > 
> > So, it seems safe.
> 
> Concurrent DMA reads cannot occur while we migrate? What holds off 
> potential I/O to the page? The page lock? Or some buffer lock?
> 
> Not that the "buffers" are segments of the page that is migrated.
> If concurrent transfers occur while we copy the page then we may see data 
> corruption.
> 
Hmm ? If a buffer in a page is under I/O, the whole page is locked, isn't it ?
If not, It seems anyone cannot use a page safely.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

I myself want to this patch to be included (to next -mm) and put this under
test. How do you think ? Nick ? Christoph ?

Thanks,
-Kame


On Wed, 23 Apr 2008 02:48:04 +0200
Nick Piggin <npiggin@suse.de> wrote:
> What was happening is that migrate_page_copy wants to transfer the PG_dirty
> bit from old page to new page, so what it would do is set_page_dirty(newpage).
> However set_page_dirty() is used to set the entire page dirty, wheras in
> this case, only part of the page was dirty, and it also was not uptodate.
> 
> Marking the whole page dirty with set_page_dirty would lead to corruption or
> unresolvable conditions -- a dirty && !uptodate page and dirty && !uptodate
> buffers.
> 
> Possibly we could just ClearPageDirty(oldpage); SetPageDirty(newpage);
> however in the interests of keeping the change minimal...
> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> ---
> Index: linux-2.6/mm/migrate.c
> ===================================================================
> --- linux-2.6.orig/mm/migrate.c
> +++ linux-2.6/mm/migrate.c
> @@ -383,7 +383,14 @@ static void migrate_page_copy(struct pag
>  
>  	if (PageDirty(page)) {
>  		clear_page_dirty_for_io(page);
> -		set_page_dirty(newpage);
> +		/*
> +		 * Want to mark the page and the radix tree as dirty, and
> +		 * redo the accounting that clear_page_dirty_for_io undid,
> +		 * but we can't use set_page_dirty because that function
> +		 * is actually a signal that all of the page has become dirty.
> +		 * Wheras only part of our page may be dirty.
> +		 */
> +		__set_page_dirty_nobuffers(newpage);
>   	}
>  
>  #ifdef CONFIG_SWAP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Tue, Apr 29, 2008 at 04:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> I myself want to this patch to be included (to next -mm) and put this under
> test. How do you think ? Nick ? Christoph ?

I think it should go upstream, yes. I imagine Andrew is probably just busy
with merging at the moment. I guess we should resubmit if it isn't picked
up in the next few days.

Thanks,
Nick

> 
> Thanks,
> -Kame
> 
> 
> On Wed, 23 Apr 2008 02:48:04 +0200
> Nick Piggin <npiggin@suse.de> wrote:
> > What was happening is that migrate_page_copy wants to transfer the PG_dirty
> > bit from old page to new page, so what it would do is set_page_dirty(newpage).
> > However set_page_dirty() is used to set the entire page dirty, wheras in
> > this case, only part of the page was dirty, and it also was not uptodate.
> > 
> > Marking the whole page dirty with set_page_dirty would lead to corruption or
> > unresolvable conditions -- a dirty && !uptodate page and dirty && !uptodate
> > buffers.
> > 
> > Possibly we could just ClearPageDirty(oldpage); SetPageDirty(newpage);
> > however in the interests of keeping the change minimal...
> > 
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > ---
> > Index: linux-2.6/mm/migrate.c
> > ===================================================================
> > --- linux-2.6.orig/mm/migrate.c
> > +++ linux-2.6/mm/migrate.c
> > @@ -383,7 +383,14 @@ static void migrate_page_copy(struct pag
> >  
> >  	if (PageDirty(page)) {
> >  		clear_page_dirty_for_io(page);
> > -		set_page_dirty(newpage);
> > +		/*
> > +		 * Want to mark the page and the radix tree as dirty, and
> > +		 * redo the accounting that clear_page_dirty_for_io undid,
> > +		 * but we can't use set_page_dirty because that function
> > +		 * is actually a signal that all of the page has become dirty.
> > +		 * Wheras only part of our page may be dirty.
> > +		 */
> > +		__set_page_dirty_nobuffers(newpage);
> >   	}
> >  
> >  #ifdef CONFIG_SWAP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 30 Apr 2008 08:56:11 +0200
Nick Piggin <npiggin@suse.de> wrote:

> On Tue, Apr 29, 2008 at 04:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> > I myself want to this patch to be included (to next -mm) and put this under
> > test. How do you think ? Nick ? Christoph ?
> 
> I think it should go upstream, yes. I imagine Andrew is probably just busy
> with merging at the moment. I guess we should resubmit if it isn't picked
> up in the next few days.
> 
I see.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Andrew Morton]

On Wed, 30 Apr 2008 08:56:11 +0200 Nick Piggin <npiggin@suse.de> wrote:

> On Tue, Apr 29, 2008 at 04:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> > I myself want to this patch to be included (to next -mm) and put this under
> > test. How do you think ? Nick ? Christoph ?
> 
> I think it should go upstream, yes. I imagine Andrew is probably just busy
> with merging at the moment. I guess we should resubmit if it isn't picked
> up in the next few days.

Well I'm actually waiting for something which looks like a patch to fly
past.  The last thing I saw was labelled "here is my proposed (uncompiled,
untested) fix".

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[KAMEZAWA Hiroyuki]

On Wed, 30 Apr 2008 00:12:49 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 30 Apr 2008 08:56:11 +0200 Nick Piggin <npiggin@suse.de> wrote:
> 
> > On Tue, Apr 29, 2008 at 04:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> > > I myself want to this patch to be included (to next -mm) and put this under
> > > test. How do you think ? Nick ? Christoph ?
> > 
> > I think it should go upstream, yes. I imagine Andrew is probably just busy
> > with merging at the moment. I guess we should resubmit if it isn't picked
> > up in the next few days.
> 
> Well I'm actually waiting for something which looks like a patch to fly
> past.  The last thing I saw was labelled "here is my proposed (uncompiled,
> untested) fix".
> 
At least, I tested and it worked well :)

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Wed, Apr 30, 2008 at 12:12:49AM -0700, Andrew Morton wrote:
> On Wed, 30 Apr 2008 08:56:11 +0200 Nick Piggin <npiggin@suse.de> wrote:
> 
> > On Tue, Apr 29, 2008 at 04:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> > > I myself want to this patch to be included (to next -mm) and put this under
> > > test. How do you think ? Nick ? Christoph ?
> > 
> > I think it should go upstream, yes. I imagine Andrew is probably just busy
> > with merging at the moment. I guess we should resubmit if it isn't picked
> > up in the next few days.
> 
> Well I'm actually waiting for something which looks like a patch to fly
> past.  The last thing I saw was labelled "here is my proposed (uncompiled,
> untested) fix".

OK.. I don't have a good setup for testing page migration which is why
I didn't test it.

But it was since tested and found to solve the problem (or at least the
warning went away). Christoph, do you have a regression test suite or
something to run it through?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Wed, 30 Apr 2008, Nick Piggin wrote:

> But it was since tested and found to solve the problem (or at least the
> warning went away). Christoph, do you have a regression test suite or
> something to run it through?

The numactl package contains a regression suite and also tools for 
testing.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

One issue that I am still not clear on is (in particular for memory 
offline) is how exactly to determine if a page is under read I/O. I 
initially thought simply checking for PageUptodate would do the trick.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: Warning on memory offline (and possible in usual migration?)

[kamezawa.hiroyu]

>
>One issue that I am still not clear on is (in particular for memory 
>offline) is how exactly to determine if a page is under read I/O. I 
>initially thought simply checking for PageUptodate would do the trick.
>
All troublesome case I found was "write". In my understanding,
at generic bufferted file write, xxx_write_begin() -> write -> xxx_write_end()
 sequence is used. xxx_write_begin locks a page and xxx_write_end unlock it. 
(and xxx_write_end() set a page to be Uptodate in usual case.)
So,it seems we can depend on that a page is locked or not.
But it's complicated....

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: Warning on memory offline (and possible in usual migration?)

[Badari Pulavarty]

On Thu, 2008-05-01 at 08:29 +0900, kamezawa.hiroyu@jp.fujitsu.com wrote:
> >
> >One issue that I am still not clear on is (in particular for memory 
> >offline) is how exactly to determine if a page is under read I/O. I 
> >initially thought simply checking for PageUptodate would do the trick.
> >
> All troublesome case I found was "write". In my understanding,
> at generic bufferted file write, xxx_write_begin() -> write -> xxx_write_end()
>  sequence is used. xxx_write_begin locks a page and xxx_write_end unlock it. 
> (and xxx_write_end() set a page to be Uptodate in usual case.)
> So,it seems we can depend on that a page is locked or not.

You can wait for PG_writeback to be cleared to wait for IO to finish.

Thanks,
Badari

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Wed, Apr 30, 2008 at 11:01:39AM -0700, Christoph Lameter wrote:
> One issue that I am still not clear on is (in particular for memory 
> offline) is how exactly to determine if a page is under read I/O. I 
> initially thought simply checking for PageUptodate would do the trick.

Yes if PageUptodate and the page is locked, then I don't believe
any read IO should happen.

But you definitely do seem to be migrating !PageUptodate pages. In
that case I think it works because of buffer_migrate_page which takes
the buffer locks and holds off read IO to buffers too.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: Re: Warning on memory offline (and possible in usual migration?)

[kamezawa.hiroyu]

----- Original Message -----
>On Thu, 2008-05-01 at 08:29 +0900, kamezawa.hiroyu@jp.fujitsu.com wrote:
>> >
>> >One issue that I am still not clear on is (in particular for memory 
>> >offline) is how exactly to determine if a page is under read I/O. I 
>> >initially thought simply checking for PageUptodate would do the trick.
>> >
>> All troublesome case I found was "write". In my understanding,
>> at generic bufferted file write, xxx_write_begin() -> write -> xxx_write_en
d()
>>  sequence is used. xxx_write_begin locks a page and xxx_write_end unlock it
. 
>> (and xxx_write_end() set a page to be Uptodate in usual case.)
>> So,it seems we can depend on that a page is locked or not.
>
>You can wait for PG_writeback to be cleared to wait for IO to finish.
 
yes, and migraion(offline) doesn't handle PG_writback page. it waits.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Thu, 1 May 2008, Nick Piggin wrote:

> Yes if PageUptodate and the page is locked, then I don't believe
> any read IO should happen.

Ok so page migration should check for that and not migrate a page that is 
!Uptodate?

> But you definitely do seem to be migrating !PageUptodate pages. In
> that case I think it works because of buffer_migrate_page which takes
> the buffer locks and holds off read IO to buffers too.

Comes about because memory offlining gets to pages that are not mapped 
into a process address space.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Thu, May 01, 2008 at 12:25:54PM -0700, Christoph Lameter wrote:
> On Thu, 1 May 2008, Nick Piggin wrote:
> 
> > Yes if PageUptodate and the page is locked, then I don't believe
> > any read IO should happen.
> 
> Ok so page migration should check for that and not migrate a page that is 
> !Uptodate?

Buffer migration seems to work OK now, why do you need to add the
restriction?

 
> > But you definitely do seem to be migrating !PageUptodate pages. In
> > that case I think it works because of buffer_migrate_page which takes
> > the buffer locks and holds off read IO to buffers too.
> 
> Comes about because memory offlining gets to pages that are not mapped 
> into a process address space.
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Fri, 2 May 2008, Nick Piggin wrote:

> On Thu, May 01, 2008 at 12:25:54PM -0700, Christoph Lameter wrote:
> > On Thu, 1 May 2008, Nick Piggin wrote:
> > 
> > > Yes if PageUptodate and the page is locked, then I don't believe
> > > any read IO should happen.
> > 
> > Ok so page migration should check for that and not migrate a page that is 
> > !Uptodate?
> 
> Buffer migration seems to work OK now, why do you need to add the
> restriction?

Because we have to protect against read I/O. We cannot migrate 
a page that is under I/O and free the memory that is being written to by a 
device.






--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Thu, May 01, 2008 at 06:07:43PM -0700, Christoph Lameter wrote:
> On Fri, 2 May 2008, Nick Piggin wrote:
> 
> > On Thu, May 01, 2008 at 12:25:54PM -0700, Christoph Lameter wrote:
> > > On Thu, 1 May 2008, Nick Piggin wrote:
> > > 
> > > > Yes if PageUptodate and the page is locked, then I don't believe
> > > > any read IO should happen.
> > > 
> > > Ok so page migration should check for that and not migrate a page that is 
> > > !Uptodate?
> > 
> > Buffer migration seems to work OK now, why do you need to add the
> > restriction?
> 
> Because we have to protect against read I/O. We cannot migrate 
> a page that is under I/O and free the memory that is being written to by a 
> device.

The buffer migration path protects against read IO on buffers.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Fri, 2 May 2008, Nick Piggin wrote:

> The buffer migration path protects against read IO on buffers.

Digging.... Ok the buffers are locked which means that they cannot be 
under I/O because I/O locks the buffers

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

I guess we need the following patch to handle !uptodate pages. Wish we had 
a better solution that would allow the skipping of pages with buffers 
under read I/O.


Subject: Page migration: Do not migrate page that is not uptodate

If we are migrating pages that are not mapped into a processes address
space then we may encounter !Uptodate pages. Page migration is now used
for offlining memory which scans unmapped pages.

If a page is not uptodate then read I/O may be in progress against it.
So do not migrate it. On the other hand if the page has buffers then
read I/O will lock a buffer. In that case we can migrate an !Uptodate
page but then migration will stall in buffer_migrate_page() until the
read is complete.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/migrate.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-2.6/mm/migrate.c
===================================================================
--- linux-2.6.orig/mm/migrate.c	2008-05-02 13:47:45.113707645 -0700
+++ linux-2.6/mm/migrate.c	2008-05-02 14:08:32.203644985 -0700
@@ -652,6 +652,23 @@ static int unmap_and_move(new_page_t get
 			goto unlock;
 		wait_on_page_writeback(page);
 	}
+
+	/*
+	 * Page may be under read I/O if its not uptodate and has no buffers.
+	 * In that case the page contents are not stable and should not be
+	 * migrated. So we just pass on that page and return -EAGAIN.
+	 *
+	 * If a page has buffers then the locks taken on the buffers
+	 * will indicate that read I/O is in progress. Then PageUptodate does
+	 * not matter because buffer_migrate_page() will stall until I/O is
+	 * complete. It would be better if we could catch that here and delay
+	 * migrating the page because we could migrate a the other pages on the
+	 * migrate list instead of waiting for I/O to complete on this page
+	 * (like done for writes in progress).
+	 */
+	if (!PageUptodate(page) && !page_has_buffers(page))
+		goto unlock;
+
 	/*
 	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
 	 * we cannot notice that anon_vma is freed while we migrates a page.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Fri, May 02, 2008 at 02:16:20PM -0700, Christoph Lameter wrote:
> I guess we need the following patch to handle !uptodate pages. Wish we had 
> a better solution that would allow the skipping of pages with buffers 
> under read I/O.

 
AFAIK, any filesystem which may not lock the page under read IO should
have PG_private set. In which case, if they don't have buffers they
should have a releasepage method. Otherwise, how would we ever reclaim
!uptodate && !buffers pages?

So I don't think we need this patch.

> 
> Subject: Page migration: Do not migrate page that is not uptodate
> 
> If we are migrating pages that are not mapped into a processes address
> space then we may encounter !Uptodate pages. Page migration is now used
> for offlining memory which scans unmapped pages.
> 
> If a page is not uptodate then read I/O may be in progress against it.
> So do not migrate it. On the other hand if the page has buffers then
> read I/O will lock a buffer. In that case we can migrate an !Uptodate
> page but then migration will stall in buffer_migrate_page() until the
> read is complete.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> ---
>  mm/migrate.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> Index: linux-2.6/mm/migrate.c
> ===================================================================
> --- linux-2.6.orig/mm/migrate.c	2008-05-02 13:47:45.113707645 -0700
> +++ linux-2.6/mm/migrate.c	2008-05-02 14:08:32.203644985 -0700
> @@ -652,6 +652,23 @@ static int unmap_and_move(new_page_t get
>  			goto unlock;
>  		wait_on_page_writeback(page);
>  	}
> +
> +	/*
> +	 * Page may be under read I/O if its not uptodate and has no buffers.
> +	 * In that case the page contents are not stable and should not be
> +	 * migrated. So we just pass on that page and return -EAGAIN.
> +	 *
> +	 * If a page has buffers then the locks taken on the buffers
> +	 * will indicate that read I/O is in progress. Then PageUptodate does
> +	 * not matter because buffer_migrate_page() will stall until I/O is
> +	 * complete. It would be better if we could catch that here and delay
> +	 * migrating the page because we could migrate a the other pages on the
> +	 * migrate list instead of waiting for I/O to complete on this page
> +	 * (like done for writes in progress).
> +	 */
> +	if (!PageUptodate(page) && !page_has_buffers(page))
> +		goto unlock;
> +
>  	/*
>  	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
>  	 * we cannot notice that anon_vma is freed while we migrates a page.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Mon, 5 May 2008, Nick Piggin wrote:

> AFAIK, any filesystem which may not lock the page under read IO should
> have PG_private set. In which case, if they don't have buffers they
> should have a releasepage method. Otherwise, how would we ever reclaim
> !uptodate && !buffers pages?

Hmmm.. Ok mpage.c does:

static void mpage_end_io_write(struct bio *bio, int err)
{
        const int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
        struct bio_vec *bvec = bio->bi_io_vec + bio->bi_vcnt - 1;

        do {
                struct page *page = bvec->bv_page;

                if (--bvec >= bio->bi_io_vec)
                        prefetchw(&bvec->bv_page->flags);

                if (!uptodate){
                        SetPageError(page);
                        if (page->mapping)
                                set_bit(AS_EIO, &page->mapping->flags);
                }
                end_page_writeback(page);
        } while (bvec >= bio->bi_io_vec);
        bio_put(bio);
}

So it seems the page is always locked if !Uptodate.
 
> So I don't think we need this patch.

Ok.

Is there any easy way to check if any of the buffers are locked? It would 
be good if we could skip the pages with pending I/O on the first migration 
passes and only get to them after most of the others have been migrated. 
The taking of the buffer locks instead of the page lock defeats the scheme 
to defer the difficult migrations till later.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Nick Piggin]

On Mon, May 05, 2008 at 10:28:48AM -0700, Christoph Lameter wrote:
> On Mon, 5 May 2008, Nick Piggin wrote:
> 
> > AFAIK, any filesystem which may not lock the page under read IO should
> > have PG_private set. In which case, if they don't have buffers they
> > should have a releasepage method. Otherwise, how would we ever reclaim
> > !uptodate && !buffers pages?
> 
> Hmmm.. Ok mpage.c does:
> 
> static void mpage_end_io_write(struct bio *bio, int err)
> {
>         const int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
>         struct bio_vec *bvec = bio->bi_io_vec + bio->bi_vcnt - 1;
> 
>         do {
>                 struct page *page = bvec->bv_page;
> 
>                 if (--bvec >= bio->bi_io_vec)
>                         prefetchw(&bvec->bv_page->flags);
> 
>                 if (!uptodate){
>                         SetPageError(page);
>                         if (page->mapping)
>                                 set_bit(AS_EIO, &page->mapping->flags);
>                 }
>                 end_page_writeback(page);
>         } while (bvec >= bio->bi_io_vec);
>         bio_put(bio);
> }
> 
> So it seems the page is always locked if !Uptodate.

Well if you weren't sure, you'd have to go through all filesystems because
any of them can do anything they like with their own pagecache really.
But in the end you've got the page count check which is the same as
reclaim. So I don't think there is really any doubt about this. The actual
page private / buffer migrations paths are obviously the less tested ones
in that code.

  
> > So I don't think we need this patch.
> 
> Ok.
> 
> Is there any easy way to check if any of the buffers are locked? It would 
> be good if we could skip the pages with pending I/O on the first migration 
> passes and only get to them after most of the others have been migrated. 
> The taking of the buffer locks instead of the page lock defeats the scheme 
> to defer the difficult migrations till later.

Can't you just test whether the buffers are locked?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Warning on memory offline (and possible in usual migration?)

[Christoph Lameter]

On Tue, 6 May 2008, Nick Piggin wrote:

> > Is there any easy way to check if any of the buffers are locked? It would 
> > be good if we could skip the pages with pending I/O on the first migration 
> > passes and only get to them after most of the others have been migrated. 
> > The taking of the buffer locks instead of the page lock defeats the scheme 
> > to defer the difficult migrations till later.
> 
> Can't you just test whether the buffers are locked?

That would mean adding some ugly code that check before we lock the page 
if we will be using buffer_migrate_page() later and then loop over the 
buffers checking for lock bits? 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>