From: Dave Chinner <david@fromorbit.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-xfs@vger.kernel.org, fstests <fstests@vger.kernel.org>
Subject: Re: [PATCH 6/6] libxcmd: add non-iterating user commands
Date: Thu, 8 Dec 2016 07:16:37 +1100 [thread overview]
Message-ID: <20161207201637.GI4219@dastard> (raw)
In-Reply-To: <CAOQ4uxhrtaHauNJypezpFAT0VCN61oZLFKtxuBLw32jn7FOSFw@mail.gmail.com>
On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote:
> On Wed, Dec 7, 2016 at 6:49 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> From: Dave Chinner <dchinner@redhat.com>
> >>
> >
> > Thank you for fixing this!
> > See some typo fixes below.
> > I will test the fix later today.
> >
>
> Short of one compilation warning I commented on,
> I tested these changes and found no regression with -g quick run
> I also verified that my test can be converted to use the one shot commands,
> e.g.:
>
> $XFS_IO_PROG -r foo \
> -C "open foo" \
> -C "pwrite -S 0x61 0 16" \
> -C "file 0" \
> -C "pread -v 0 16" \
> | _filter_xfs_io
>
> $XFS_IO_PROG -r bar \
> -C "mmap -r 0 16" \
> -C "open bar" \
> -C "pwrite -S 0x61 0 16" \
> -C "mread -v 0 16" \
> | _filter_xfs_io
>
> Notice that I used explicit -C for all commands including the implicit
> one shot commands.
>
> 1. Do you think that xfs_io should error on -c "open foo" to force
> explicit -C "open foo"?
No.
> it can't be breaking any existing users, because -c "open foo" is
> already very broken.
Maybe so, but there are users of it. e.g:
$ git grep open |grep XFS_IO
tests/overlay/001: $XFS_IO_PROG -c "open" $f >>$seqres.full
$
This is precisely why I made oneshot commands just work silently
with "-c" - who knows what will break if we start rejecting commands
that otherwise work just fine when there is only one open file....
> 2. You marked mmap ONE_SHOT, but not all the m* commands.
> Stands to reason that they should all be marked ONE_SHOT. because iterating
> the file table has nothing to do with the m* commands. no?
It is not clear to me what the correct thing to do here is, I don't
have the time right now to look into it, so I didn't
change mread/mwrite/msync behaviour. If it's broken before it is
still broken now.
> About the fix to overlay/016.
> How would you prefer to address the conditional availability of xfs_io -C?
>
> 1. new helper _require_xfs_io_one_shot_command
> 2. _require_xfs_io_command "open" (which internally checks for xfs_io -C "open")
> 3. third option?
I don't really care - #2 is probably neatest. If what you do is too
ugly to live then I'll let you know.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2016-12-07 20:17 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-07 3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
2016-12-07 3:47 ` [PATCH 1/6] libxcmd: check CMD_FLAG_GLOBAL inside args_command() Dave Chinner
2016-12-07 3:47 ` [PATCH 2/6] libxcmd: rename args_command to command_iterator Dave Chinner
2016-12-07 3:47 ` [PATCH 3/6] libxcmd: merge command() and iterate_command() Dave Chinner
2016-12-07 3:47 ` [PATCH 4/6] libxcmd: don't check generic library commands Dave Chinner
2016-12-07 3:47 ` [PATCH 5/6] xfs_io: make various commands one-shot only Dave Chinner
2016-12-15 18:21 ` Eric Sandeen
2016-12-16 0:53 ` Dave Chinner
2016-12-16 1:50 ` Eric Sandeen
2016-12-16 4:21 ` Dave Chinner
2016-12-07 3:47 ` [PATCH 6/6] libxcmd: add non-iterating user commands Dave Chinner
2016-12-07 4:49 ` Amir Goldstein
2016-12-07 4:57 ` Amir Goldstein
2016-12-07 14:21 ` Amir Goldstein
2016-12-07 20:16 ` Dave Chinner [this message]
2016-12-08 10:14 ` Amir Goldstein
2016-12-08 22:22 ` Dave Chinner
2016-12-15 19:09 ` Eric Sandeen
2017-01-12 5:14 ` [PATCH 0/6] xfs_io: fix up command iteration Amir Goldstein
2017-01-12 12:52 ` Eric Sandeen
-- strict thread matches above, loose matches on Subject: below --
2016-12-16 4:41 [PATCH v2 " Dave Chinner
2016-12-16 4:41 ` [PATCH 6/6] libxcmd: add non-iterating user commands Dave Chinner
2016-12-16 6:39 ` Amir Goldstein
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=20161207201637.GI4219@dastard \
--to=david@fromorbit.com \
--cc=amir73il@gmail.com \
--cc=fstests@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/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;
as well as URLs for NNTP newsgroup(s).