Re: [PATCH 03/13] libnvdimm: introduce nvdimm_flush()

[Jeff Moyer <jmoyer@redhat.com>]

Dan Williams <dan.j.williams@intel.com> writes:

> nvdimm_flush() is an alternative to the x86 pcommit instruction.  It is
> an optional write flushing mechanism that an nvdimm bus can provide for
> the pmem driver to consume.  In the case of the NFIT nvdimm-bus-provider
> nvdimm_flush() is implemented as a series of flush-hint-address [1]
> writes to each dimm in the interleave set that backs the namespace.  For
> now this implementation is just a simple replacement of wmb_pmem() /
> arch_has_wmb_pmem() with nvdimm_flush() / nvdimm_has_flush().

Dan, this needs a whole heck of a lot more explanation.  As I understand
it, you're talking about flushing write pending queues (not the CPU
cache).  And given that the write pending queues are part of the
persistence domain on systems with ADR (now required for pmem), they
don't need to be flushed for normal pmem, only for block window
accesses.  And so this confuses me:

> We defer the full implementation of nvdimm_flush() until the
>implementation is prepared to also handle the blk-region case.

The one thing they're required to address is not addressed?  I
understand that you may want to push data out as far as possible to
limit the exposure to hardware failures, but that's not how you're
positioning this patch, and so it's very confusing.

Please also make the documentation above nvdimm_flush and
nvdimm_has_flush much more idiot-proof.

> @@ -234,7 +249,7 @@ static int pmem_attach_disk(struct device *dev,
>  	dev_set_drvdata(dev, pmem);
>  	pmem->phys_addr = res->start;
>  	pmem->size = resource_size(res);
> -	if (!arch_has_wmb_pmem())
> +	if (nvdimm_has_flush(nd_region) < 0)
>  		dev_warn(dev, "unable to guarantee persistence of writes\n");

And this doesn't make sense to me, either.  NVDIMM-N's do not have to
have flush hint addresses in order to guarantee persistence.  I guess
that's no different than what we have now, but you're sure not
addressing this bogus warning message.  And yes, I know there's no way
to query the platform to see if ADR is available.  But how many people
do you think are sticking NVDIMM-Ns in systems that do not have ADR?
Just get rid of the message already.

Cheers,
Jeff