linux-kernel - Re: + compaction-add-def_blk_aops-migrate-function-for-memory-compaction.patch added to -mm tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170309121507.GH15874@quack2.suse.cz>
Date:   Thu, 9 Mar 2017 13:15:07 +0100
From:   Jan Kara <jack@...e.cz>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     akpm@...ux-foundation.org, zhouxianrong@...wei.com,
        Mi.Sophia.Wang@...wei.com, hannes@...xchg.org,
        kirill.shutemov@...ux.intel.com, mgorman@...hsingularity.net,
        minchan@...nel.org, viro@...iv.linux.org.uk, weidu.du@...wei.com,
        won.ho.park@...wei.com, zhangshiming5@...wei.com,
        zhouxiaoyan1@...wei.com, zhouxiyu@...wei.com,
        mm-commits@...r.kernel.org, Jan Kara <jack@...e.cz>,
        Hugh Dickins <hughd@...gle.com>,
        Linux-FSDevel <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: +
 compaction-add-def_blk_aops-migrate-function-for-memory-compaction.patch
 added to -mm tree

On Thu 09-03-17 08:58:45, Vlastimil Babka wrote:
> On 03/09/2017 12:55 AM, akpm@...ux-foundation.org wrote:
> > 
> > The patch titled
> >      Subject: compaction: add def_blk_aops migrate function for memory compaction
> > has been added to the -mm tree.  Its filename is
> >      compaction-add-def_blk_aops-migrate-function-for-memory-compaction.patch
> > 
> > This patch should soon appear at
> >     http://ozlabs.org/~akpm/mmots/broken-out/compaction-add-def_blk_aops-migrate-function-for-memory-compaction.patch
> > and later at
> >     http://ozlabs.org/~akpm/mmotm/broken-out/compaction-add-def_blk_aops-migrate-function-for-memory-compaction.patch
> > 
> > Before you just go and hit "reply", please:
> >    a) Consider who else should be cc'ed
> >    b) Prefer to cc a suitable mailing list as well
> >    c) Ideally: find the original patch on the mailing list and do a
> >       reply-to-all to that, adding suitable additional cc's
> > 
> > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> > 
> > The -mm tree is included into linux-next and is updated
> > there every 3-4 working days
> > 
> > ------------------------------------------------------
> > From: zhouxianrong <zhouxianrong@...wei.com>
> > Subject: compaction: add def_blk_aops migrate function for memory compaction
> 
> That's not really a mm/compaction patch, but a block layer/migration patch. I
> don't know internals of those so well, so I added some CC's.

Thanks!

> > The reason for doing this is based on two factors.
> > 
> > 1. larg file read/write operations with order 0 can fragmentize
> >    memory rapidly.
> > 
> > 2. when a special filesystem does not supply migratepage callback,
> >    kernel would fallback to default function fallback_migrate_page.
> >    but fallback_migrate_page could not migrate diry page nicely;
> >    specially kcompactd with MIGRATE_SYNC_LIGHT could not migrate
> >    diry pages due to this until clear_page_dirty_for_io in some
> >    procedure. i think it is not suitable here in this scenario.
> >    for dirty pages we should migrate it rather than skip or writeout
> >    it in kcomapctd with MIGRATE_SYNC_LIGHT. i think this problem is
> >    for all filesystem without migratepage not only for block device fs.
> > 
> > So for compaction under large file writing supply migratepage for
> > def_blk_aops.
> 
> Is this really safe to do? buffer_migrate_page() has some assumptions listed in
> its comment (and maybe more that are not listed). Do we know it's safe to use it
> for all def_blk_aops users?

So this is definitely *not* OK for ext4 which is using def_blk_aops. Ext4
will create temporary buffer heads that alias buffer heads attached to the
block device page when committing a transaction (see fs/jbd2/journal.c:
jbd2_journal_write_metadata_buffer()) and these will point to the original
page.

Now I'm not saying ext4 / jbd2 cannot be modified so that it somehow makes
it clear to the page migration code it should stay away from the page.
However it demostrates that this patch is not safe as is...

								Honza

> 
> > Link: http://lkml.kernel.org/r/1488937915-78955-1-git-send-email-zhouxianrong@huawei.com
> > Signed-off-by: zhouxianrong <zhouxianrong@...wei.com>
> > Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> > Cc: Johannes Weiner <hannes@...xchg.org>
> > Cc: Minchan Kim <minchan@...nel.org>
> > Cc: Mel Gorman <mgorman@...hsingularity.net>
> > Cc: Vlastimil Babka <vbabka@...e.cz>
> > Cc: Al Viro <viro@...iv.linux.org.uk>
> > Cc: <Mi.Sophia.Wang@...wei.com>
> > Cc: <zhouxiyu@...wei.com>
> > Cc: <weidu.du@...wei.com>
> > Cc: <zhangshiming5@...wei.com>
> > Cc: <won.ho.park@...wei.com>
> > Cc: <zhouxiaoyan1@...wei.com>
> > Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> > ---
> > 
> >  fs/block_dev.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff -puN fs/block_dev.c~compaction-add-def_blk_aops-migrate-function-for-memory-compaction fs/block_dev.c
> > --- a/fs/block_dev.c~compaction-add-def_blk_aops-migrate-function-for-memory-compaction
> > +++ a/fs/block_dev.c
> > @@ -2064,6 +2064,9 @@ static const struct address_space_operat
> >  	.releasepage	= blkdev_releasepage,
> >  	.direct_IO	= blkdev_direct_IO,
> >  	.is_dirty_writeback = buffer_check_dirty_writeback,
> > +#ifdef CONFIG_MIGRATION
> > +	.migratepage = buffer_migrate_page,
> > +#endif
> >  };
> >  
> >  #define	BLKDEV_FALLOC_FL_SUPPORTED					\
> > _
> > 
> > Patches currently in -mm which might be from zhouxianrong@...wei.com are
> > 
> > compaction-add-def_blk_aops-migrate-function-for-memory-compaction.patch
> > 
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR