| Message ID | 1480943017-6175-1-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Dec 06, 2016 at 07:24:12AM +0200, Amir Goldstein wrote: > On Tue, Dec 6, 2016 at 12:15 AM, Dave Chinner <david@fromorbit.com> wrote: > > It's the command loop iteration problem we need to fix, not neuter > > the multiple file capability of xfs_io. > > > > OK, v4 it is. > For some reason you fail to spell out your idea of "fixing", > so I keep having to guess what you mean. I outlined the probable fix last week: https://www.spinics.net/lists/fstests/msg04544.html i.e. fix the one-shot commands with CMD_FLAG_GLOBAL, but indicated we'd need to look at the git history to be sure of what is necessary. > This time I am guessing that you mean: > - open all the files in args list (all using the same -r/-f flags) > - iterate all commands exactly once without implicitly changing "file" Nope. I meant "go look at the git history and determine what the historical behaviour was and determine how we ended up with this mess. Then from that analysis, decide what to do." Unfortunately, you haven't shown any indication that you've looked at the git history, nor that you actually listened to me about the CMD_FLAG_GLOBAL. I've now done that code archeology and it indicates that my suggestion was correct. If you didn't understand what I suggested, then say so clearly and unambiguously so I know I need to explain it more clearly. The /worst/ thing you can do is keep sending new patches that take the same path to one I've already NACKed without having looked at the solution I proposed or done the historical analysis I suggested was necessary. Cheers, Dave.
On Wed, Dec 7, 2016 at 1:25 AM, Dave Chinner <david@fromorbit.com> wrote: ... > > Nope. I meant "go look at the git history and determine what the > historical behaviour was and determine how we ended up with this > mess. Then from that analysis, decide what to do." > > Unfortunately, you haven't shown any indication that you've looked > at the git history, nor that you actually listened to me about the > CMD_FLAG_GLOBAL. I've now done that code archeology and it indicates > that my suggestion was correct. > Thank you for doing that digging. I might have gotten the wrong impression that you were going to do that anyway and figured you would do a better job at this than me, having a longer history with libxfs and xfs_io than me. > If you didn't understand what I suggested, then say so clearly and > unambiguously so I know I need to explain it more clearly. Sorry I wasn't clear about that. My problem, or lack of clarity, was revolving around the semantic changes that would have to be made. I did not want to go fix the problem without knowing what those are going to be, so in the mean while, I suggested a few starter options which do not change semantics for existing known users, allow overlay/016 to work properly and leave the multiple file semantics for you to clarify. > The > /worst/ thing you can do is keep sending new patches that take the > same path to one I've already NACKed without having looked at the > solution I proposed or done the historical analysis I suggested was > necessary. > Had I known /this/ was the worst thing in your book, I would have avoided it. I took that path mainly because we seem to have little overlap working hours, so the round trip of this thread makes progress hard. I apologize if this came off as "shut up Dave! and take my patch". The only reason I suggested changing behavior for multiple files and "break existing users" is because Eric indicated he had no idea about this feature and indicated that it is an undocumented feature. With someone like Eric saying that, it grew my suspicion that maybe we really do not need to fix multiple files for xfs_io?? Could you please consider this option for a moment? Perhaps we are better off correcting a wrong turn taken in 2004 then fixing its consequences now? After all, xfs_io is not the Linux kernel. Its a tool used mainly by developers as you yourself indicated. If killing multiple files will break someones use case, you should know about it pretty quick and we can then resort to changing semantics and adding new command line flags. The motivation for taking this path is not just laziness to "fix the real problem". It stands to make the code a lot simpler and to keep the man page correct and coherent with expected behavior rather then adding more obscure semantics which /are/ going to be confusing. For your consideration. Sorry if we started off on the wrong foot. Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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/io/init.c b/io/init.c index a9191cf..82b6700 100644 --- a/io/init.c +++ b/io/init.c @@ -90,14 +90,15 @@ init_commands(void) cowextsize_init(); } +/* + * Return true for first call and false for the next call, + * to execute each command once. + */ static int init_args_command( int index) { - if (index >= filecount) - return 0; - file = &filetable[index++]; - return index; + return !index; } static int
There is an undocumented and possibly unused feature in xfs_io where all commands are executed per file in the file args list. This feature creates ambiguity when trying to execute commands such as "open" and "file" from command line and result in an endless loop in the command loop code. Also, when running xfs_io -c <cmd> without any file args, xfs_io exits without doing anything. This behavior is also undocumented and makes very little sense. Change the behavior of xfs_io to execute each command specified with -c <cmd> exactly once, regardless of the number of files in the file args list. This enables writing proper xfs_io scripts in command line, which include "open" and "file" commands. This change does not modify the behavior of xfs_io in the most commonly used case of single file argument in command line and no "open" and "file" commands in command line. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- io/init.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) v2: - Fix the case of multiple file args v1: - Fix the case of zero file args