Re: [PATCH v5 5/7] pmem: add pmem_recovery_write() dax op

Christoph Hellwig
2022-02-02T13:43:54Z

> @@ -257,10 +263,15 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector,
> __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
> long nr_pages, void **kaddr, pfn_t *pfn)
> {
> + bool bad_pmem;
> + bool do_recovery = false;
> resource_size_t offset = PFN_PHYS(pgoff) + pmem->data_offset;
>
> - if (unlikely(is_bad_pmem(&pmem->bb, PFN_PHYS(pgoff) / 512,
> - PFN_PHYS(nr_pages))))
> + bad_pmem = is_bad_pmem(&pmem->bb, PFN_PHYS(pgoff) / 512,
> + PFN_PHYS(nr_pages));
> + if (bad_pmem && kaddr)
> + do_recovery = dax_recovery_started(pmem->dax_dev, kaddr);
> + if (bad_pmem && !do_recovery)
> return -EIO;

I find the passing of the recovery flag through the address very
cumbersome. I remember there was some kind of discussion, but this looks
pretty ugly.

Also no need for the bad_pmem variable:

if (is_bad_pmem(&pmem->bb, PFN_PHYS(pgoff) / 512, PFN_PHYS(nr_pages)) &&
(!kaddr | !dax_recovery_started(pmem->dax_dev, kaddr)))
return -EIO;

Also: the !kaddr check could go into dax_recovery_started. That way
even if we stick with the overloading kaddr could also be used just for
the flag if needed.