| Message ID | alpine.LFD.2.00.1404151749490.2146@localhost.localdomain |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
diff --git a/ltp/fsstress.c b/ltp/fsstress.c index 1eec11a..29b790b 100644 --- a/ltp/fsstress.c +++ b/ltp/fsstress.c @@ -208,7 +208,7 @@ opdesc_t ops[] = { { OP_MKNOD, "mknod", mknod_f, 2, 1 }, { OP_PUNCH, "punch", punch_f, 1, 1 }, { OP_ZERO, "zero", zero_f, 1, 1 }, - { OP_COLLAPSE, "collapse", collapse_f, 1, 1 }, + { OP_COLLAPSE, "collapse", collapse_f, 0, 1 }, { OP_READ, "read", read_f, 1, 0 }, { OP_READLINK, "readlink", readlink_f, 1, 0 }, { OP_RENAME, "rename", rename_f, 2, 1 }, diff --git a/ltp/fsx.c b/ltp/fsx.c index 47d3ee8..194d7a3 100644 --- a/ltp/fsx.c +++ b/ltp/fsx.c @@ -144,7 +144,7 @@ int mapped_writes = 1; /* -W flag disables */ int fallocate_calls = 1; /* -F flag disables */ int punch_hole_calls = 1; /* -H flag disables */ int zero_range_calls = 1; /* -z flag disables */ -int collapse_range_calls = 1; /* -C flag disables */ +int collapse_range_calls = 0; /* -C flag disables */ int mapped_reads = 1; /* -R flag disables it */ int fsxgoodfd = 0; int o_direct; /* -Z */