[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 2 Feb 2022 22:13:26 +0000
From: Jane Chu <jane.chu@...cle.com>
To: Christoph Hellwig <hch@...radead.org>
CC: "david@...morbit.com" <david@...morbit.com>,
"djwong@...nel.org" <djwong@...nel.org>,
"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
"vishal.l.verma@...el.com" <vishal.l.verma@...el.com>,
"dave.jiang@...el.com" <dave.jiang@...el.com>,
"agk@...hat.com" <agk@...hat.com>,
"snitzer@...hat.com" <snitzer@...hat.com>,
"dm-devel@...hat.com" <dm-devel@...hat.com>,
"ira.weiny@...el.com" <ira.weiny@...el.com>,
"willy@...radead.org" <willy@...radead.org>,
"vgoyal@...hat.com" <vgoyal@...hat.com>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"nvdimm@...ts.linux.dev" <nvdimm@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>
Subject: Re: [PATCH v5 5/7] pmem: add pmem_recovery_write() dax op
On 2/2/2022 5:43 AM, Christoph Hellwig wrote:
>> @@ -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;
Okay.
>
> 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.
The !kaddr check is in dax_recovery_started(), and that reminded me the
check should be in dax_prep_recovery() too.
Thanks!
-jane
Powered by blists - more mailing lists