<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 5, 2017 at 11:29 PM, Mikulas Patocka <span dir="ltr"><<a href="mailto:mpatocka@redhat.com" target="_blank">mpatocka@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"><br>
<br>
On Thu, 31 Aug 2017, Dan Williams wrote:<br>
<br>
> On Thu, Aug 31, 2017 at 8:04 PM, Mikulas Patocka <<a href="mailto:mpatocka@redhat.com">mpatocka@redhat.com</a>> wrote:<br>
> ><br>
> ><br>
> > On Thu, 31 Aug 2017, Dan Williams wrote:<br>
> ><br>
> >> On Thu, Aug 31, 2017 at 6:47 PM, Mikulas Patocka <<a href="mailto:mpatocka@redhat.com">mpatocka@redhat.com</a>> wrote:<br>
> >> > The patch abebfbe2f731 ("dm: add ->flush() dax operation support") is<br>
> >> > buggy. A dm device may be composed of multiple underlying devices and all<br>
> >> > of them need to be flushed. The patch just routes the flush request to the<br>
> >> > first device and ignores the other devices.<br>
> >> ><br>
> >> > It could be fixed by adding more complex logic to the device mapper. But<br>
> >> > there is only one implementation of the method pmem_dax_ops->flush - that<br>
> >> > is pmem_dax_flush() - and it calls arch_wb_cache_pmem(). Consequently, we<br>
> >> > don't need the pmem_dax_ops->flush abstraction at all, we can call<br>
> >> > arch_wb_cache_pmem() directly from dax_flush() because dax_dev->ops->flush<br>
> >> > couldn't ever reach anything different from arch_wb_cache_pmem().<br>
> >><br>
> >> Unfortunately, this is not true, see usage of DAXDEV_WRITE_CACHE. This<br>
> >> is for platforms that arrange for cpu caches to be flushed on<br>
> >> power-fail, like standalone storage appliances, where it would be a<br>
> ><br>
> > BTW. if the system is capable of flushing caches on power failure, it is<br>
> > system-wide feature, it is not per-device feature. The CPU caches may<br>
> > store data for any persistent memory device.<br>
> ><br>
> > You either have enough remaining power to flush the CPU caches or not.<br>
><br>
> Whether you need to flush caches or not can be a per-address range<br>
> attribute and can change over time.<br>
<br>
</span>There is only one way to flush the cache (the clwb instruction), you don't<br>
need a method that abstracts over this instruction. A simple flag or<br>
static key that skips clwb() is sufficient.<br>
<br>
Linux kernel API is changeable, so you don't need to overdesign the API<br>
for future. If it ever happens that we have a system with two types of<br>
persistent memory that need different instructions to flush them, we can<br>
design a proper abstraction. But there is no reason to design the<br>
abstraction in advance for a situation that will perhaps never happen.<br></blockquote><div><br></div><div>True, it is over designed and we can cross that complexity bridge in the future when / if it arises.</div><div><br></div><div>Since dm is already checking dax_write_cache_enabled() to set its own flush enabled bit I think your patch looks good. You can add:</div><div><br></div><div>Reviewed-by: Dan Williams <<a href="mailto:dan.j.williams@intel.com">dan.j.williams@intel.com</a>></div><div><br></div></div></div></div>