Page migration issue with UBIFS
diff mbox

Message ID 20160316142156.GA23595@node.shutemov.name
State New
Headers show

Commit Message

Kirill A. Shutemov March 16, 2016, 2:21 p.m. UTC
On Wed, Mar 16, 2016 at 12:18:50AM +0100, Richard Weinberger wrote:
> Am 15.03.2016 um 16:37 schrieb Christoph Hellwig:
> > On Tue, Mar 15, 2016 at 04:32:40PM +0100, Richard Weinberger wrote:
> >>> Or if ->page_mkwrite() was called, why the page is not dirty?
> >>
> >> BTW: UBIFS does not implement ->migratepage(), could this be a problem?
> > 
> > This might be the reason.  I can't reall make sense of
> > buffer_migrate_page, but it seems to migrate buffer_head state to
> > the new page.
> > 
> > I'd love to know why CMA even tries to migrate pages that don't have a
> > ->migratepage method, this seems incredibly dangerous to me.
> 
> FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
> longer explode upon page migration.
> Tomorrow I'll do more tests to make sure.

Could you check if something like this would fix the issue.
Completely untested.

Comments

Kirill A. Shutemov March 16, 2016, 2:27 p.m. UTC | #1
On Wed, Mar 16, 2016 at 05:21:56PM +0300, Kirill A. Shutemov wrote:
> On Wed, Mar 16, 2016 at 12:18:50AM +0100, Richard Weinberger wrote:
> > Am 15.03.2016 um 16:37 schrieb Christoph Hellwig:
> > > On Tue, Mar 15, 2016 at 04:32:40PM +0100, Richard Weinberger wrote:
> > >>> Or if ->page_mkwrite() was called, why the page is not dirty?
> > >>
> > >> BTW: UBIFS does not implement ->migratepage(), could this be a problem?
> > > 
> > > This might be the reason.  I can't reall make sense of
> > > buffer_migrate_page, but it seems to migrate buffer_head state to
> > > the new page.
> > > 
> > > I'd love to know why CMA even tries to migrate pages that don't have a
> > > ->migratepage method, this seems incredibly dangerous to me.
> > 
> > FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
> > longer explode upon page migration.
> > Tomorrow I'll do more tests to make sure.
> 
> Could you check if something like this would fix the issue.
> Completely untested.
> 
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 065c88f8e4b8..9da34120dc5e 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -52,6 +52,7 @@
>  #include "ubifs.h"
>  #include <linux/mount.h>
>  #include <linux/slab.h>
> +#include <linux/migrate.h>
>  
>  static int read_block(struct inode *inode, void *addr, unsigned int block,
>  		      struct ubifs_data_node *dn)
> @@ -1452,6 +1453,20 @@ static int ubifs_set_page_dirty(struct page *page)
>  	return ret;
>  }
>  
> +static int ubifs_migrate_page(struct address_space *mapping,
> +		struct page *newpage, struct page *page, enum migrate_mode mode)
> +{
> +	if (PagePrivate(page)) {
> +		SetPagePrivate(newpage);
> +		__set_page_dirty_nobuffers(newpage);
> +	}
> +
> +	if (PageChecked(page))
> +		SetPageChecked(newpage);

These two lines are redundant, migrate_page_copy() would do this for us.

> +
> +	return migrate_page(mapping, newpage, page, mode);
> +}
> +
>  static int ubifs_releasepage(struct page *page, gfp_t unused_gfp_flags)
>  {
>  	/*
> @@ -1591,6 +1606,7 @@ const struct address_space_operations ubifs_file_address_operations = {
>  	.write_end      = ubifs_write_end,
>  	.invalidatepage = ubifs_invalidatepage,
>  	.set_page_dirty = ubifs_set_page_dirty,
> +	.migratepage	= ubifs_migrate_page,
>  	.releasepage    = ubifs_releasepage,
>  };
>  
> -- 
>  Kirill A. Shutemov
Richard Weinberger March 16, 2016, 8:47 p.m. UTC | #2
Adding more CC's.

Am 16.03.2016 um 15:27 schrieb Kirill A. Shutemov:
> On Wed, Mar 16, 2016 at 05:21:56PM +0300, Kirill A. Shutemov wrote:
>> On Wed, Mar 16, 2016 at 12:18:50AM +0100, Richard Weinberger wrote:
>>> Am 15.03.2016 um 16:37 schrieb Christoph Hellwig:
>>>> On Tue, Mar 15, 2016 at 04:32:40PM +0100, Richard Weinberger wrote:
>>>>>> Or if ->page_mkwrite() was called, why the page is not dirty?
>>>>>
>>>>> BTW: UBIFS does not implement ->migratepage(), could this be a problem?
>>>>
>>>> This might be the reason.  I can't reall make sense of
>>>> buffer_migrate_page, but it seems to migrate buffer_head state to
>>>> the new page.
>>>>
>>>> I'd love to know why CMA even tries to migrate pages that don't have a
>>>> ->migratepage method, this seems incredibly dangerous to me.
>>>
>>> FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
>>> longer explode upon page migration.
>>> Tomorrow I'll do more tests to make sure.
>>
>> Could you check if something like this would fix the issue.

Nope.

[  108.080000] BUG: Bad page state in process drm-stress-test  pfn:5c674
[  108.080000] page:deb8ce80 count:0 mapcount:0 mapping:  (null) index:0x0
[  108.090000] flags: 0x810(dirty|private)
[  108.100000] page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
[  108.100000] bad because of flags:
[  108.110000] flags: 0x800(private)
[  108.110000] Modules linked in:
[  108.120000] CPU: 0 PID: 1855 Comm: drm-stress-test Not tainted 4.4.4-gaae1ad1-dirty #14
[  108.120000] Hardware name: Allwinner sun4i/sun5i Families
[  108.120000] [<c0015eb4>] (unwind_backtrace) from [<c0012cec>] (show_stack+0x10/0x14)
[  108.120000] [<c0012cec>] (show_stack) from [<c02abaf8>] (dump_stack+0x8c/0xa0)
[  108.120000] [<c02abaf8>] (dump_stack) from [<c00cbe78>] (bad_page+0xcc/0x11c)
[  108.120000] [<c00cbe78>] (bad_page) from [<c00cc0f4>] (free_pages_prepare+0x22c/0x2f4)
[  108.120000] [<c00cc0f4>] (free_pages_prepare) from [<c00cdf2c>] (free_hot_cold_page+0x34/0x194)
[  108.120000] [<c00cdf2c>] (free_hot_cold_page) from [<c00ce0d4>] (free_hot_cold_page_list+0x48/0xdc)
[  108.120000] [<c00ce0d4>] (free_hot_cold_page_list) from [<c00d55a8>] (release_pages+0x1dc/0x224)
[  108.120000] [<c00d55a8>] (release_pages) from [<c00d56d8>] (pagevec_lru_move_fn+0xe8/0xf8)
[  108.120000] [<c00d56d8>] (pagevec_lru_move_fn) from [<c00d579c>] (__lru_cache_add+0x60/0x88)
[  108.120000] [<c00d579c>] (__lru_cache_add) from [<c00d9578>] (putback_lru_page+0x68/0xbc)
[  108.120000] [<c00d9578>] (putback_lru_page) from [<c010bd6c>] (migrate_pages+0x208/0x730)
[  108.120000] [<c010bd6c>] (migrate_pages) from [<c00d0860>] (alloc_contig_range+0x168/0x2f4)
[  108.120000] [<c00d0860>] (alloc_contig_range) from [<c010cdb4>] (cma_alloc+0x170/0x2c0)
[  108.120000] [<c010cdb4>] (cma_alloc) from [<c001a9d4>] (__alloc_from_contiguous+0x38/0xd8)
[  108.120000] [<c001a9d4>] (__alloc_from_contiguous) from [<c001adb8>] (__dma_alloc+0x234/0x278)
[  108.120000] [<c001adb8>] (__dma_alloc) from [<c001ae8c>] (arm_dma_alloc+0x54/0x5c)
[  108.120000] [<c001ae8c>] (arm_dma_alloc) from [<c035bd70>] (drm_gem_cma_create+0x9c/0xf0)
[  108.120000] [<c035bd70>] (drm_gem_cma_create) from [<c035bde0>] (drm_gem_cma_create_with_handle+0x1c/0xe8)
[  108.120000] [<c035bde0>] (drm_gem_cma_create_with_handle) from [<c035bf48>] (drm_gem_cma_dumb_create+0x3c/0x48)
[  108.120000] [<c035bf48>] (drm_gem_cma_dumb_create) from [<c0340d18>] (drm_ioctl+0x12c/0x440)
[  108.120000] [<c0340d18>] (drm_ioctl) from [<c011fc7c>] (do_vfs_ioctl+0x3f4/0x614)
[  108.120000] [<c011fc7c>] (do_vfs_ioctl) from [<c011fed0>] (SyS_ioctl+0x34/0x5c)
[  108.120000] [<c011fed0>] (SyS_ioctl) from [<c000f2c0>] (ret_fast_syscall+0x0/0x34)

It is still not clear why UBIFS has to provide a >migratepage() and what the expected semantics
are.
What we know so far is that the fall back migration function is broken. I'm sure not only on UBIFS.

Can CMA folks please clarify? :-)

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joonsoo Kim March 17, 2016, 7:11 a.m. UTC | #3
On Wed, Mar 16, 2016 at 09:47:20PM +0100, Richard Weinberger wrote:
> Adding more CC's.
> 
> Am 16.03.2016 um 15:27 schrieb Kirill A. Shutemov:
> > On Wed, Mar 16, 2016 at 05:21:56PM +0300, Kirill A. Shutemov wrote:
> >> On Wed, Mar 16, 2016 at 12:18:50AM +0100, Richard Weinberger wrote:
> >>> Am 15.03.2016 um 16:37 schrieb Christoph Hellwig:
> >>>> On Tue, Mar 15, 2016 at 04:32:40PM +0100, Richard Weinberger wrote:
> >>>>>> Or if ->page_mkwrite() was called, why the page is not dirty?
> >>>>>
> >>>>> BTW: UBIFS does not implement ->migratepage(), could this be a problem?
> >>>>
> >>>> This might be the reason.  I can't reall make sense of
> >>>> buffer_migrate_page, but it seems to migrate buffer_head state to
> >>>> the new page.
> >>>>
> >>>> I'd love to know why CMA even tries to migrate pages that don't have a
> >>>> ->migratepage method, this seems incredibly dangerous to me.
> >>>
> >>> FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
> >>> longer explode upon page migration.
> >>> Tomorrow I'll do more tests to make sure.
> >>
> >> Could you check if something like this would fix the issue.
> 
> Nope.
> 
> [  108.080000] BUG: Bad page state in process drm-stress-test  pfn:5c674
> [  108.080000] page:deb8ce80 count:0 mapcount:0 mapping:  (null) index:0x0
> [  108.090000] flags: 0x810(dirty|private)
> [  108.100000] page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
> [  108.100000] bad because of flags:
> [  108.110000] flags: 0x800(private)
> [  108.110000] Modules linked in:
> [  108.120000] CPU: 0 PID: 1855 Comm: drm-stress-test Not tainted 4.4.4-gaae1ad1-dirty #14
> [  108.120000] Hardware name: Allwinner sun4i/sun5i Families
> [  108.120000] [<c0015eb4>] (unwind_backtrace) from [<c0012cec>] (show_stack+0x10/0x14)
> [  108.120000] [<c0012cec>] (show_stack) from [<c02abaf8>] (dump_stack+0x8c/0xa0)
> [  108.120000] [<c02abaf8>] (dump_stack) from [<c00cbe78>] (bad_page+0xcc/0x11c)
> [  108.120000] [<c00cbe78>] (bad_page) from [<c00cc0f4>] (free_pages_prepare+0x22c/0x2f4)
> [  108.120000] [<c00cc0f4>] (free_pages_prepare) from [<c00cdf2c>] (free_hot_cold_page+0x34/0x194)
> [  108.120000] [<c00cdf2c>] (free_hot_cold_page) from [<c00ce0d4>] (free_hot_cold_page_list+0x48/0xdc)
> [  108.120000] [<c00ce0d4>] (free_hot_cold_page_list) from [<c00d55a8>] (release_pages+0x1dc/0x224)
> [  108.120000] [<c00d55a8>] (release_pages) from [<c00d56d8>] (pagevec_lru_move_fn+0xe8/0xf8)
> [  108.120000] [<c00d56d8>] (pagevec_lru_move_fn) from [<c00d579c>] (__lru_cache_add+0x60/0x88)
> [  108.120000] [<c00d579c>] (__lru_cache_add) from [<c00d9578>] (putback_lru_page+0x68/0xbc)
> [  108.120000] [<c00d9578>] (putback_lru_page) from [<c010bd6c>] (migrate_pages+0x208/0x730)
> [  108.120000] [<c010bd6c>] (migrate_pages) from [<c00d0860>] (alloc_contig_range+0x168/0x2f4)
> [  108.120000] [<c00d0860>] (alloc_contig_range) from [<c010cdb4>] (cma_alloc+0x170/0x2c0)
> [  108.120000] [<c010cdb4>] (cma_alloc) from [<c001a9d4>] (__alloc_from_contiguous+0x38/0xd8)
> [  108.120000] [<c001a9d4>] (__alloc_from_contiguous) from [<c001adb8>] (__dma_alloc+0x234/0x278)
> [  108.120000] [<c001adb8>] (__dma_alloc) from [<c001ae8c>] (arm_dma_alloc+0x54/0x5c)
> [  108.120000] [<c001ae8c>] (arm_dma_alloc) from [<c035bd70>] (drm_gem_cma_create+0x9c/0xf0)
> [  108.120000] [<c035bd70>] (drm_gem_cma_create) from [<c035bde0>] (drm_gem_cma_create_with_handle+0x1c/0xe8)
> [  108.120000] [<c035bde0>] (drm_gem_cma_create_with_handle) from [<c035bf48>] (drm_gem_cma_dumb_create+0x3c/0x48)
> [  108.120000] [<c035bf48>] (drm_gem_cma_dumb_create) from [<c0340d18>] (drm_ioctl+0x12c/0x440)
> [  108.120000] [<c0340d18>] (drm_ioctl) from [<c011fc7c>] (do_vfs_ioctl+0x3f4/0x614)
> [  108.120000] [<c011fc7c>] (do_vfs_ioctl) from [<c011fed0>] (SyS_ioctl+0x34/0x5c)
> [  108.120000] [<c011fed0>] (SyS_ioctl) from [<c000f2c0>] (ret_fast_syscall+0x0/0x34)
> 
> It is still not clear why UBIFS has to provide a >migratepage() and what the expected semantics
> are.
> What we know so far is that the fall back migration function is broken. I'm sure not only on UBIFS.
> 
> Can CMA folks please clarify? :-)

Hello,

As you mentioned earlier, this issue would not be directly related
to CMA. It looks like it is more general issue related to interaction
between MM and FS. Your first error log shows that error happens when
ubifs_set_page_dirty() is called in try_to_unmap_one() which also
can be called by reclaimer (kswapd or direct reclaim). Quick search shows
that problem also happens on reclaim. Is that fixed?

http://www.spinics.net/lists/linux-fsdevel/msg79531.html

I think that you need to CC other people who understand interaction
between MM and FS perfectly.

Sorry about not much helpful here.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Weinberger March 17, 2016, 8:13 a.m. UTC | #4
Am 17.03.2016 um 08:11 schrieb Joonsoo Kim:
>> It is still not clear why UBIFS has to provide a >migratepage() and what the expected semantics
>> are.
>> What we know so far is that the fall back migration function is broken. I'm sure not only on UBIFS.
>>
>> Can CMA folks please clarify? :-)
> 
> Hello,
> 
> As you mentioned earlier, this issue would not be directly related
> to CMA. It looks like it is more general issue related to interaction
> between MM and FS. Your first error log shows that error happens when
> ubifs_set_page_dirty() is called in try_to_unmap_one() which also
> can be called by reclaimer (kswapd or direct reclaim). Quick search shows
> that problem also happens on reclaim. Is that fixed?
> 
> http://www.spinics.net/lists/linux-fsdevel/msg79531.html

Well, this problem happened only on a tainted kernel and never popped up again.
So, I really don't know. :-)

> I think that you need to CC other people who understand interaction
> between MM and FS perfectly.

Who is missing on the CC list?

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joonsoo Kim March 17, 2016, 3:17 p.m. UTC | #5
2016-03-17 17:13 GMT+09:00 Richard Weinberger <richard@nod.at>:
> Am 17.03.2016 um 08:11 schrieb Joonsoo Kim:
>>> It is still not clear why UBIFS has to provide a >migratepage() and what the expected semantics
>>> are.
>>> What we know so far is that the fall back migration function is broken. I'm sure not only on UBIFS.
>>>
>>> Can CMA folks please clarify? :-)
>>
>> Hello,
>>
>> As you mentioned earlier, this issue would not be directly related
>> to CMA. It looks like it is more general issue related to interaction
>> between MM and FS. Your first error log shows that error happens when
>> ubifs_set_page_dirty() is called in try_to_unmap_one() which also
>> can be called by reclaimer (kswapd or direct reclaim). Quick search shows
>> that problem also happens on reclaim. Is that fixed?
>>
>> http://www.spinics.net/lists/linux-fsdevel/msg79531.html
>
> Well, this problem happened only on a tainted kernel and never popped up again.
> So, I really don't know. :-)
>
>> I think that you need to CC other people who understand interaction
>> between MM and FS perfectly.
>
> Who is missing on the CC list?

Vlastimil already added Hugh and Mel to other relevant thread but I think
this thread has more information about the problem so I add them here.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig March 21, 2016, 3:28 p.m. UTC | #6
On Wed, Mar 16, 2016 at 05:21:56PM +0300, Kirill A. Shutemov wrote:
> > FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
> > longer explode upon page migration.
> > Tomorrow I'll do more tests to make sure.
> 
> Could you check if something like this would fix the issue.
> Completely untested.

We really need to make the default safe for file systems that don't
have a migratepage method.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton March 21, 2016, 11 p.m. UTC | #7
On Wed, 16 Mar 2016 21:47:20 +0100 Richard Weinberger <richard@nod.at> wrote:

> Adding more CC's.
> 
> Am 16.03.2016 um 15:27 schrieb Kirill A. Shutemov:
> > On Wed, Mar 16, 2016 at 05:21:56PM +0300, Kirill A. Shutemov wrote:
> >> On Wed, Mar 16, 2016 at 12:18:50AM +0100, Richard Weinberger wrote:
> >>> Am 15.03.2016 um 16:37 schrieb Christoph Hellwig:
> >>>> On Tue, Mar 15, 2016 at 04:32:40PM +0100, Richard Weinberger wrote:
> >>>>>> Or if ->page_mkwrite() was called, why the page is not dirty?
> >>>>>
> >>>>> BTW: UBIFS does not implement ->migratepage(), could this be a problem?
> >>>>
> >>>> This might be the reason.  I can't reall make sense of
> >>>> buffer_migrate_page, but it seems to migrate buffer_head state to
> >>>> the new page.
> >>>>
> >>>> I'd love to know why CMA even tries to migrate pages that don't have a
> >>>> ->migratepage method, this seems incredibly dangerous to me.
> >>>
> >>> FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
> >>> longer explode upon page migration.
> >>> Tomorrow I'll do more tests to make sure.
> >>
> >> Could you check if something like this would fix the issue.
> 
> Nope.
> 
> [  108.080000] BUG: Bad page state in process drm-stress-test  pfn:5c674
> [  108.080000] page:deb8ce80 count:0 mapcount:0 mapping:  (null) index:0x0
> [  108.090000] flags: 0x810(dirty|private)
> [  108.100000] page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
> [  108.100000] bad because of flags:
> [  108.110000] flags: 0x800(private)
> [  108.110000] Modules linked in:
> [  108.120000] CPU: 0 PID: 1855 Comm: drm-stress-test Not tainted 4.4.4-gaae1ad1-dirty #14
> [  108.120000] Hardware name: Allwinner sun4i/sun5i Families
> [  108.120000] [<c0015eb4>] (unwind_backtrace) from [<c0012cec>] (show_stack+0x10/0x14)
> [  108.120000] [<c0012cec>] (show_stack) from [<c02abaf8>] (dump_stack+0x8c/0xa0)
> [  108.120000] [<c02abaf8>] (dump_stack) from [<c00cbe78>] (bad_page+0xcc/0x11c)
> [  108.120000] [<c00cbe78>] (bad_page) from [<c00cc0f4>] (free_pages_prepare+0x22c/0x2f4)
> [  108.120000] [<c00cc0f4>] (free_pages_prepare) from [<c00cdf2c>] (free_hot_cold_page+0x34/0x194)
> [  108.120000] [<c00cdf2c>] (free_hot_cold_page) from [<c00ce0d4>] (free_hot_cold_page_list+0x48/0xdc)
> [  108.120000] [<c00ce0d4>] (free_hot_cold_page_list) from [<c00d55a8>] (release_pages+0x1dc/0x224)
> [  108.120000] [<c00d55a8>] (release_pages) from [<c00d56d8>] (pagevec_lru_move_fn+0xe8/0xf8)
> [  108.120000] [<c00d56d8>] (pagevec_lru_move_fn) from [<c00d579c>] (__lru_cache_add+0x60/0x88)
> [  108.120000] [<c00d579c>] (__lru_cache_add) from [<c00d9578>] (putback_lru_page+0x68/0xbc)
> [  108.120000] [<c00d9578>] (putback_lru_page) from [<c010bd6c>] (migrate_pages+0x208/0x730)
> [  108.120000] [<c010bd6c>] (migrate_pages) from [<c00d0860>] (alloc_contig_range+0x168/0x2f4)
> [  108.120000] [<c00d0860>] (alloc_contig_range) from [<c010cdb4>] (cma_alloc+0x170/0x2c0)
> [  108.120000] [<c010cdb4>] (cma_alloc) from [<c001a9d4>] (__alloc_from_contiguous+0x38/0xd8)
> [  108.120000] [<c001a9d4>] (__alloc_from_contiguous) from [<c001adb8>] (__dma_alloc+0x234/0x278)
> [  108.120000] [<c001adb8>] (__dma_alloc) from [<c001ae8c>] (arm_dma_alloc+0x54/0x5c)
> [  108.120000] [<c001ae8c>] (arm_dma_alloc) from [<c035bd70>] (drm_gem_cma_create+0x9c/0xf0)
> [  108.120000] [<c035bd70>] (drm_gem_cma_create) from [<c035bde0>] (drm_gem_cma_create_with_handle+0x1c/0xe8)
> [  108.120000] [<c035bde0>] (drm_gem_cma_create_with_handle) from [<c035bf48>] (drm_gem_cma_dumb_create+0x3c/0x48)
> [  108.120000] [<c035bf48>] (drm_gem_cma_dumb_create) from [<c0340d18>] (drm_ioctl+0x12c/0x440)
> [  108.120000] [<c0340d18>] (drm_ioctl) from [<c011fc7c>] (do_vfs_ioctl+0x3f4/0x614)
> [  108.120000] [<c011fc7c>] (do_vfs_ioctl) from [<c011fed0>] (SyS_ioctl+0x34/0x5c)
> [  108.120000] [<c011fed0>] (SyS_ioctl) from [<c000f2c0>] (ret_fast_syscall+0x0/0x34)
> 
> It is still not clear why UBIFS has to provide a >migratepage() and what the expected semantics
> are.
> What we know so far is that the fall back migration function is broken. I'm sure not only on UBIFS.
> 

The above says "PagePrivate was still set", and UBIFS does muck with
PagePrivate.  Perhaps the fs isn't clearing things up in all the right
places.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Weinberger March 21, 2016, 11:06 p.m. UTC | #8
Am 22.03.2016 um 00:00 schrieb Andrew Morton:
> On Wed, 16 Mar 2016 21:47:20 +0100 Richard Weinberger <richard@nod.at> wrote:
> 
>> Adding more CC's.
>>
>> Am 16.03.2016 um 15:27 schrieb Kirill A. Shutemov:
>>> On Wed, Mar 16, 2016 at 05:21:56PM +0300, Kirill A. Shutemov wrote:
>>>> On Wed, Mar 16, 2016 at 12:18:50AM +0100, Richard Weinberger wrote:
>>>>> Am 15.03.2016 um 16:37 schrieb Christoph Hellwig:
>>>>>> On Tue, Mar 15, 2016 at 04:32:40PM +0100, Richard Weinberger wrote:
>>>>>>>> Or if ->page_mkwrite() was called, why the page is not dirty?
>>>>>>>
>>>>>>> BTW: UBIFS does not implement ->migratepage(), could this be a problem?
>>>>>>
>>>>>> This might be the reason.  I can't reall make sense of
>>>>>> buffer_migrate_page, but it seems to migrate buffer_head state to
>>>>>> the new page.
>>>>>>
>>>>>> I'd love to know why CMA even tries to migrate pages that don't have a
>>>>>> ->migratepage method, this seems incredibly dangerous to me.
>>>>>
>>>>> FYI, with a dummy ->migratepage() which returns only -EINVAL UBIFS does no
>>>>> longer explode upon page migration.
>>>>> Tomorrow I'll do more tests to make sure.
>>>>
>>>> Could you check if something like this would fix the issue.
>>
>> Nope.
>>
>> [  108.080000] BUG: Bad page state in process drm-stress-test  pfn:5c674
>> [  108.080000] page:deb8ce80 count:0 mapcount:0 mapping:  (null) index:0x0
>> [  108.090000] flags: 0x810(dirty|private)
>> [  108.100000] page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
>> [  108.100000] bad because of flags:
>> [  108.110000] flags: 0x800(private)
>> [  108.110000] Modules linked in:
>> [  108.120000] CPU: 0 PID: 1855 Comm: drm-stress-test Not tainted 4.4.4-gaae1ad1-dirty #14
>> [  108.120000] Hardware name: Allwinner sun4i/sun5i Families
>> [  108.120000] [<c0015eb4>] (unwind_backtrace) from [<c0012cec>] (show_stack+0x10/0x14)
>> [  108.120000] [<c0012cec>] (show_stack) from [<c02abaf8>] (dump_stack+0x8c/0xa0)
>> [  108.120000] [<c02abaf8>] (dump_stack) from [<c00cbe78>] (bad_page+0xcc/0x11c)
>> [  108.120000] [<c00cbe78>] (bad_page) from [<c00cc0f4>] (free_pages_prepare+0x22c/0x2f4)
>> [  108.120000] [<c00cc0f4>] (free_pages_prepare) from [<c00cdf2c>] (free_hot_cold_page+0x34/0x194)
>> [  108.120000] [<c00cdf2c>] (free_hot_cold_page) from [<c00ce0d4>] (free_hot_cold_page_list+0x48/0xdc)
>> [  108.120000] [<c00ce0d4>] (free_hot_cold_page_list) from [<c00d55a8>] (release_pages+0x1dc/0x224)
>> [  108.120000] [<c00d55a8>] (release_pages) from [<c00d56d8>] (pagevec_lru_move_fn+0xe8/0xf8)
>> [  108.120000] [<c00d56d8>] (pagevec_lru_move_fn) from [<c00d579c>] (__lru_cache_add+0x60/0x88)
>> [  108.120000] [<c00d579c>] (__lru_cache_add) from [<c00d9578>] (putback_lru_page+0x68/0xbc)
>> [  108.120000] [<c00d9578>] (putback_lru_page) from [<c010bd6c>] (migrate_pages+0x208/0x730)
>> [  108.120000] [<c010bd6c>] (migrate_pages) from [<c00d0860>] (alloc_contig_range+0x168/0x2f4)
>> [  108.120000] [<c00d0860>] (alloc_contig_range) from [<c010cdb4>] (cma_alloc+0x170/0x2c0)
>> [  108.120000] [<c010cdb4>] (cma_alloc) from [<c001a9d4>] (__alloc_from_contiguous+0x38/0xd8)
>> [  108.120000] [<c001a9d4>] (__alloc_from_contiguous) from [<c001adb8>] (__dma_alloc+0x234/0x278)
>> [  108.120000] [<c001adb8>] (__dma_alloc) from [<c001ae8c>] (arm_dma_alloc+0x54/0x5c)
>> [  108.120000] [<c001ae8c>] (arm_dma_alloc) from [<c035bd70>] (drm_gem_cma_create+0x9c/0xf0)
>> [  108.120000] [<c035bd70>] (drm_gem_cma_create) from [<c035bde0>] (drm_gem_cma_create_with_handle+0x1c/0xe8)
>> [  108.120000] [<c035bde0>] (drm_gem_cma_create_with_handle) from [<c035bf48>] (drm_gem_cma_dumb_create+0x3c/0x48)
>> [  108.120000] [<c035bf48>] (drm_gem_cma_dumb_create) from [<c0340d18>] (drm_ioctl+0x12c/0x440)
>> [  108.120000] [<c0340d18>] (drm_ioctl) from [<c011fc7c>] (do_vfs_ioctl+0x3f4/0x614)
>> [  108.120000] [<c011fc7c>] (do_vfs_ioctl) from [<c011fed0>] (SyS_ioctl+0x34/0x5c)
>> [  108.120000] [<c011fed0>] (SyS_ioctl) from [<c000f2c0>] (ret_fast_syscall+0x0/0x34)
>>
>> It is still not clear why UBIFS has to provide a >migratepage() and what the expected semantics
>> are.
>> What we know so far is that the fall back migration function is broken. I'm sure not only on UBIFS.
>>
> 
> The above says "PagePrivate was still set", and UBIFS does muck with
> PagePrivate.  Perhaps the fs isn't clearing things up in all the right
> places.

This is not the original splat. It is the output after applying a non-functional patch from Kirill.

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 065c88f8e4b8..9da34120dc5e 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -52,6 +52,7 @@ 
 #include "ubifs.h"
 #include <linux/mount.h>
 #include <linux/slab.h>
+#include <linux/migrate.h>
 
 static int read_block(struct inode *inode, void *addr, unsigned int block,
 		      struct ubifs_data_node *dn)
@@ -1452,6 +1453,20 @@  static int ubifs_set_page_dirty(struct page *page)
 	return ret;
 }
 
+static int ubifs_migrate_page(struct address_space *mapping,
+		struct page *newpage, struct page *page, enum migrate_mode mode)
+{
+	if (PagePrivate(page)) {
+		SetPagePrivate(newpage);
+		__set_page_dirty_nobuffers(newpage);
+	}
+
+	if (PageChecked(page))
+		SetPageChecked(newpage);
+
+	return migrate_page(mapping, newpage, page, mode);
+}
+
 static int ubifs_releasepage(struct page *page, gfp_t unused_gfp_flags)
 {
 	/*
@@ -1591,6 +1606,7 @@  const struct address_space_operations ubifs_file_address_operations = {
 	.write_end      = ubifs_write_end,
 	.invalidatepage = ubifs_invalidatepage,
 	.set_page_dirty = ubifs_set_page_dirty,
+	.migratepage	= ubifs_migrate_page,
 	.releasepage    = ubifs_releasepage,
 };