Message-ID: <alpine.LSU.2.11.1402251217030.2380@eggly.anvils>
Date:	Tue, 25 Feb 2014 15:23:35 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Namjae Jeon <linkinjeon@...il.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Matthew Wilcox <matthew@....cx>, Theodore Ts'o <tytso@....edu>,
	Dave Chinner <david@...morbit.com>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	viro@...iv.linux.org.uk, bpm@....com, adilger.kernel@...ger.ca,
	jack@...e.cz, mtk.manpages@...il.com, lczerner@...hat.com,
	linux-fsdevel@...r.kernel.org, xfs@....sgi.com,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Namjae Jeon <namjae.jeon@...sung.com>
Subject: Re: [PATCH v5 0/10] fs: Introduce new flag(FALLOC_FL_COLLAPSE_RANGE)
 for fallocate

On Tue, 25 Feb 2014, Dave Chinner wrote:
> On Tue, Feb 25, 2014 at 02:16:01PM +1100, Stephen Rothwell wrote:
> > On Mon, 24 Feb 2014 11:57:10 +1100 Dave Chinner <david@...morbit.com> wrote:
> > >
> > > > Namjae Jeon (10):
> > > >   fs: Add new flag(FALLOC_FL_COLLAPSE_RANGE) for fallocate
> > > >   xfs: Add support FALLOC_FL_COLLAPSE_RANGE for fallocate
> > > 
> > > I've pushed these to the following branch:
> > > 
> > > 	git://oss.sgi.com/xfs/xfs.git xfs-collapse-range
> > > 
> > > And so they'll be in tomorrow's linux-next tree.
> > > 
> > > >   ext4: Add support FALLOC_FL_COLLAPSE_RANGE for fallocate
> > > 
> > > I've left this one alone for the ext4 guys to sort out.
> > 
> > So presumably that xfs tree branch is now completely stable and so Ted
> > could just merge that branch into the ext4 tree as well and put the ext4
> > part on top of that in his tree.
> 
> Well, for some definition of stable. Right now it's just a topic
> branch that is merged into the for-next branch, so in theory it is
> still just a set of pending changes in a branch in a repo that has
> been pushed to linux-next for testing.
> 
> That said, I don't see that branch changing unless we find bugs in
> the code or a problem with the API needs fixing, at which point I
> would add more commits to it and rebase the for-next branch that you
> are pulling into the linux-next tree.
> 
> Realistically, I'm waiting for Lukas to repost his other pending
> fallocate changes (the zero range changes) so I can pull the VFS and
> XFS bits of that into the XFS tree and I can test them together
> before I'll call the xfs-collapse-range stable and ready to be
> merged into some other tree...

Thank you, Namjae and Dave, for driving this; and thank you, Ted and
Matthew, for raising appropriate mmap concerns (2013-7-31 and 2014-2-2).
I was aware of this work in progress, but only now found time to look.

I've not studied the implementation, knowing too little of ext4 and
xfs; but it sounds like the approach you've taken, writing out dirties
and truncating all pagecache from the critical offset onwards, is the
sensible approach for now - lame, and leaves me wondering whether an
offline tool wouldn't be more appropriate; but a safe place to start
if we suppose it will be extended to handle pagecache better in future.

Of course I'm interested in the possibility of extending it to tmpfs;
which may not be a worthwhile exercise in itself, except that it would
force us to face and solve any pagecache/radixtree issues, if possible,
thereby enhancing the support for disk-based filesystems.

I doubt we should look into that before Jan Kara's range locking mods
arrive, or are rejected.  As I understand it, you're going ahead with
this, knowing that there can be awkward races with concurrent faults -
more likely to cause trinity fuzzer reports, than interfere with daily
usage (trinity seems to be good at faulting into holes being punched).

That's probably the right pragmatic decision; but I'm a little worried
that it's justfied by saying we already have such races in hole-punch.
Collapse is significantly more challenging than either hole-punch or
truncation: the shifting of pages from one offset to another is new,
and might present nastier bugs.

Emphasis on "might": I expect it's impossible, given your current
approach, but something to be on guard against is unmap_mapping_range()
failing to find and unmap a pte, because the page is mapped at the
"wrong" place in the vma, resulting in BUG_ON(page_mapped(page))
in __delete_from_page_cache().

One thing that is slightly wrong in what you have right now, is your
use of truncate_pagecache_range(): you'll need to add an "even_cows"
arg to that (or make it a wrapper to a __truncate_pagecache_range()
taking additional "even_cows" arg).  That arg governs what is done
with anonymous COW'ed pages in a MAP_PRIVATE mmap of the file.

Truncation is required by spec to unmap them; hole punching was up
to us to spec, did not unmap them originally (because there was no
preliminary call to unmap_mapping_range(), so it happened to rely
on the inefficient fallback within truncate_inode_page()), and that
seemed fine because, why discard a user's data unnecessarily?

But your case is different: collapse is much closer to truncation,
and if you do not unmap the private COW'ed pages, then pages left
behind beyond the EOF will break the spec that requires SIGBUS when
touching there, and pages within EOF will be confusingly derived
from file data now belonging to another offset or none (move these
pages within the user address space? no, I don't think anon_vmas
would allow that, and there may be no right place to move them).

It's clear that the right and easy thing to do is just to unmap
them (all of them, from critical offset to EOF), in the rare case
of there being any such pages.  Whether this detail needs to be
mentioned in the man page (I don't like throwing away a user's
data without warning) I'm not sure, Michael can judge.

FALLOC_FL_COLLAPSE_RANGE: I'm a little sad at the name COLLAPSE,
but probably seven months too late to object.  It surprises me that
you're doing all this work to deflate a part of the file, without
the obvious complementary work to inflate it - presumably all those
advertisers whose ads you're cutting out, will come back to us soon
to ask for inflation, so that they have somewhere to reinsert them ;)

But you have the good precedent of "truncate" being used to extend
files, so I suppose "collapse" can one day be enhanced to inflate a
file when given negative len.  Or perhaps, like truncate, that would
lead to too much "if (bigger) do_one_thing() else something_else()",
and a separate FALLOC_FL_ would prove better.  Certainly there's no
requirement that you should implement this, I was just a little
surprised that you had not.

I should mention that when "we" implemented this thirty years ago,
we had a strong conviction that the system call should be idempotent:
that is, the len argument should indicate the final i_size, not the
amount being removed from it.  Now, I don't remember the grounds for
that conviction: maybe it was just an idealistic preference for how
to design a good system call.  I can certainly see that defining it
that way round would surprise many app programmers.  Just mentioning
this in case anyone on these lists sees a practical advantage to
doing it that way instead.

I see you've included xfstests and xfs_io updates, nice.  Did you
realize that util-linux has a /usr/bin/fallocate?  I hope someone
will update that too.

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

Powered by blists - more mailing lists