On Tue, Sep 5, 2017 at 11:29 PM, Mikulas Patocka <[email protected]> wrote:
> > > On Thu, 31 Aug 2017, Dan Williams wrote: > > > On Thu, Aug 31, 2017 at 8:04 PM, Mikulas Patocka <[email protected]> > wrote: > > > > > > > > > On Thu, 31 Aug 2017, Dan Williams wrote: > > > > > >> On Thu, Aug 31, 2017 at 6:47 PM, Mikulas Patocka <[email protected]> > 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 <[email protected]>
-- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
