Re: [PATCH 3/3] mlock: avoid dirtying pages and triggering writeback

Andrew Morton
2010-11-19T22:55:57Z

On Thu, 18 Nov 2010 23:23:16 -0800
Michel Lespinasse <walken@google.com> wrote:

> On Thu, Nov 18, 2010 at 09:41:22AM -0800, Hugh Dickins wrote:
> > On Thu, 18 Nov 2010, Christoph Hellwig wrote:
> > > On Thu, Nov 18, 2010 at 10:11:43AM +1100, Dave Chinner wrote:
> > > > Hence I think that avoiding ->page_mkwrite callouts is likely to
> > > > break some filesystems in subtle, undetected ways. IMO, regardless
> > > > of what is done, it would be really good to start by writing a new
> > > > regression test to exercise and encode the expected the mlock
> > > > behaviour so we can detect regressions later on....
> > >
> > > I think it would help if we could drink a bit of the test driven design
> > > coolaid here. Michel, can you write some testcases where pages on a
> > > shared mapping are mlocked, then dirtied and then munlocked, and then
> > > written out using msync/fsync. Anything that fails this test on
> > > btrfs/ext4/gfs/xfs/etc obviously doesn't work.
>
> I think it's still under debate what's an acceptable result for this test
> (i.e. what's supposed to happen during mlock of a shared mapping of
> a sparsely allocated file - is a fallocate equivalent supposed to happen ?)
> But I agree discussing based on test results will make things more concrete.
>
> > Whilst it's hard to argue against a request for testing, Dave's worries
> > just sprang from a misunderstanding of all the talk about "avoiding ->
> > page_mkwrite". There's nothing strange or risky about Michel's patch,
> > it does not avoid ->page_mkwrite when there is a write: it just stops
> > pretending that there was a write when locking down the shared area.
>
> So, I decided to test this using memtoy.

Wait. You *tested* the kernel?

I dunno, kids these days...

> /data is a separate partition
> where I had just 10GB free space, and /data/hole20G was created using
> dd if=/dev/zero of=/data/hole20G bs=1M seek=20480 count=0.
>
> memtoy>file /data/hole20G shared
> memtoy>map hole20G
>
> At this point the file is mapped using a writable, shared VMA.
>
> memtoy>touch hole20G
> memtoy: touched 5242880 pages in 30.595 secs
>
> At this point memtoy's address space is populated with zeroed
> pages. The pages are distinct (meminfo does show 20G of allocated pages),
> are classified as file pages, not anon, and are associated with the
> struct address_space for /data/hole20G. That file still does not have
> blocks allocated, as can be seen with du /data/hole20G.
>
> memtoy>lock hole20G
>
> memtoy tries to mlock the hole20G VMA.
> This is where things get interesting.
>
> Using ext2, without my patch (ext3 should be similar):
> - first, mlock does fast progress going though file pages, marking them
> as dirty. Eventually, it hits the dirty limit and gets throttled.
> - then, mlock does slow progress as it needs to wait for writeback.
> writeback occurs and allocates blocks for the /data/hole20G.
> Eventually, the /data partition gets full.
> - then, mlock does no progress as it's at the dirty limit and nothing
> gets written back.
> - mlock never terminates.
>
> Using ext2, with my patch (ext3 should be similar):
> - mlock goes through all pages in ~5 seconds, marking them as mlocked
> (but still not dirty)
> - mlock completes. /data/hole20G still does not have blocks allocated.
> - if memtoy is then instructed to dirty all the pages
> (using 'touch hole20G write'):
> - first, memtoy does fast progress faulting through file pages, marking
> them as dirty. Eventually, it hits the dirty limit and gets throttled.
> - then, memtoy does slow progress as it needs to wait for writeback.
> writeback occurs and allocates blocks for the /data/hole20G.
> Eventually, the /data partition gets full.
> - then, memtoy does no progress as it's at the dirty limit and nothing
> gets written back. It gets stuck into a write fault that never
> completes.
> - i.e. this is essentially the same lockup as without my patch, except that
> it occurs when the application tries to dirty the shared file rather than
> during mlock itself.

Seems to me that this bug is the first thing we should be looking at.

> Using ext4, without my patch:
> - first, mlock does fast progress going though file pages, marking them
> as dirty. Eventually, it hits the dirty limit and gets throttled.
> - then, mlock does slow progress as it needs to wait for writeback.
> writeback occurs and allocates blocks for the /data/hole20G.
> Eventually, the /data partition gets full.
> - then, mlock returns an error.
>
> Using ext4, with my patch:
> - mlock goes through all pages in ~5 seconds, marking them as mlocked
> (but still not dirty)
> - mlock completes. /data/hole20G still does not have blocks allocated.
> - if memtoy is then instructed to dirty all the pages
> (using 'touch hole20G write'):
> - first, memtoy does fast progress faulting through file pages, marking
> them as dirty. Eventually, it hits the dirty limit and gets throttled.
> - then, memtoy does slow progress as it needs to wait for writeback.
> writeback occurs and allocates blocks for the /data/hole20G.
> Eventually, the /data partition gets full.
> - at that point, memtoy dies of SIGBUS.
> - i.e. for filesystems that define the page_mkwrite callback, the mlock
> behavior when running out of space writing to sparse files is clearly
> nicer without my patch than with it.
>
>
> Not 100% sure what to make of these results.
>
> Approaching the problem the other way - would there be any objection to
> adding code to do an fallocate() equivalent at the start of mlock ?
> This would be a no-op when the file is fully allocated on disk, and would
> allow mlock to return an error if the file can't get fully allocated
> (no idea what errno should be for such case, though).

Dirtying all that memory at mlock() time is pretty obnoxious.

I'm inclined to agree that your patch implements the desirable
behaviour: don't dirty the page, don't do block allocation. Take a
fault at first-dirtying and do it then. This does degrade mlock a bit:
the user will find that the first touch of an mlocked page can cause
synchronous physical I/O, which isn't mlocky behaviour *at all*. But
we have to be able to do this anyway - whenever the kupdate function
writes back the dirty pages it has to mark them read-only again so the
kernel knows when they get redirtied.

I do agree that this will result in worse file layout for some
reasonable userspace code patterns. But it was always that way, except
for the eleven-release window where we kinda accidentally fixed that up
in-kernel. Hopefully most apps which care are already ensuring that
the file is well laid-out.

So all that leaves me thinking that we merge your patches as-is. Then
work out why users can fairly trivially use mlock to hang the kernel on
ext2 and ext3 (and others?)