Re: [Qemu-devel] [PATCH V9 4/6] util/mmap-alloc: support MAP_SYNC in qemu_ram_mmap()

[Yi Zhang <yi.z.zhang@linux.intel.com>]

On 2019-01-21 at 12:44:00 -0200, Eduardo Habkost wrote:
> On Mon, Jan 21, 2019 at 01:15:36PM +0800, Yi Zhang wrote:
> > On 2019-01-18 at 16:11:47 -0200, Eduardo Habkost wrote:
> [...]
> > > Anyway, I see a more fundamental problem in each version of this
> > > patch: the semantics of the command-line options are not clearly
> > > documented.
> > > 
> > > We have at least 3 different possible use cases we might need to
> > > support:
> > > 
> > > 1) pmem=on, MAP_SYNC not desired
> > > 2) pmem=on, MAP_SYNC desired but optional
> > 
> > Form V9, As Michael suggest, We removed the sync option, MAP_SYNC will
> > force on while we set pmem=on. So we only have 2 user cases, Will update
> > to user documentation.
> > 1) pmem=on, MAP_SYNC not desired
> > We will not pass the flag to mmap2
> 
> If this use case is supported, how the command-line should look
> like to enable it?
> 
> 
> > 2) pmem=on, MAP_SYNC desired
> > We will pass the flag to mmap2
> 
> Same question as above: how the command-line should look like for
> this use case?
Sorry, I got some miss-understood of the MAP_SYNC desired.
As we talk with Micheal:

we give up on a bit of flexibility, and just say
pmem=on forces MAP_SYNC. on a MAP_SYNC capable configrations(kernel+
backend dax)

Current user case is like below:

 1. pmem=on is set, shared=on is set, MAP_SYNC supported in linux kernel:
        a: backend is a dax supporting file.
	 - MAP_SYNC will active.
	b: backedn is not a dax supporting file.
	 - mmap will result in an EOPNOTSUPP error.

2. The reset of cases:
	- we will never pass the MAP_SYNC to mmap2

> 
> > 
> > > 3) pmem=on, MAP_SYNC required, not optional
> > > 
> > > Which cases from the list above we need to support?
> > > 
> > > From the cases above, what's the expected semantics of "pmem=on"
> > > with no extra options?
> 
> We still need to answer that question.
> 
> The current semantics of pmem=on (with no extra options) is (1).
> It looks like we can't change it to (2) without breaking existing
> configurations.  If you make existing configurations stop working
> on hosts where they currently work, you need to explain why it's
> OK to do that.
> 
> 
> > > 
> > > If these questions are not answered (in the commit message and
> > > user documentation), we won't be able to review and discuss the
> > > code.
> > > 
> > > 
> [...]
> 
> 
> -- 
> Eduardo
>