Re: [PATCH 05/13] x86, dax: replace clear_pmem() with open coded memset + dax_ops->flush

[Dan Williams]

On Fri, Jan 20, 2017 at 2:27 AM, Jan Kara <jack@xxxxxxx> wrote:
> On Thu 19-01-17 19:50:39, Dan Williams wrote:
>> The clear_pmem() helper simply combines a memset() plus a cache flush.
>> Now that the flush routine is optionally provided by the dax device
>> driver we can avoid unnecessary cache management on dax devices fronting
>> volatile memory.
>>
>> With clear_pmem() gone we can follow on with a patch to make pmem cache
>> management completely defined within the pmem driver.
> ...
>> diff --git a/fs/dax.c b/fs/dax.c
>> index 160024e403f6..8883ce4d391e 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -986,6 +986,7 @@ static bool dax_range_is_aligned(struct block_device *bdev,
>>  int __dax_zero_page_range(struct block_device *bdev, sector_t sector,
>>               unsigned int offset, unsigned int length)
>>  {
>> +     const struct dax_operations *dax_ops = to_dax_ops(bdev);
>>       struct blk_dax_ctl dax = {
>>               .sector         = sector,
>>               .size           = PAGE_SIZE,
>> @@ -999,7 +1000,9 @@ int __dax_zero_page_range(struct block_device *bdev, sector_t sector,
>>       } else {
>>               if (dax_map_atomic(bdev, &dax) < 0)
>>                       return PTR_ERR(dax.addr);
>> -             clear_pmem(dax.addr + offset, length);
>> +             memset(dax.addr + offset, 0, length);
>> +             if (dax_ops->flush)
>> +                     dax_ops->flush(dax.addr + offset, length);
>>               dax_unmap_atomic(bdev, &dax);
>>       }
>>       return 0;
>
> Shouldn't we rather have some callback in dax_ops for clearing memory?
> If we had all accesses to persistent memory inside DAX code wrapped inside
> appropriate device wrappers that can report errors, we can have proper
> error handling for the case we hit MCE, can't we?

That's true. I was thinking along those lines in the badblocks thread
and then missed that I did not give clearing pmem the same treatment.
No objections from me, I'll add ->clear() to dax_ops.