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

[Michael S. Tsirkin]

On Tue, Jan 22, 2019 at 11:21:25AM +0800, Yi Zhang wrote:
> 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

I don't see code probing for MAP_SYNC support. Did I miss it?
But if all you want is to have old linux ignore MAP_SYNC,
I think you got your wish automatically - just do not set
MAP_SHARED_VALIDATE.


> > 
> > > 
> > > > 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
> >