Re: [xfsprogs PATCH v2 2/3] xfs_io: add MAP_SYNC support to mmap()

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html