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