Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1216381 > unrolled thread

[PATCH] dax, pmem: add support for msync

Back to article view | Back to linux.kernel

Contents

  [PATCH] dax, pmem: add support for msync Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-08-31 21:10 +0200
    Re: [PATCH] dax, pmem: add support for msync Christoph Hellwig <hch@lst.de> - 2015-08-31 21:10 +0200
      Re: [PATCH] dax, pmem: add support for msync Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-08-31 21:30 +0200
        Re: [PATCH] dax, pmem: add support for msync Christoph Hellwig <hch@lst.de> - 2015-08-31 21:40 +0200
    Re: [PATCH] dax, pmem: add support for msync Dave Chinner <david@fromorbit.com> - 2015-09-01 01:40 +0200
      Re: [PATCH] dax, pmem: add support for msync Christoph Hellwig <hch@lst.de> - 2015-09-01 09:10 +0200
        Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-01 14:20 +0200
          Re: [PATCH] dax, pmem: add support for msync Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-09-02 21:10 +0200
            Re: [PATCH] dax, pmem: add support for msync "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-02 22:20 +0200
            Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-03 08:40 +0200
              Re: [PATCH] dax, pmem: add support for msync Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-09-03 18:50 +0200
        Re: [PATCH] dax, pmem: add support for msync Dave Chinner <david@fromorbit.com> - 2015-09-02 00:30 +0200
          Re: [PATCH] dax, pmem: add support for msync Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-09-02 05:20 +0200
            Re: [PATCH] dax, pmem: add support for msync Dave Chinner <david@fromorbit.com> - 2015-09-02 07:20 +0200
              Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 12:30 +0200
                Re: [PATCH] dax, pmem: add support for msync Dave Hansen <dave.hansen@linux.intel.com> - 2015-09-02 16:30 +0200
                  Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 17:20 +0200
                    Re: [PATCH] dax, pmem: add support for msync Dave Hansen <dave.hansen@linux.intel.com> - 2015-09-02 17:50 +0200
                      Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 18:10 +0200
                        Re: [PATCH] dax, pmem: add support for msync Dave Hansen <dave.hansen@linux.intel.com> - 2015-09-02 18:20 +0200
                          Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-03 08:50 +0200
            Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 12:10 +0200
      Re: [PATCH] dax, pmem: add support for msync "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-01 12:10 +0200
        Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-01 13:30 +0200
        Re: [PATCH] dax, pmem: add support for msync Dave Chinner <david@fromorbit.com> - 2015-09-02 00:50 +0200
          Re: [PATCH] dax, pmem: add support for msync "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-02 11:20 +0200
            Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 11:40 +0200
              Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 11:50 +0200
                Re: [PATCH] dax, pmem: add support for msync "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-02 11:50 +0200
                  Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-02 12:30 +0200
            Re: [PATCH] dax, pmem: add support for msync Dave Chinner <david@fromorbit.com> - 2015-09-03 03:00 +0200
    Re: [PATCH] dax, pmem: add support for msync Boaz Harrosh <boaz@plexistor.com> - 2015-09-01 15:20 +0200
      Re: [PATCH] dax, pmem: add support for msync Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-09-02 19:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1218065

On 09/02/2015 07:19 PM, Dave Hansen wrote:
> On 09/02/2015 09:00 AM, Boaz Harrosh wrote:
>>>> We are going to have 2-socket systems with 6TB of persistent memory in
>>>> them.  I think it's important to design this mechanism so that it scales
>>>> to memory sizes like that and supports large mmap()s.
>>>>
>>>> I'm not sure the application you've seen thus far are very
>>>> representative of what we want to design for.
>>>>
>> We have a patch pending to introduce a new mmap flag that pmem aware
>> applications can set to eliminate any kind of flushing. MMAP_PMEM_AWARE.
> 
> Great!  Do you have a link so that I can review it and compare it to
> Ross's approach?
> 

Ha? I have not seen a new mmap flag from Ross, yet I have been off lately
so it is logical that I might have missed it.

Could you send me a link?

(BTW my patch I did not release yet, I'll cc you once its done)

Thanks
Boaz

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

[toc] | [prev] | [next] | [standalone]

#1217473

On 09/02/2015 06:19 AM, Ross Zwisler wrote:
> On Wed, Sep 02, 2015 at 08:21:20AM +1000, Dave Chinner wrote:
>> Which means applications that should "just work" without
>> modification on DAX are now subtly broken and don't actually
>> guarantee data is safe after a crash. That's a pretty nasty
>> landmine, and goes against *everything* we've claimed about using
>> DAX with existing applications.
>>
>> That's wrong, and needs fixing.
> 
> I agree that we need to fix fsync as well, and that the fsync solution could
> be used to implement msync if we choose to go that route.  I think we might
> want to consider keeping the msync and fsync implementations separate, though,
> for two reasons.
> 
> 1) The current msync implementation is much more efficient than what will be
> needed for fsync.  Fsync will need to call into the filesystem, traverse all
> the blocks, get kernel virtual addresses from those and then call
> wb_cache_pmem() on those kernel addresses.  

I was thinking about this some more, and no this is not what we need to do
because of the virtual-based-cache ARCHs. And what we do for these systems
will also work for physical-based-cache ARCHs.

What we need to do, is dig into the mapping structure and pic up the current
VMA on the call to fsync. Then just flush that one on that virtual address,
(since it is current at the context of the fsync sys call)

And of course we need to do like I wrote, we must call fsync on vm_operations->close
before the VMA mappings goes away. Then an fsync after unmap is a no-op.

> I think this is a necessary evil
> for fsync since you don't have a VMA, but for msync we do and we can just
> flush using the user addresses without any fs lookups.
> 

right see above

> 2) I believe that the near-term fsync code will rely on struct pages for
> PMEM, which I believe are possible but optional as of Dan's last patch set:
> 
> https://lkml.org/lkml/2015/8/25/841
> 
> I believe that this means that if we don't have struct pages for PMEM (becuase
> ZONE_DEVICE et al. are turned off) fsync won't work.  I'd be nice not to lose
> msync as well.

Please see above it can be made to work. Actually what we do is the
traversal-kernel-ptr thing, and the fsync-on-unmap. And it works we have heavy
persistence testing and it is all very good.

So no, without pages it can all work very-well. There is only the sync problem
that I intend to fix soon, is only a matter of keeping a dax-dirty inode-list
per sb.

So no this is not an excuse.

Cheers
Boaz

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

[toc] | [prev] | [next] | [standalone]

#1216747

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.

Writeback infrastructure is non-existent for DAX. Without struct page we
don't have anything to transfer pte_ditry() to. And I'm not sure we need
to invent some. For DAX flushing in-place can be cheaper than dirty
tracking beyond page tables.

> For non-dax configurations, msync defers all that to vfs_fsync_range(),
> because it has to be implemented there for fsync() to work.

Not necessary. I think fsync() for DAX can be implemented with rmap over
all file's VMA and msync() them with commiting metadata afterwards.

But we also need to commit to persistent on zap_page_range() to make it
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.

Why?
IIUC, msync() doesn't have any requirements wrt metadata, right?

> I pointed this out almost 6 months ago (i.e. that fsync was broken)
> anf hinted at how to solve it. Fix fsync, and msync gets fixed for
> free:
> 
> https://lists.01.org/pipermail/linux-nvdimm/2015-March/000341.html
> 
> I've also reported to Willy that DAX write page faults don't work
> correctly, either. xfstests generic/080 exposes this: a read
> from a page followed immediately by a write to that page does not
> result in ->page_mkwrite being called on the write and so
> backing store is not allocated for the page, nor are the timestamps
> for the file updated. This will also result in fsync (and msync)
> not working properly.

Is that because XFS doesn't provide vm_ops->pfn_mkwrite?

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

[toc] | [prev] | [next] | [standalone]

#1216779

On 09/01/2015 01:08 PM, Kirill A. Shutemov wrote:
<>
> 
> Is that because XFS doesn't provide vm_ops->pfn_mkwrite?
> 

Right that would explain it, because I sent that patch exactly to solve
this problem. I haven't looked at latest code for a while but I should
checkout the latest and make a patch for xfs if it is indeed missing.

Thanks
Boaz

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

[toc] | [prev] | [next] | [standalone]

#1217149

On Tue, Sep 01, 2015 at 01:08:04PM +0300, Kirill A. Shutemov 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:
> > 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.
> 
> Why?
> IIUC, msync() doesn't have any requirements wrt metadata, right?

Of course it does. If the backing store allocation has not been
committed, then after a crash there will be a hole in file and
so it will read as zeroes regardless of what data was written and
flushed.

> > I pointed this out almost 6 months ago (i.e. that fsync was broken)
> > anf hinted at how to solve it. Fix fsync, and msync gets fixed for
> > free:
> > 
> > https://lists.01.org/pipermail/linux-nvdimm/2015-March/000341.html
> > 
> > I've also reported to Willy that DAX write page faults don't work
> > correctly, either. xfstests generic/080 exposes this: a read
> > from a page followed immediately by a write to that page does not
> > result in ->page_mkwrite being called on the write and so
> > backing store is not allocated for the page, nor are the timestamps
> > for the file updated. This will also result in fsync (and msync)
> > not working properly.
> 
> Is that because XFS doesn't provide vm_ops->pfn_mkwrite?

I didn't know that had been committed. I don't recall seeing a pull
request with that in it, none of the XFS DAX patches conflicted
against it and there's been no runtime errors. I'll fix it up.

As such, shouldn't there be a check in the VM (in ->mmap callers)
that if we have the vma is returned with VM_MIXEDMODE enabled that
->pfn_mkwrite is not NULL?  It's now clear to me that any filesystem
that sets VM_MIXEDMODE needs to support both page_mkwrite and
pfn_mkwrite, and such a check would have caught this immediately...

Cheers,

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

[toc] | [prev] | [next] | [standalone]

#1217441

On Wed, Sep 02, 2015 at 08:49:22AM +1000, Dave Chinner wrote:
> On Tue, Sep 01, 2015 at 01:08:04PM +0300, Kirill A. Shutemov 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:
> > > 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.
> > 
> > Why?
> > IIUC, msync() doesn't have any requirements wrt metadata, right?
> 
> Of course it does. If the backing store allocation has not been
> committed, then after a crash there will be a hole in file and
> so it will read as zeroes regardless of what data was written and
> flushed.

Any reason why backing store allocation cannot be committed on *_mkwrite?

> > > I pointed this out almost 6 months ago (i.e. that fsync was broken)
> > > anf hinted at how to solve it. Fix fsync, and msync gets fixed for
> > > free:
> > > 
> > > https://lists.01.org/pipermail/linux-nvdimm/2015-March/000341.html
> > > 
> > > I've also reported to Willy that DAX write page faults don't work
> > > correctly, either. xfstests generic/080 exposes this: a read
> > > from a page followed immediately by a write to that page does not
> > > result in ->page_mkwrite being called on the write and so
> > > backing store is not allocated for the page, nor are the timestamps
> > > for the file updated. This will also result in fsync (and msync)
> > > not working properly.
> > 
> > Is that because XFS doesn't provide vm_ops->pfn_mkwrite?
> 
> I didn't know that had been committed. I don't recall seeing a pull
> request with that in it

It went though -mm tree.

> none of the XFS DAX patches conflicted
> against it and there's been no runtime errors. I'll fix it up.
> 
> As such, shouldn't there be a check in the VM (in ->mmap callers)
> that if we have the vma is returned with VM_MIXEDMODE enabled that
> ->pfn_mkwrite is not NULL?  It's now clear to me that any filesystem
> that sets VM_MIXEDMODE needs to support both page_mkwrite and
> pfn_mkwrite, and such a check would have caught this immediately...

I guess it's "both or none" case. We have VM_MIXEDMAP users who don't care
about *_mkwrite.

I'm not yet sure it would be always correct, but something like this will
catch the XFS case, without false-positive on other stuff in my KVM setup:

diff --git a/mm/mmap.c b/mm/mmap.c
index 3f78bceefe5a..f2e29a541e14 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1645,6 +1645,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
                        vma->vm_ops = &dummy_ops;
                }
 
+               /*
+                * Make sure that for VM_MIXEDMAP VMA has both
+                * vm_ops->page_mkwrite and vm_ops->pfn_mkwrite or has none.
+                */
+               if ((vma->vm_ops->page_mkwrite || vma->vm_ops->pfn_mkwrite) &&
+                               vma->vm_flags & VM_MIXEDMAP) {
+                       VM_BUG_ON_VMA(!vma->vm_ops->page_mkwrite, vma);
+                       VM_BUG_ON_VMA(!vma->vm_ops->pfn_mkwrite, vma);
+               }
                addr = vma->vm_start;
                vm_flags = vma->vm_flags;
        } else if (vm_flags & VM_SHARED) {
-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217455

On 09/02/2015 12:13 PM, Kirill A. Shutemov wrote:
> On Wed, Sep 02, 2015 at 08:49:22AM +1000, Dave Chinner wrote:
>> On Tue, Sep 01, 2015 at 01:08:04PM +0300, Kirill A. Shutemov 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:
>>>> 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.
>>>
>>> Why?
>>> IIUC, msync() doesn't have any requirements wrt metadata, right?
>>
>> Of course it does. If the backing store allocation has not been
>> committed, then after a crash there will be a hole in file and
>> so it will read as zeroes regardless of what data was written and
>> flushed.
> 
> Any reason why backing store allocation cannot be committed on *_mkwrite?
> 
>>>> I pointed this out almost 6 months ago (i.e. that fsync was broken)
>>>> anf hinted at how to solve it. Fix fsync, and msync gets fixed for
>>>> free:
>>>>
>>>> https://lists.01.org/pipermail/linux-nvdimm/2015-March/000341.html
>>>>
>>>> I've also reported to Willy that DAX write page faults don't work
>>>> correctly, either. xfstests generic/080 exposes this: a read
>>>> from a page followed immediately by a write to that page does not
>>>> result in ->page_mkwrite being called on the write and so
>>>> backing store is not allocated for the page, nor are the timestamps
>>>> for the file updated. This will also result in fsync (and msync)
>>>> not working properly.
>>>
>>> Is that because XFS doesn't provide vm_ops->pfn_mkwrite?
>>
>> I didn't know that had been committed. I don't recall seeing a pull
>> request with that in it
> 
> It went though -mm tree.
> 
>> none of the XFS DAX patches conflicted
>> against it and there's been no runtime errors. I'll fix it up.
>>
>> As such, shouldn't there be a check in the VM (in ->mmap callers)
>> that if we have the vma is returned with VM_MIXEDMODE enabled that
>> ->pfn_mkwrite is not NULL?  It's now clear to me that any filesystem
>> that sets VM_MIXEDMODE needs to support both page_mkwrite and
>> pfn_mkwrite, and such a check would have caught this immediately...
> 
> I guess it's "both or none" case. We have VM_MIXEDMAP users who don't care
> about *_mkwrite.
> 
> I'm not yet sure it would be always correct, but something like this will
> catch the XFS case, without false-positive on other stuff in my KVM setup:
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 3f78bceefe5a..f2e29a541e14 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1645,6 +1645,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>                         vma->vm_ops = &dummy_ops;
>                 }
>  
> +               /*
> +                * Make sure that for VM_MIXEDMAP VMA has both
> +                * vm_ops->page_mkwrite and vm_ops->pfn_mkwrite or has none.
> +                */
> +               if ((vma->vm_ops->page_mkwrite || vma->vm_ops->pfn_mkwrite) &&
> +                               vma->vm_flags & VM_MIXEDMAP) {
> +                       VM_BUG_ON_VMA(!vma->vm_ops->page_mkwrite, vma);
> +                       VM_BUG_ON_VMA(!vma->vm_ops->pfn_mkwrite, vma);

BTW: the page_mkwrite is used for reading of holes that put zero-pages at the radix tree.
     One can just map a single global zero-page in pfn-mode for that.

Kirill Hi. Please don't make these BUG_ONs its counter productive believe me.
Please make them WARN_ON_ONCE() it is not a crashing bug to work like this.
(Actually it is not a bug at all in some cases, but we can relax that when a user
 comes up)

Thanks
Boaz

> +               }
>                 addr = vma->vm_start;
>                 vm_flags = vma->vm_flags;
>         } else if (vm_flags & VM_SHARED) {
> 

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

[toc] | [prev] | [next] | [standalone]

#1217456

On 09/02/2015 12:37 PM, Boaz Harrosh wrote:
>>  
>> +               /*
>> +                * Make sure that for VM_MIXEDMAP VMA has both
>> +                * vm_ops->page_mkwrite and vm_ops->pfn_mkwrite or has none.
>> +                */
>> +               if ((vma->vm_ops->page_mkwrite || vma->vm_ops->pfn_mkwrite) &&
>> +                               vma->vm_flags & VM_MIXEDMAP) {
>> +                       VM_BUG_ON_VMA(!vma->vm_ops->page_mkwrite, vma);
>> +                       VM_BUG_ON_VMA(!vma->vm_ops->pfn_mkwrite, vma);
> 
> BTW: the page_mkwrite is used for reading of holes that put zero-pages at the radix tree.
>      One can just map a single global zero-page in pfn-mode for that.
> 
> Kirill Hi. Please don't make these BUG_ONs its counter productive believe me.
> Please make them WARN_ON_ONCE() it is not a crashing bug to work like this.
> (Actually it is not a bug at all in some cases, but we can relax that when a user
>  comes up)
> 
> Thanks
> Boaz
> 

Second thought I do not like this patch. This is why we have xftests for, the fact of it
is that test 080 catches this. For me this is enough.

An FS developer should test his code, and worst case we help him on ML, like we did
in this case.

Thanks
Boaz

>> +               }
>>                 addr = vma->vm_start;
>>                 vm_flags = vma->vm_flags;
>>         } else if (vm_flags & VM_SHARED) {
>>
> 

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

[toc] | [prev] | [next] | [standalone]

#1217458

On Wed, Sep 02, 2015 at 12:41:44PM +0300, Boaz Harrosh wrote:
> On 09/02/2015 12:37 PM, Boaz Harrosh wrote:
> >>  
> >> +               /*
> >> +                * Make sure that for VM_MIXEDMAP VMA has both
> >> +                * vm_ops->page_mkwrite and vm_ops->pfn_mkwrite or has none.
> >> +                */
> >> +               if ((vma->vm_ops->page_mkwrite || vma->vm_ops->pfn_mkwrite) &&
> >> +                               vma->vm_flags & VM_MIXEDMAP) {
> >> +                       VM_BUG_ON_VMA(!vma->vm_ops->page_mkwrite, vma);
> >> +                       VM_BUG_ON_VMA(!vma->vm_ops->pfn_mkwrite, vma);
> > 
> > BTW: the page_mkwrite is used for reading of holes that put zero-pages at the radix tree.
> >      One can just map a single global zero-page in pfn-mode for that.
> > 
> > Kirill Hi. Please don't make these BUG_ONs its counter productive believe me.

This is VM_BUG_ON, not normal BUG_ON. VM_BUG_ON is under CONFIG_DEBUG_VM 
which is disabled on production kernels.

> > Please make them WARN_ON_ONCE() it is not a crashing bug to work like this.
> > (Actually it is not a bug at all in some cases, but we can relax that when a user
> >  comes up)
> > 
> > Thanks
> > Boaz
> > 
> 
> Second thought I do not like this patch. This is why we have xftests for, the fact of it
> is that test 080 catches this. For me this is enough.

I don't insist on applying the patch. And I worry about false-positives.

> An FS developer should test his code, and worst case we help him on ML, like we did
> in this case.
> 
> Thanks
> Boaz
> 
> >> +               }
> >>                 addr = vma->vm_start;
> >>                 vm_flags = vma->vm_flags;
> >>         } else if (vm_flags & VM_SHARED) {
> >>
> > 
> 

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

[toc] | [prev] | [next] | [standalone]

#1217503

On 09/02/2015 12:47 PM, Kirill A. Shutemov wrote:
<>
> 
> I don't insist on applying the patch. And I worry about false-positives.
> 

Thanks, yes
Boaz

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

[toc] | [prev] | [next] | [standalone]

#1217975

On Wed, Sep 02, 2015 at 12:13:21PM +0300, Kirill A. Shutemov wrote:
> On Wed, Sep 02, 2015 at 08:49:22AM +1000, Dave Chinner wrote:
> > On Tue, Sep 01, 2015 at 01:08:04PM +0300, Kirill A. Shutemov 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:
> > > > 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.
> > > 
> > > Why?
> > > IIUC, msync() doesn't have any requirements wrt metadata, right?
> > 
> > Of course it does. If the backing store allocation has not been
> > committed, then after a crash there will be a hole in file and
> > so it will read as zeroes regardless of what data was written and
> > flushed.
> 
> Any reason why backing store allocation cannot be committed on *_mkwrite?

Oh, I could change that if you want, it'll just be ridiculously
slow because it requires journal flushes on every page fault that
needs to change the filesytsem block map (i.e. every allocation and/or
every unwritten extent conversion).

Sycnhronous journalling requires flushing the log on every
transaction commit. That involves switching to a work queue, copying
the changes into a log buffer, issuing IO to flush the journal,
waiting for that to complete, etc. i.e.  synchronous journalling
incurs a minimum overhead of 4 context switches per page fault that
needs to allocate/convert backing store, along with all the CPU time
needed to process the journal commit.

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 3f78bceefe5a..f2e29a541e14 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1645,6 +1645,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>                         vma->vm_ops = &dummy_ops;
>                 }
>  
> +               /*
> +                * Make sure that for VM_MIXEDMAP VMA has both
> +                * vm_ops->page_mkwrite and vm_ops->pfn_mkwrite or has none.
> +                */
> +               if ((vma->vm_ops->page_mkwrite || vma->vm_ops->pfn_mkwrite) &&
> +                               vma->vm_flags & VM_MIXEDMAP) {
> +                       VM_BUG_ON_VMA(!vma->vm_ops->page_mkwrite, vma);
> +                       VM_BUG_ON_VMA(!vma->vm_ops->pfn_mkwrite, vma);
> +               }

Doesn't really help developers that don't use CONFIG_DEBUG_VM. i.e
it's the FS developers that you need to warn, not VM developers -
in this case a "WARN_ON_ONCE" is probably more appropriate.

Cheers,

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

[toc] | [prev] | [next] | [standalone]

#1216840

On 08/31/2015 09:59 PM, 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.
> 
> The inclusion of <linux/dax.h> in fs/dax.c was done to make checkpatch
> happy.  Previously it was complaining about a bunch of undeclared
> functions that could be made static.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
> This patch is based on libnvdimm-for-next from our NVDIMM tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/nvdimm/nvdimm.git/
> 
> with some DAX patches on top.  The baseline tree can be found here:
> 
> https://github.com/01org/prd/tree/dax_msync
> ---
>  arch/x86/include/asm/pmem.h | 13 +++++++------
>  fs/dax.c                    | 17 +++++++++++++++++
>  include/linux/dax.h         |  1 +
>  include/linux/pmem.h        | 22 +++++++++++++++++++++-
>  mm/msync.c                  | 10 +++++++++-
>  5 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
> index d8ce3ec..85c07b2 100644
> --- a/arch/x86/include/asm/pmem.h
> +++ b/arch/x86/include/asm/pmem.h
> @@ -67,18 +67,19 @@ static inline void arch_wmb_pmem(void)
>  }
>  
>  /**
> - * __arch_wb_cache_pmem - write back a cache range with CLWB
> - * @vaddr:	virtual start address
> + * arch_wb_cache_pmem - write back a cache range with CLWB
> + * @addr:	virtual start address
>   * @size:	number of bytes to write back
>   *
>   * Write back a cache range using the CLWB (cache line write back)
>   * instruction.  This function requires explicit ordering with an
> - * arch_wmb_pmem() call.  This API is internal to the x86 PMEM implementation.
> + * arch_wmb_pmem() call.
>   */
> -static inline void __arch_wb_cache_pmem(void *vaddr, size_t size)
> +static inline void arch_wb_cache_pmem(void __pmem *addr, size_t size)
>  {
>  	u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
>  	unsigned long clflush_mask = x86_clflush_size - 1;
> +	void *vaddr = (void __force *)addr;
>  	void *vend = vaddr + size;
>  	void *p;
>  
> @@ -115,7 +116,7 @@ static inline size_t arch_copy_from_iter_pmem(void __pmem *addr, size_t bytes,
>  	len = copy_from_iter_nocache(vaddr, bytes, i);
>  
>  	if (__iter_needs_pmem_wb(i))
> -		__arch_wb_cache_pmem(vaddr, bytes);
> +		arch_wb_cache_pmem(addr, bytes);
>  
>  	return len;
>  }
> @@ -138,7 +139,7 @@ static inline void arch_clear_pmem(void __pmem *addr, size_t size)
>  	else
>  		memset(vaddr, 0, size);
>  
> -	__arch_wb_cache_pmem(vaddr, size);
> +	arch_wb_cache_pmem(addr, size);
>  }
>  
>  static inline bool __arch_has_wmb_pmem(void)
> diff --git a/fs/dax.c b/fs/dax.c
> index fbe18b8..ed6aec1 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -17,6 +17,7 @@
>  #include <linux/atomic.h>
>  #include <linux/blkdev.h>
>  #include <linux/buffer_head.h>
> +#include <linux/dax.h>
>  #include <linux/fs.h>
>  #include <linux/genhd.h>
>  #include <linux/highmem.h>
> @@ -25,6 +26,7 @@
>  #include <linux/mutex.h>
>  #include <linux/pmem.h>
>  #include <linux/sched.h>
> +#include <linux/sizes.h>
>  #include <linux/uio.h>
>  #include <linux/vmstat.h>
>  
> @@ -753,3 +755,18 @@ int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block)
>  	return dax_zero_page_range(inode, from, length, get_block);
>  }
>  EXPORT_SYMBOL_GPL(dax_truncate_page);
> +
> +void dax_sync_range(unsigned long addr, size_t len)
> +{
> +	while (len) {
> +		size_t chunk_len = min_t(size_t, SZ_1G, len);
> +

Where does the  SZ_1G come from is it because you want to do cond_resched()
every 1G bytes so not to get stuck for a long time?

It took me a while to catch, At first I thought it might be do to wb_cache_pmem()
limitations. Would you put a comment in the next iteration?

> +		wb_cache_pmem((void __pmem *)addr, chunk_len);
> +		len -= chunk_len;
> +		addr += chunk_len;
> +		if (len)
> +			cond_resched();
> +	}
> +	wmb_pmem();
> +}
> +EXPORT_SYMBOL_GPL(dax_sync_range);
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index b415e52..504b33f 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -14,6 +14,7 @@ int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t,
>  		dax_iodone_t);
>  int __dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t,
>  		dax_iodone_t);
> +void dax_sync_range(unsigned long addr, size_t len);
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  int dax_pmd_fault(struct vm_area_struct *, unsigned long addr, pmd_t *,
>  				unsigned int flags, get_block_t, dax_iodone_t);
> diff --git a/include/linux/pmem.h b/include/linux/pmem.h
> index 85f810b3..aa29ebb 100644
> --- a/include/linux/pmem.h
> +++ b/include/linux/pmem.h
> @@ -53,12 +53,18 @@ static inline void arch_clear_pmem(void __pmem *addr, size_t size)
>  {
>  	BUG();

See below

>  }
> +
> +static inline void arch_wb_cache_pmem(void __pmem *addr, size_t size)
> +{
> +	BUG();

There is a clflush_cache_range() defined for generic use. On ADR systems (even without pcommit)
this works perfectly and is persistent. why not use that in the generic case?

Also all the above and below can be implements via this one.

One usage of pmem is overlooked by all this API. The use of DRAM as pmem, across a VM
or cross reboot. you have a piece of memory exposed as pmem to the subsytem which survives
past the boot of that system. The CPU cache still needs flushing in this case.
(People are already using this for logs and crash dumps)

So all arches including all x86 variants can have a working generic implementation
that will work, based on clflush_cache_range()

I'll work on this ASAP and send patches ...

> +}
>  #endif
>  
>  /*
>   * Architectures that define ARCH_HAS_PMEM_API must provide
>   * implementations for arch_memcpy_to_pmem(), arch_wmb_pmem(),
> - * arch_copy_from_iter_pmem(), arch_clear_pmem() and arch_has_wmb_pmem().
> + * arch_copy_from_iter_pmem(), arch_clear_pmem(), arch_wb_cache_pmem()
> + * and arch_has_wmb_pmem().
>   */
>  static inline void memcpy_from_pmem(void *dst, void __pmem const *src, size_t size)
>  {
> @@ -202,4 +208,18 @@ static inline void clear_pmem(void __pmem *addr, size_t size)
>  	else
>  		default_clear_pmem(addr, size);
>  }
> +
> +/**
> + * wb_cache_pmem - write back a range of cache lines
> + * @vaddr:	virtual start address
> + * @size:	number of bytes to write back
> + *
> + * Write back the cache lines starting at 'vaddr' for 'size' bytes.
> + * This function requires explicit ordering with an wmb_pmem() call.
> + */
> +static inline void wb_cache_pmem(void __pmem *addr, size_t size)
> +{
> +	if (arch_has_pmem_api())
> +		arch_wb_cache_pmem(addr, size);
> +}
>  #endif /* __PMEM_H__ */
> diff --git a/mm/msync.c b/mm/msync.c
> index bb04d53..2a4739c 100644
> --- a/mm/msync.c
> +++ b/mm/msync.c
> @@ -7,6 +7,7 @@
>  /*
>   * The msync() system call.
>   */
> +#include <linux/dax.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/mman.h>
> @@ -59,6 +60,7 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
>  	for (;;) {
>  		struct file *file;
>  		loff_t fstart, fend;
> +		unsigned long range_len;
>  
>  		/* Still start < end. */
>  		error = -ENOMEM;
> @@ -77,10 +79,16 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
>  			error = -EBUSY;
>  			goto out_unlock;
>  		}
> +
> +		range_len = min(end, vma->vm_end) - start;
> +
> +		if (vma_is_dax(vma))
> +			dax_sync_range(start, range_len);
> +

Ye no I hate this. (No need to touch mm)

All we need to do is define a dax_fsync()

dax FS registers a special dax vector for ->fsync() that vector
needs to call dax_fsync(); first as part of its fsync operation.
Then dax_fsync() is:

dax_fsync()
{
	/* we always write with sync so only fsync if the file mmap'ed */
	if (mapping_mapped(inode->i_mapping) == 0)
		return 0;

	dax_sync_range(start, range_len);
}

Thanks
Boaz

>  		file = vma->vm_file;
>  		fstart = (start - vma->vm_start) +
>  			 ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> -		fend = fstart + (min(end, vma->vm_end) - start) - 1;
> +		fend = fstart + range_len - 1;
>  		start = vma->vm_end;
>  		if ((flags & MS_SYNC) && file &&
>  				(vma->vm_flags & VM_SHARED)) {
> 

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

[toc] | [prev] | [next] | [standalone]

#1217773

On Tue, Sep 01, 2015 at 04:12:42PM +0300, Boaz Harrosh wrote:
> On 08/31/2015 09:59 PM, Ross Zwisler wrote:
> > @@ -753,3 +755,18 @@ int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block)
> >  	return dax_zero_page_range(inode, from, length, get_block);
> >  }
> >  EXPORT_SYMBOL_GPL(dax_truncate_page);
> > +
> > +void dax_sync_range(unsigned long addr, size_t len)
> > +{
> > +	while (len) {
> > +		size_t chunk_len = min_t(size_t, SZ_1G, len);
> > +
> 
> Where does the  SZ_1G come from is it because you want to do cond_resched()
> every 1G bytes so not to get stuck for a long time?
> 
> It took me a while to catch, At first I thought it might be do to wb_cache_pmem()
> limitations. Would you put a comment in the next iteration?

Yep, the SZ_1G is just to make sure we cond_reshced() every once in a while.
Is there a documented guideline somewhere as to how long a kernel thread is
allowed to spin before calling cond_resched()?   So far I haven' been able to
find anything solid on this - it seems like each developer has their own
preferences, and that those preferences vary pretty widely.

In any case, assuming we continue to separate the msync() and fsync()
implementations for DAX (which right now I'm doubting, to be honest), I'll add
in a comment to explain this logic.

> > diff --git a/include/linux/pmem.h b/include/linux/pmem.h
> > index 85f810b3..aa29ebb 100644
> > --- a/include/linux/pmem.h
> > +++ b/include/linux/pmem.h
> > @@ -53,12 +53,18 @@ static inline void arch_clear_pmem(void __pmem *addr, size_t size)
> >  {
> >  	BUG();
> 
> See below
> 
> >  }
> > +
> > +static inline void arch_wb_cache_pmem(void __pmem *addr, size_t size)
> > +{
> > +	BUG();
> 
> There is a clflush_cache_range() defined for generic use. On ADR systems (even without pcommit)
> this works perfectly and is persistent. why not use that in the generic case?

Nope, we really do need to use wb_cache_pmem() because clflush_cache_range()
isn't an architecture neutral API.  wb_cache_pmem() also has the advantage
that on x86 it will take advantage of the new CLWB instruction if it is
available on the platform, and it doesn't introduce any unnecessary memory
fencing.  This works on both PCOMMIT-aware systems and on ADR boxes without
PCOMMIT.
 
> One usage of pmem is overlooked by all this API. The use of DRAM as pmem, across a VM
> or cross reboot. you have a piece of memory exposed as pmem to the subsytem which survives
> past the boot of that system. The CPU cache still needs flushing in this case.
> (People are already using this for logs and crash dumps)

I'm confused about this "DRAM as pmem" use case - are the requirements
essentially the same as the ADR case?  You need to make sure that pre-reboot
the dirty cache lines have been flushed from the processor cache, but if they
are in platform buffers (the "safe zone" for ADR) you're fine?

If so, we're good to go, I think.  Dan's most recent patch series made it so
we correctly handle systems that have the PMEM API but not PCOMMIT:

https://lists.01.org/pipermail/linux-nvdimm/2015-August/002005.html

If the "DRAM as pmem across reboots" case isn't okay with your dirty data
being in the ADR safe zone, I think you're toast.  Without PCOMMIT the kernel
cannot guarantee that the data has ever made it durably to the DIMMs,
regardless what clflush/clflushopt/clwb magic you do.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web