On Thu, Dec 08, 2016 at 12:14:24PM +0200, Amir Goldstein wrote: > On Wed, Dec 7, 2016 at 10:16 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote: > ... > >> > >> 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.... > > > > Interesting example. Here -c "open" is actually an 'alias' for > -c "stat", which could have been non one shot. > Nevertheless, I see your point. > > >> 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. > > > > Very well, but I am not sure why mmap should be marked one shot? Look at the help documentation: $ xfs_io -c "help mmap" mmap [N] | [-rwx] [-s size] [off len] -- mmap a range in the current file, show mappings maps a range within the current file into memory Example: 'mmap -rw 0 1m' - maps one megabyte from the start of the current file .... It explicitly and repeatedly says "current file" in the description of it's operation. Any typical user is going to read that and think that it means "current open file", not "map a range on all open files". > Possibly, mmap caught your filters because it is CMD_NOFILE_OK, > but in fact, the only case where mmap with no open files works is > after mmaping files and closing all open files. Well, yes. We've come across applications that do exactly this in the past, and had to simulate their behaviours. It's entirely reasonable to want to list or change the active mappings after all the open files have been closed. > Currently, this command: > $ xfs_io -c "mmap 0 4" -C "mmap" foo bar > > Results in: > [000] 0x7f17319ae000 - 0x7f17319ae004 rwx bar (0 : 4) > > IMO, it would be more useful if it resulted in: > 000 0x7f27e289d000 - 0x7f27e289d004 rwx foo (0 : 4) > [001] 0x7f27e289c000 - 0x7f27e289c004 rwx bar (0 : 4) > > Which will allow following up with more specific -C m* commands > and it would be more consistent with the result of: > $ xfs_io -C "file" foo bar > 000 foo (foreign,non-sync,non-direct,read-write) > [001] bar (foreign,non-sync,non-direct,read-write) Maybe so but, unfortunately, mapping tables are different to the file tables. As I have already said: it's not clear what the correct behaviour here is because mapping commands /don't iterate the mapping table/. The exception is the mmap command, and that mapping table iteration behaviour is the reason I changed mmap to be a oneshot command, otherwise it does stupid things when multiple files are open and they are iterated. If you want to have mmap commands do sane things for iteration, then you need to address the file vs mapping iteration problem, not hack special one-off behaviours into the mmap commands. -Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
