On Tue, Sep 5, 2017 at 11:29 PM, Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
On Thu, 31 Aug 2017, Dan Williams wrote:
> On Thu, Aug 31, 2017 at 8:04 PM, Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> >
> >
> > On Thu, 31 Aug 2017, Dan Williams wrote:
> >
> >> On Thu, Aug 31, 2017 at 6:47 PM, Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> >> > The patch abebfbe2f731 ("dm: add ->flush() dax operation support") is
> >> > buggy. A dm device may be composed of multiple underlying devices and all
> >> > of them need to be flushed. The patch just routes the flush request to the
> >> > first device and ignores the other devices.
> >> >
> >> > It could be fixed by adding more complex logic to the device mapper. But
> >> > there is only one implementation of the method pmem_dax_ops->flush - that
> >> > is pmem_dax_flush() - and it calls arch_wb_cache_pmem(). Consequently, we
> >> > don't need the pmem_dax_ops->flush abstraction at all, we can call
> >> > arch_wb_cache_pmem() directly from dax_flush() because dax_dev->ops->flush
> >> > couldn't ever reach anything different from arch_wb_cache_pmem().
> >>
> >> Unfortunately, this is not true, see usage of DAXDEV_WRITE_CACHE. This
> >> is for platforms that arrange for cpu caches to be flushed on
> >> power-fail, like standalone storage appliances, where it would be a
> >
> > BTW. if the system is capable of flushing caches on power failure, it is
> > system-wide feature, it is not per-device feature. The CPU caches may
> > store data for any persistent memory device.
> >
> > You either have enough remaining power to flush the CPU caches or not.
>
> Whether you need to flush caches or not can be a per-address range
> attribute and can change over time.
There is only one way to flush the cache (the clwb instruction), you don't
need a method that abstracts over this instruction. A simple flag or
static key that skips clwb() is sufficient.
Linux kernel API is changeable, so you don't need to overdesign the API
for future. If it ever happens that we have a system with two types of
persistent memory that need different instructions to flush them, we can
design a proper abstraction. But there is no reason to design the
abstraction in advance for a situation that will perhaps never happen.
True, it is over designed and we can cross that complexity bridge in the future when / if it arises.
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:
Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
