From: Dave Chinner <david@fromorbit.com>
To: Theodore Ts'o <tytso@mit.edu>
Cc: fstests@vger.kernel.org, hughd@google.com, Junho Ryu <jayr@google.com>
Subject: Re: [PATCH 05/12] xfstests: do not unmount tmpfs during remount
Date: Thu, 11 Feb 2016 10:07:00 +1100 [thread overview]
Message-ID: <20160210230700.GA19486@dastard> (raw)
In-Reply-To: <20160210160732.GE26922@thunk.org>
On Wed, Feb 10, 2016 at 11:07:32AM -0500, Theodore Ts'o wrote:
> On Wed, Feb 10, 2016 at 05:07:16PM +1100, Dave Chinner wrote:
> > > # Remounting with nodiratime option
> > > -_scratch_unmount
> > > -_scratch_mount "-o nodiratime"
> > > +_scratch_remount "-o nodiratime"
> >
> > This makes me go "no, that can't work, nodiratime is not an option
> > that we allow on remount."
>
> Hmm, yes, we're not consistent here. xfs doesn't allow nodiratime on
> remounts. ext4 allows nodiratime on remount, but not diratime, so
> there's no way to clear nodiratime once its set. tmpfs allows
> diratime and nodiratime on remount.
>
> I wonder if it's worth it make things more consistent across the
> various file systems. What do you think?
Different problem, care factor approaching zero right now. :/
Talk to Eric - he's futzing with this stuff on XFS right now.
> > So, at minimum, the name of the helper needs to get changed so that
> > it doesn't imply that a "-o remount" with new options is being
> > done...
>
> Hmm, how about having two helper functions: _scratch_remount that
> doesn't take any arguments, and a _scratch_change_mount_opts which
> does?
No, it's not really the options that are the problem here. The
problem is -o remount vs unmount/mount and what the test is actually
expecting.
I'd say "_scratch_remount" should do "-o remount" unconditionally
(least surprise) and the current _scratch_remount should be changed
to something like _scratch_cycle_mount(). That way both can take
options, but it's clear they do different things. tmpfs can simply
implement them the same way.
> > > -_umount_mount
> > > +_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
> > > +_scratch_mount > /dev/null 2>&1 || _fail "mount failed"
> >
> > Please don't add _fail to mkfs/mount like this, especially where the
> > test doesn't already have them.
>
> Could you say more about why? We do have tests that do use _fail if
> the mkfs or mount fails. Should we be changing them to remove it?
Because, in general, scratch_mkfs should not ever fail, and nor
should a mounting the scratch device. If they do fail, something has
already gone wrong...
> But if we do that, and the mount fails for some reason, then won't
> things stagger on and perhaps make life harder to debug.
Yes, but we don't care that they make lots of noise or that they
stagger on, because that staggering on can exercise failure and
other unexpected paths that wouldn't otherwise get exercised. I've
lost count of the number of times that I've had a stuck process
prevent an unmount and a subsequent test has tripped over and
uncovered a previously unknown bug because we allowed mkfs and mount
to fail....
As for debugging - you always have to go back to the first failure
you see in xfstests and work from there, so the subsequent noise of
other tests failing can simply be ignored. Yes, it can make it a
little harder to identify the first failure, but it only takes a few
seconds looking at log files to identify that these functions have
failed...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2016-02-10 23:09 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-10 1:49 [PATCH 00/12] xfstests: fix up various tmpfs failures Theodore Ts'o
2016-02-10 1:49 ` [PATCH 01/12] check: avoid error messages of tests/$FS does not exist Theodore Ts'o
2016-02-10 5:45 ` Dave Chinner
2016-02-10 15:34 ` Theodore Ts'o
2016-02-10 16:28 ` Christoph Hellwig
2016-02-10 23:17 ` Theodore Ts'o
2016-02-10 22:19 ` Dave Chinner
2016-02-10 23:32 ` Theodore Ts'o
2016-02-10 1:49 ` [PATCH 02/12] common: _scratch_mkfs_sized() for tmpfs Theodore Ts'o
2016-02-10 6:00 ` Dave Chinner
2016-02-10 15:58 ` Theodore Ts'o
2016-02-10 22:37 ` Dave Chinner
2016-02-10 1:49 ` [PATCH 03/12] generic: use mount point instead of device name Theodore Ts'o
2016-02-10 1:49 ` [PATCH 04/12] generic: add _require_odirect to three more tests Theodore Ts'o
2016-02-10 9:15 ` Eryu Guan
2016-02-10 16:11 ` Theodore Ts'o
2016-02-10 22:51 ` Dave Chinner
2016-02-10 23:21 ` Theodore Ts'o
2016-02-10 1:49 ` [PATCH 05/12] xfstests: do not unmount tmpfs during remount Theodore Ts'o
2016-02-10 6:07 ` Dave Chinner
2016-02-10 16:07 ` Theodore Ts'o
2016-02-10 18:04 ` Theodore Ts'o
2016-02-10 23:07 ` Dave Chinner [this message]
2016-02-10 23:28 ` Theodore Ts'o
2016-02-11 3:07 ` Dave Chinner
2016-02-11 15:25 ` Theodore Ts'o
2016-02-11 17:36 ` Darrick J. Wong
2016-02-10 1:49 ` [PATCH 06/12] generic: do not unmount before calling _check_scratch_fs() Theodore Ts'o
2016-02-10 1:49 ` [PATCH 07/12] generic: require fiemap for generic/009 Theodore Ts'o
2016-02-10 1:49 ` [PATCH 08/12] xfstests: fix generic/312 on tmpfs, ignore /proc/partitions Theodore Ts'o
2016-02-10 5:54 ` Dave Chinner
2016-02-10 23:39 ` Theodore Ts'o
2016-02-11 2:53 ` Dave Chinner
2016-02-10 1:49 ` [PATCH 09/12] xfstests: generic/079 requires chattr, not xattrs Theodore Ts'o
2016-02-10 9:09 ` Eryu Guan
2016-02-10 16:09 ` Theodore Ts'o
2016-02-10 1:49 ` [PATCH 10/12] generic: disable generic/027 for tmpfs Theodore Ts'o
2016-02-10 5:58 ` Dave Chinner
2016-02-10 15:48 ` Theodore Ts'o
2016-02-10 23:13 ` Dave Chinner
2016-02-10 1:50 ` [PATCH 11/12] xfstests: add executable permission to tests Theodore Ts'o
2016-02-10 9:07 ` Eryu Guan
2016-02-10 1:50 ` [PATCH 12/12] xfstests: increase tmpfs memory size Theodore Ts'o
2016-02-10 2:10 ` [PATCH 00/12] xfstests: fix up various tmpfs failures Theodore Ts'o
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160210230700.GA19486@dastard \
--to=david@fromorbit.com \
--cc=fstests@vger.kernel.org \
--cc=hughd@google.com \
--cc=jayr@google.com \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox