On Thu, Dec 21, 2017 at 10:41:10AM -0700, Ross Zwisler wrote:
> On Thu, Dec 21, 2017 at 09:09:08AM -0800, Darrick J. Wong wrote:
> > On Tue, Dec 05, 2017 at 04:56:50PM -0700, Ross Zwisler wrote:
>
> > > @@ -195,6 +200,13 @@ mmap_f(
> > > case 'x':
> > > prot |= PROT_EXEC;
> > > break;
> > > + case 'S':
> > > + flags = MAP_SYNC | MAP_SHARED_VALIDATE;
> > > + if (!flags) {
> >
> > Heh, subtle. :)
> >
> > /me wonders if it'd be better to do this explicitly:
> >
> > #ifdef HAVE_MAP_SYNC
> > flags = MAP_SYNC | MAP_SHARED_VALIDATE;
> > #else
> > printf("MAP_SYNC not supported\n");
> > return 1;
> > #endif
> >
> > ...though it's ugly.
>
> Yea, I was trying to avoid #ifdefery. If you prefer this, though, I'm happy
> to change it. Or maybe a comment would be enough?
>
> /*
> * If MAP_SYNC and MAP_SHARED_VALIDATE aren't defined in the system headers we
> * will have defined them both as 0.
> */
>
> ?
Works for me.
>
> > > + printf("MAP_SYNC not supported\n");
> > > + return 0;
> >
> > Are we supposed to be returning nonzero values for failing commands?
>
> I don't think so. All the other error conditions in this function also return
> 0. I think the important thing is that 'exitcode' is set to 1 at the
> beginning of the function per Dave's patch,
>
> https://www.spinics.net/lists/linux-xfs/msg13605.html
>
> which I pulled into my baseline:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/zwisler/xfsprogs-dev.git/refs/?h=map_sync_v3
>
> So, I likewise leave 'exitcode' as 1, bail out with a return code of 0, and
> then you get the overall failure return of 1 from xfs_io at the shell.
Ok.
> > > @@ -764,7 +764,7 @@ Each (sec, nsec) pair constitutes a single timestamp value.
> > >
> > > .SH MEMORY MAPPED I/O COMMANDS
> > > .TP
> > > -.BI "mmap [ " N " | [[ \-rwx ] [\-s " size " ] " "offset length " ]]
> > > +.BI "mmap [ " N " | [[ \-rwxS ] [\-s " size " ] " "offset length " ]]
> > > With no arguments,
> > > .B mmap
> > > shows the current mappings. Specifying a single numeric argument
> > > @@ -780,6 +780,10 @@ PROT_WRITE
> > > .RB ( \-w ),
> > > and PROT_EXEC
> > > .RB ( \-x ).
> > > +The mapping will be created with the MAP_SHARED flag by default, or with the
> > > +Linux specific (MAP_SYNC | MAP_SHARED_VALIDATE) flags if
> >
> > I assume I'll be able to look up exactly what MAP_SYNC provides in the mmap
> > manpage, right?
>
> Yep, Jan updated the man page for both MAP_SYNC and MAP_SHARED_VALIDATE:
> https://lists.01.org/pipermail/linux-nvdimm/2017-November/013158.html
Will have a look.
> Thank you for the review.
--D
> --
> 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
--
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
