Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 05/11] mm, fsdax: Refactor memory-failure handler for
 dax mapping
To:     Christoph Hellwig <hch@lst.de>
CC:     <linux-kernel@vger.kernel.org>, <linux-xfs@vger.kernel.org>,
        <linux-nvdimm@lists.01.org>, <linux-mm@kvack.org>,
        <linux-fsdevel@vger.kernel.org>, <dm-devel@redhat.com>,
        <darrick.wong@oracle.com>, <dan.j.williams@intel.com>,
        <david@fromorbit.com>, <agk@redhat.com>, <snitzer@redhat.com>,
        <rgoldwyn@suse.de>, <qi.fuli@fujitsu.com>, <y-goto@fujitsu.com>
References: <20210208105530.3072869-1-ruansy.fnst@cn.fujitsu.com>
 <20210208105530.3072869-6-ruansy.fnst@cn.fujitsu.com>
 <20210210133347.GD30109@lst.de>
From:   Ruan Shiyang <ruansy.fnst@cn.fujitsu.com>
Message-ID: <45a20d88-63ee-d678-ad86-6ccd8cdf7453@cn.fujitsu.com>
Date:   Wed, 17 Feb 2021 10:56:11 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210210133347.GD30109@lst.de>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2021/2/10 下午9:33, Christoph Hellwig wrote:
>> +extern int mf_dax_mapping_kill_procs(struct address_space *mapping, pgoff_t index, int flags);
> 
> No nee for the extern, please avoid the overly long line.

OK.

I'd like to confirm one thing...  I have checked all of this patchset by 
checkpatch.pl and it did not report the overly long line warning.  So, I 
should still obey the rule of 80 chars one line?

> 
>> @@ -120,6 +121,13 @@ static int hwpoison_filter_dev(struct page *p)
>>   	if (PageSlab(p))
>>   		return -EINVAL;
>>   
>> +	if (pfn_valid(page_to_pfn(p))) {
>> +		if (is_device_fsdax_page(p))
>> +			return 0;
>> +		else
>> +			return -EINVAL;
>> +	}
>> +
> 
> This looks odd.  For one there is no need for an else after a return.
> But more importantly page_mapping() as called below pretty much assumes
> a valid PFN, so something is fishy in this function.

Yes, a mistake here.  I'll fix it.

> 
>> +	if (is_zone_device_page(p)) {
>> +		if (is_device_fsdax_page(p))
>> +			tk->addr = vma->vm_start +
>> +					((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
> 
> The arithmetics here scream for a common helper, I suspect there might
> be other places that could use the same helper.
> 
>> +int mf_dax_mapping_kill_procs(struct address_space *mapping, pgoff_t index, int flags)
> 
> Overly long line.  Also the naming scheme with the mf_ seems rather
> unusual. Maybe dax_kill_mapping_procs?  Also please add a kerneldoc
> comment describing the function given that it exported.
> 

OK.  Thanks for your guidance.


--
Thanks,
Ruan Shiyang.

>