Comments

On 4/15/14, 11:02 AM, Lukáš Czerner wrote:
> On Sun, 13 Apr 2014, Theodore Ts'o wrote:> >> Date: Sun, 13 Apr 2014 18:00:16 -0400>> From: Theodore Ts'o <tytso@mit.edu>>> To: Ext4 Developers List <linux-ext4@vger.kernel.org>>> Cc: Namjae Jeon <linkinjeon@gmail.com>>> Subject: Re: [PATCH] ext4: add fallocate mode blocking for debugging purposes>>>> On Sun, Apr 13, 2014 at 04:21:58PM -0400, Theodore Ts'o wrote:>>> If a particular fallocate mode is causing test failures, give the>>> tester the ability to block a particular fallocate mode so that the>>> use of a particular fallocate mode will be reported as not supported.>>>>>> For example, if the COLLAPSE_RANGE fallocate mode is causing test>>> failures, this allows us to suppress it so we can more easily test the>>> rest of the file system code.>>>> Hi Namjae,>>>> One of the reasons this patch set is that after Lukas added>> COLLAPSE_RANGE support into fsx, we've started seeing a number of>> failures which seem to be directly related to COLLAPSE_RANGE.> > Ah, I did mentioned it when I added COLLAPSE_RANGE to the fsx and> fsstress, but I forgot to cc you Namjae, sorry about that.> > But about the patch. It seems a little bit weird to change kernel> for this. The way I am doing it is by changing ltp/fsx.c and> ltp.fsstress.c to disable the particular mode:
I tend to agree, better to fix the kernel than to add a knob to turn it
off. And fsx changes can happen a lot quicker than kernel changes. [1]
And if it's really unsafe, and you really want to add a knob, I'd at least
default it to off until it's non-corrupting, and add a message that
this tunable will go away as soon as it's stable, so you'll have no
qualms about quickly deprecating it...
-Eric
[1] it'd be nifty to make an env. var in xfstests which can globally
disable certain fsx operations across all tests which run fsx...
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Tue, Apr 15, 2014 at 11:15:41AM -0500, Eric Sandeen wrote:
> > I tend to agree, better to fix the kernel than to add a knob to turn it> off. And fsx changes can happen a lot quicker than kernel changes. [1]> > And if it's really unsafe, and you really want to add a knob, I'd at least> default it to off until it's non-corrupting, and add a message that> this tunable will go away as soon as it's stable, so you'll have no> qualms about quickly deprecating it...
Yeah, I went back and forth on this. One of there reasons why I added
a kernel knob is that *I* can make the kernel change a lot faster than
it would be to tweak all of the various xfstests program to globally
disable certain operations in fsx, fstress, etc.
I also had a sneaking suspicion that we might have a similar issue
with the INSERT RANGE patches which are coming down the pike, and so
having a general way of also being able INSERT RANGE if to be able to
quickly determine whether a potential bug was caused by INSERT RANGE
or some other pending changes might also be useful.
I freely admit it is a bit of a hack, though. Does the hack smell
less bad if we wrap it in CONFIG_EXT4FS_DEBUG?
> [1] it'd be nifty to make an env. var in xfstests which can globally> disable certain fsx operations across all tests which run fsx...
Yes, although as I mentioned above, it would be really nice if it
worked across all of the various tests, and not just be limited to
fsx, or even just fsx and fstress.
- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On 4/15/14, 1:44 PM, Theodore Ts'o wrote:
> On Tue, Apr 15, 2014 at 11:15:41AM -0500, Eric Sandeen wrote:>>>> I tend to agree, better to fix the kernel than to add a knob to turn it>> off. And fsx changes can happen a lot quicker than kernel changes. [1]>>>> And if it's really unsafe, and you really want to add a knob, I'd at least>> default it to off until it's non-corrupting, and add a message that>> this tunable will go away as soon as it's stable, so you'll have no>> qualms about quickly deprecating it...> > Yeah, I went back and forth on this. One of there reasons why I added> a kernel knob is that *I* can make the kernel change a lot faster than> it would be to tweak all of the various xfstests program to globally> disable certain operations in fsx, fstress, etc.> > I also had a sneaking suspicion that we might have a similar issue> with the INSERT RANGE patches which are coming down the pike, and so> having a general way of also being able INSERT RANGE if to be able to> quickly determine whether a potential bug was caused by INSERT RANGE> or some other pending changes might also be useful.> > I freely admit it is a bit of a hack, though. Does the hack smell> less bad if we wrap it in CONFIG_EXT4FS_DEBUG?> >> [1] it'd be nifty to make an env. var in xfstests which can globally>> disable certain fsx operations across all tests which run fsx...> > Yes, although as I mentioned above, it would be really nice if it> worked across all of the various tests, and not just be limited to> fsx, or even just fsx and fstress.
Well, for tests which are specific to collapse range, it'd be trivial
to add a "collapse" group, and exclude it.
For generic stress tests which happen to do collapse range, it'd take
a bit more. But that'd probably still be the generic solution.
XFS got bitten too, there were collapse range problems. But the fixes
are already in the pipe AFAIK.
-Eric
> - Ted> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On 4/15/14, 1:44 PM, Theodore Ts'o wrote:
> On Tue, Apr 15, 2014 at 11:15:41AM -0500, Eric Sandeen wrote:>>>> I tend to agree, better to fix the kernel than to add a knob to turn it>> off. And fsx changes can happen a lot quicker than kernel changes. [1]>>>> And if it's really unsafe, and you really want to add a knob, I'd at least>> default it to off until it's non-corrupting, and add a message that>> this tunable will go away as soon as it's stable, so you'll have no>> qualms about quickly deprecating it...> > Yeah, I went back and forth on this. One of there reasons why I added> a kernel knob is that *I* can make the kernel change a lot faster than> it would be to tweak all of the various xfstests program to globally> disable certain operations in fsx, fstress, etc.> > I also had a sneaking suspicion that we might have a similar issue> with the INSERT RANGE patches which are coming down the pike, and so> having a general way of also being able INSERT RANGE if to be able to> quickly determine whether a potential bug was caused by INSERT RANGE> or some other pending changes might also be useful.
Also: I'd humbly suggest just not merging those until they pass stringent
tests like fsx & fsstress...
Adding a pre-emptive knob to turn them off post-merge when they turn
out to be broken sounds backwards to me...
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Tue, Apr 15, 2014 at 02:44:42PM -0400, Theodore Ts'o wrote:
> On Tue, Apr 15, 2014 at 11:15:41AM -0500, Eric Sandeen wrote:> > > > I tend to agree, better to fix the kernel than to add a knob to turn it> > off. And fsx changes can happen a lot quicker than kernel changes. [1]> > > > And if it's really unsafe, and you really want to add a knob, I'd at least> > default it to off until it's non-corrupting, and add a message that> > this tunable will go away as soon as it's stable, so you'll have no> > qualms about quickly deprecating it...> > Yeah, I went back and forth on this. One of there reasons why I added> a kernel knob is that *I* can make the kernel change a lot faster than> it would be to tweak all of the various xfstests program to globally> disable certain operations in fsx, fstress, etc.
Actually, we shouldn't be changing xfstests or adding workarounds in
the kernel to avoid certain operations. We should be fixing the damn
bugs that are being exposed.
Yes, the addition of zero range and collapse range to fsx and
fsstress has exposed bugs in the XFS code as well, and that causes
assert failures all over the place. But that's a *good thing* - now
those bugs are all fixed and ready to be sent to Linus:
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/xfs.git;a=shortlog;h=refs/heads/xfs-fixes-for-3.15-rc2
And so in a couple of days the problem goes away for everyone using
XFS. Do the same (i.e. fix the bugs) for ext4, and the problem goes
away.
> I also had a sneaking suspicion that we might have a similar issue> with the INSERT RANGE patches which are coming down the pike, and so> having a general way of also being able INSERT RANGE if to be able to> quickly determine whether a potential bug was caused by INSERT RANGE> or some other pending changes might also be useful.
Well, only if you ignore the lesson we've just learnt.
i.e. that we have to add the FALLOC_FL_INSERT_RANGE to fsx and
fsstress as well as having corner case tests and it needs to pass
those tests before XFS support is ready for upstream inclusion. At
least, that's the lesson I learnt from as the xfstests and XFS
Maintainer - we didn't put the QA bar for inclusion high enough, and
so problems slipped through.
If you want to add more strict testing requirements for ext4
inclusion, then you're welcome to request them for the ext4
implementation of that functionality. You don't have to accept the
code until you're happy with it....
Cheers,
Dave.

On Tue, Apr 15, 2014 at 05:32:43PM -0500, Eric Sandeen wrote:
> > I also had a sneaking suspicion that we might have a similar issue> > with the INSERT RANGE patches which are coming down the pike, and so> > having a general way of also being able INSERT RANGE if to be able to> > quickly determine whether a potential bug was caused by INSERT RANGE> > or some other pending changes might also be useful.> > Also: I'd humbly suggest just not merging those until they pass stringent> tests like fsx & fsstress...> > Adding a pre-emptive knob to turn them off post-merge when they turn> out to be broken sounds backwards to me...
Having learned from COLLAPSE RANGE, I agree. The fact that we didn't
have full testing during the whole development cycle was unfortunate.
And we got lucky with the renameat patches, since I wasn't able to get
tha testing done because the xfstests commits didn't get merged until
*after* the they renameat commits got merged, and also because I
didn't notice that the i386 system call wasn't wired up when I was
doing my manual "just before I push to Linus" testing.
I plan on insisting that INSERT RANGE support being in the VFS, and be
fully enabled, and that we have full INSERT RANGE testing into
xfstests, during the development cycle. Some of the work that I've
been doing with kvm-xfstests and why I created a github tytso/xfstests
git tree is specifically to make sure things go much more smoothly
this time around. (That way, if there is some new fs feature patch,
such as COLLAPSE RANGE or renameat(2) where the tests are still being
refined for xfstests inclusion, we can still have something we can all
use on an interim basis during the development cycle.)
However, with all of this being said, while new feature patches such
sa INSERT RANGE are cooking in the ext4.git tree, so that multiple
developers *can* do that testing, having a knob to turn the feature on
and off without having to do a kernel recompile is convenient.
Cheers,
- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[added xfs@oss.sgi.com]
On Tue, Apr 15, 2014 at 07:30:39PM -0400, Theodore Ts'o wrote:
> On Tue, Apr 15, 2014 at 05:32:43PM -0500, Eric Sandeen wrote:> > > I also had a sneaking suspicion that we might have a similar issue> > > with the INSERT RANGE patches which are coming down the pike, and so> > > having a general way of also being able INSERT RANGE if to be able to> > > quickly determine whether a potential bug was caused by INSERT RANGE> > > or some other pending changes might also be useful.> > > > Also: I'd humbly suggest just not merging those until they pass stringent> > tests like fsx & fsstress...> > > > Adding a pre-emptive knob to turn them off post-merge when they turn> > out to be broken sounds backwards to me...> > Having learned from COLLAPSE RANGE, I agree. The fact that we didn't> have full testing during the whole development cycle was unfortunate.> And we got lucky with the renameat patches, since I wasn't able to get> tha testing done because the xfstests commits didn't get merged until> *after* the they renameat commits got merged, and also because I> didn't notice that the i386 system call wasn't wired up when I was> doing my manual "just before I push to Linus" testing.
I asked for renameat2 tests long before inclusion occurred. The fact
is that we can't co-ordinate xfstests inclusion for a feature that
we don't even know is going to be included until someone sends Linus
a pull request....
> I plan on insisting that INSERT RANGE support being in the VFS, and be> fully enabled, and that we have full INSERT RANGE testing into> xfstests, during the development cycle.
There wasn't a problem with the timing of xfstests inclusion - the
problem was with the fact we didn't have sufficient QA coverage in
xfstests when the initial upstream kernel commits occurred. This
time around, the difference will be that this time we'll have fsx
and fsstress coverage *before* kernel support is added, and I've
already asked for that:
http://oss.sgi.com/archives/xfs/2014-04/msg00121.html
> Some of the work that I've> been doing with kvm-xfstests and why I created a github tytso/xfstests> git tree is specifically to make sure things go much more smoothly> this time around.
Ted, this looks and sounds like you're preparing to fork xfstests.
Why? What's the problem with working upstream on test development
and refinement like everyone else does?
This thread is a demonstration of how avoiding upstream interaction
results in nasty hacks being proposed. If you asked the question on
the xfs mailing list of how to avoid various fsstress/fsx
operations, we woul dhave told you that using FSSTRESS_AVOID and
adding an equivalent FSX_AVOID to xfstests is all that is needed.
i.e. we already have partial infrastructure support for what you
need in xfstests and it would be about 30 minutes work to add
FSX_AVOID....
Is that fast enough for you?
Indeed, we could also use similar env vars to ensure various
_requires_* checks fail and to populate FSSTRESS_AVOID/FSX_AVOID
automatically and so tests that require this functionality are not
run.
IOWs, it's in your best interests to work with upstream to add the
functionality you require to xfstests. History tells us that forking
development into private repositories has never worked out well for
anyone, so I'd really, really like you to *at least try* to work
with upstream as your primary test development environment....
Cheers,
Dave.

On Wed, Apr 16, 2014 at 09:25:56AM +1000, Dave Chinner wrote:
> Actually, we shouldn't be changing xfstests or adding workarounds in> the kernel to avoid certain operations. We should be fixing the damn> bugs that are being exposed.
Well, I'm waiting for Namjae to look into the test failures.
Unfortunately I don't have time right now to fix it myself.
In the meantime, I wanted to do a full baseline test run to make sure
we didn't have any other regressions or failures post -rc1, and so
being able to filter out collapse range allowed me to kick off a test
of the rest of the patches I was hoping to push to Linus for -rc2.
> i.e. that we have to add the FALLOC_FL_INSERT_RANGE to fsx and> fsstress as well as having corner case tests and it needs to pass> those tests before XFS support is ready for upstream inclusion. At> least, that's the lesson I learnt from as the xfstests and XFS> Maintainer - we didn't put the QA bar for inclusion high enough, and> so problems slipped through.> > If you want to add more strict testing requirements for ext4> inclusion, then you're welcome to request them for the ext4> implementation of that functionality. You don't have to accept the> code until you're happy with it....
No arguments here; I plan to do the same.
Regards,
- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Wed, Apr 16, 2014 at 10:06:35AM +1000, Dave Chinner wrote:
> > Some of the work that I've> > been doing with kvm-xfstests and why I created a github tytso/xfstests> > git tree is specifically to make sure things go much more smoothly> > this time around.> > Ted, this looks and sounds like you're preparing to fork xfstests.> Why? What's the problem with working upstream on test development> and refinement like everyone else does?
I'd prefer not to fork xfstests. However, I do want to get more ext4
developers using automated xfstests testing, so I can scale better.
In order to do that, I need to be able to make it really easy for
people to who aren't hard-core xfstests people to be able to use it.
One of the nice things about kvm-xfstests is that a *lot* easier for
people to figure out how to use it. If I can lower the activation
energy required to get people to use xfstests, it saves me time in the
end.
The reason why I created the github repository is because if I'm going
to be shipping a KVM test appliance image that people can use in a
turn-key environment, I'd prefer that all of the sources, including
any local changes that I might need to make the tests run as smoothly
as possible, are available in a public repository. (And at one point,
I did have up to 12 local changes, which is why I wanted it tracked in
a repo.)
Every single local change I made was either a test or commit that
hadn't been accepted into the upstream xfstests repository yet, or a
fix I wrote that I sent upstream. And as soon as the fixes made it
into the upstream xfstests repository, I rebased them away. At the
moment, there's only once commit in my xfstests github repository
which isn't upstream and it's the:
check: add support for an external file containing tests to exclude
commit for which I've sent the V2 version to you.
So for the most part, I want to keep the repo as close to upstream as
possible, and ideally identical to upstream, and I've been working
towards that end.
> This thread is a demonstration of how avoiding upstream interaction> results in nasty hacks being proposed. If you asked the question on> the xfs mailing list of how to avoid various fsstress/fsx> operations, we woul dhave told you that using FSSTRESS_AVOID and> adding an equivalent FSX_AVOID to xfstests is all that is needed.> i.e. we already have partial infrastructure support for what you> need in xfstests and it would be about 30 minutes work to add> FSX_AVOID....> > Is that fast enough for you?> > Indeed, we could also use similar env vars to ensure various> _requires_* checks fail and to populate FSSTRESS_AVOID/FSX_AVOID> automatically and so tests that require this functionality are not> run.
Well, it took me about 1 minute to write the dozen line kernel patch.
I really didn't want to ask you to make changes to xfstests for me,
but if you're willing to make those changes, that would be great. I
really didn't want to presume, though. And if the answer is that I
need to spend the time making all of these changes --- I'll try, but
if I don't have time, I may end up taking the more expedient path.
> IOWs, it's in your best interests to work with upstream to add the> functionality you require to xfstests. History tells us that forking> development into private repositories has never worked out well for> anyone, so I'd really, really like you to *at least try* to work> with upstream as your primary test development environment....
As I said, every single patch which I put in my local xfstests tree I
also sent upstream.
That being said, I wasn't sure whether you were going to accept that
last change, since there was similar, but for me, not usable
functionality in the form of the -X option. So if you weren't going
to accept a change to allow the excluded list of tests to be kept in a
single file outside of the tests/* subdirectory, I probably would have
carried it as a separate patch --- because it's something I need, and
the current -X functionality really isn't easy to maintain (you need
to have many more files, and they have to be dropped into the xfstests
tests/* subdirectory).
I know that you and I haven't seen eye to eye in the past. For
example, the NO_HIDE_STALE out of tree patch which is running on
thousands and thousands numbers of machines inside Google, but which
the XFS folks have considered evil incarnate. I will freely admit
that I'm much more of a pragmatist and much less of a purist on
certain matters.
So sure, I'm certainly going to _try_ to work with upstream xfstests.
I've done that to date. But I'm certainly not going to presume that
you're going to like or accept all of the changes I might want to
propose.
Regards,
- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html