Re: [PATCH] dax, pmem: add support for msync

[Boaz Harrosh]

On 09/01/2015 10:06 AM, Christoph Hellwig wrote:
> On Tue, Sep 01, 2015 at 09:38:03AM +1000, Dave Chinner wrote:
>> On Mon, Aug 31, 2015 at 12:59:44PM -0600, Ross Zwisler wrote:
>>> For DAX msync we just need to flush the given range using
>>> wb_cache_pmem(), which is now a public part of the PMEM API.
>>
>> This is wrong, because it still leaves fsync() broken on dax.
>>
>> Flushing dirty data to stable storage is the responsibility of the
>> writeback infrastructure, not the VMA/mm infrasrtucture. For non-dax
>> configurations, msync defers all that to vfs_fsync_range(), because
>> it has to be implemented there for fsync() to work.
>>
>> Even for DAX, msync has to call vfs_fsync_range() for the filesystem to commit
>> the backing store allocations to stable storage, so there's not
>> getting around the fact msync is the wrong place to be flushing
>> DAX mappings to persistent storage.
> 
> DAX does call ->fsync before and after this patch.  And with all
> the recent fixes we take care to ensure data is written though the
> cache for everything but mmap-access.  With this patch from Ross
> we ensure msync writes back the cache before calling ->fsync so that
> the filesystem can then do it's work like converting unwritten extents.
> 
> The only downside is that previously on Linux you could always use
> fsync as a replaement for msymc, which isn't true anymore for DAX.
> 

Hi Christoph

So the approach we took was a bit different to exactly solve these
problem, and to also not over flush too much. here is what we did.

* At vm_operations_struct we also override the .close vector (say call it dax_vm_close)

* At dax_vm_close() on writable files call ->fsync(,vma->vm_start, vma->vm_end,)
  (We have an inode flag if the file was actually dirtied, but even if not, that will
   not be that bad, so a file was opened for write, mmapped, but actually never
   modified. Not a lot of these, and the do nothing cl_flushing is very fast)

* At ->fsync() do the actual cl_flush for all cases but only iff
	if (mapping_mapped(inode->i_mapping) == 0)
		return 0;

  This is because data written not through mmap is already persistent and we
  do not need the cl_flushing

Apps expect all these to work:
1. open mmap m-write msync ... close
2. open mmap m-write fsync ... close
3. open mmap m-write unmap ... fsync close

4. open mmap m-write sync ...

The first 3 are supported with above, because what happens is that at [3]
the fsync actually happens on unmap and fsync is redundant in that case.

The only broken scenario is [3]. We do not have a list of "dax-dirty" inodes
per sb to iterate on and call inode-sync on. This cause problems mostly in
freeze because with actual [3] scenario the file will be eventually closed
and persistent, but after the call to sync returns.

Its on my TODO to fix [3] based on instructions from Dave.
The mmap call will put the inode on the list and the dax_vm_close will
remove it. One of the regular dirty list should be used as suggested by
Dave.

> But given that we need the virtual address to write back the cache
> I can't see how to do this differently given that clwb() needs the
> user virtual address to flush the cache.

On Intel or any systems that have physical-based caching this is not
a problem you just iterate on all get_block() of the range and flush
the Kernel's virt_addr of the block, this is easy.

With ARCHs with per VM caching you need to go through the i_mapping VMAs list
and flush like that. I guess there is a way to schedule yourself as a process VMA
somehow.
I'm not sure how to solve this split, perhaps two generic functions, that
are selected through the ARCH.

Just my $0.017
Boaz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/