Re: [PATCH v2 2/2] dax,pmem: Implement pmem based dax data recovery

[Jane Chu <jane.chu@oracle.com>]

On 11/5/2021 7:04 PM, Darrick J. Wong wrote:
<snip>
>> @@ -303,20 +303,85 @@ static long pmem_dax_direct_access(struct dax_device *dax_dev,
>>   }
>>   
>>   /*
>> - * Use the 'no check' versions of copy_from_iter_flushcache() and
>> - * copy_mc_to_iter() to bypass HARDENED_USERCOPY overhead. Bounds
>> - * checking, both file offset and device offset, is handled by
>> - * dax_iomap_actor()
>> + * Even though the 'no check' versions of copy_from_iter_flushcache()
>> + * and copy_mc_to_iter() are used to bypass HARDENED_USERCOPY overhead,
>> + * 'read'/'write' aren't always safe when poison is consumed. They happen
>> + * to be safe because the 'read'/'write' range has been guaranteed
>> + * be free of poison(s) by a prior call to dax_direct_access() on the
>> + * caller stack.
>> + * But on a data recovery code path, the 'read'/'write' range is expected
>> + * to contain poison(s), and so poison(s) is explicit checked, such that
>> + * 'read' can fetch data from clean page(s) up till the first poison is
>> + * encountered, and 'write' requires the range be page aligned in order
>> + * to restore the poisoned page's memory type back to "rw" after clearing
>> + * the poison(s).
>> + * In the event of poison related failure, (size_t) -EIO is returned and
>> + * caller may check the return value after casting it to (ssize_t).
>> + *
>> + * TODO: add support for CPUs that support MOVDIR64B instruction for
>> + * faster poison clearing, and possibly smaller error blast radius.
> 
> I get that it's still early days yet for whatever pmem stuff is going on
> for 5.17, but I feel like this ought to be a separate function called by
> pmem_copy_from_iter, with this huge comment attached to that recovery
> function.

Thanks, will refactor both functions.

> 
>>    */
>>   static size_t pmem_copy_from_iter(struct dax_device *dax_dev, pgoff_t pgoff,
>>   		void *addr, size_t bytes, struct iov_iter *i, int mode)
>>   {
>> +	phys_addr_t pmem_off;
>> +	size_t len, lead_off;
>> +	struct pmem_device *pmem = dax_get_private(dax_dev);
>> +	struct device *dev = pmem->bb.dev;
>> +
>> +	if (unlikely(mode == DAX_OP_RECOVERY)) {
>> +		lead_off = (unsigned long)addr & ~PAGE_MASK;
>> +		len = PFN_PHYS(PFN_UP(lead_off + bytes));
>> +		if (is_bad_pmem(&pmem->bb, PFN_PHYS(pgoff) / 512, len)) {
>> +			if (lead_off || !(PAGE_ALIGNED(bytes))) {
>> +				dev_warn(dev, "Found poison, but addr(%p) and/or bytes(%#lx) not page aligned\n",
>> +					addr, bytes);
>> +				return (size_t) -EIO;
>> +			}
>> +			pmem_off = PFN_PHYS(pgoff) + pmem->data_offset;
>> +			if (pmem_clear_poison(pmem, pmem_off, bytes) !=
>> +						BLK_STS_OK)
>> +				return (size_t) -EIO;
> 
> Looks reasonable enough to me, though you might want to restructure this
> to reduce the amount of indent.

Agreed.

> 
> FWIW I dislike how is_bad_pmem mixes units (sector_t vs. bytes), that
> was seriously confusing.  But I guess that's a weird quirk of the
> badblocks API and .... ugh.
> 
> (I dunno, can we at least clean up the nvdimm parts and some day replace
> the badblocks backend with something that can handle more than 16
> records?  interval_tree is more than up to that task, I know, I use it
> for xfs online fsck...)

Let me look into this and get back to you.

> 
>> +		}
>> +	}
>> +
>>   	return _copy_from_iter_flushcache(addr, bytes, i);
>>   }
>>   
>>   static size_t pmem_copy_to_iter(struct dax_device *dax_dev, pgoff_t pgoff,
>>   		void *addr, size_t bytes, struct iov_iter *i, int mode)
>>   {
>> +	int num_bad;
>> +	size_t len, lead_off;
>> +	unsigned long bad_pfn;
>> +	bool bad_pmem = false;
>> +	size_t adj_len = bytes;
>> +	sector_t sector, first_bad;
>> +	struct pmem_device *pmem = dax_get_private(dax_dev);
>> +	struct device *dev = pmem->bb.dev;
>> +
>> +	if (unlikely(mode == DAX_OP_RECOVERY)) {
>> +		sector = PFN_PHYS(pgoff) / 512;
>> +		lead_off = (unsigned long)addr & ~PAGE_MASK;
>> +		len = PFN_PHYS(PFN_UP(lead_off + bytes));
>> +		if (pmem->bb.count)
>> +			bad_pmem = !!badblocks_check(&pmem->bb, sector,
>> +					len / 512, &first_bad, &num_bad);
>> +		if (bad_pmem) {
>> +			bad_pfn = PHYS_PFN(first_bad * 512);
>> +			if (bad_pfn == pgoff) {
>> +				dev_warn(dev, "Found poison in page: pgoff(%#lx)\n",
>> +					pgoff);
>> +				return -EIO;
>> +			}
>> +			adj_len = PFN_PHYS(bad_pfn - pgoff) - lead_off;
>> +			dev_WARN_ONCE(dev, (adj_len > bytes),
>> +					"out-of-range first_bad?");
>> +		}
>> +		if (adj_len == 0)
>> +			return (size_t) -EIO;
> 
> Uh, are we supposed to adjust bytes here or something?

Because we're trying to read as much data as possible...
What is your concern here?

thanks!
-jane

> 
> --D
> 
>> +	}
>> +
>>   	return _copy_mc_to_iter(addr, bytes, i);
>>   }
>>