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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.