Re: [RFC PATCH v3 4/9] mm, fsdax: Refactor memory-failure handler for dax mapping

From: Ruan Shiyang <ruansy.fnst@cn.fujitsu.com>
To: Dave Chinner <david@fromorbit.com>
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>, <linux-raid@vger.kernel.org>,
	<darrick.wong@oracle.com>, <dan.j.williams@intel.com>,
	<hch@lst.de>, <song@kernel.org>, <rgoldwyn@suse.de>,
	<qi.fuli@fujitsu.com>, <y-goto@fujitsu.com>
Subject: Re: [RFC PATCH v3 4/9] mm, fsdax: Refactor memory-failure handler for dax mapping
Date: Fri, 18 Dec 2020 09:48:34 +0800	[thread overview]
Message-ID: <513e7602-80d7-8d8c-ed5d-06b8113823bf@cn.fujitsu.com> (raw)
In-Reply-To: <20201216212648.GN632069@dread.disaster.area>

On 2020/12/17 上午5:26, Dave Chinner wrote:
> On Tue, Dec 15, 2020 at 08:14:09PM +0800, Shiyang Ruan wrote:
>> The current memory_failure_dev_pagemap() can only handle single-mapped
>> dax page for fsdax mode.  The dax page could be mapped by multiple files
>> and offsets if we let reflink feature & fsdax mode work together.  So,
>> we refactor current implementation to support handle memory failure on
>> each file and offset.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@cn.fujitsu.com>
>> ---
> .....
>>   static const char *action_name[] = {
>> @@ -1147,6 +1148,60 @@ static int try_to_split_thp_page(struct page *page, const char *msg)
>>   	return 0;
>>   }
>>   
>> +int mf_dax_mapping_kill_procs(struct address_space *mapping, pgoff_t index, int flags)
>> +{
>> +	const bool unmap_success = true;
>> +	unsigned long pfn, size = 0;
>> +	struct to_kill *tk;
>> +	LIST_HEAD(to_kill);
>> +	int rc = -EBUSY;
>> +	loff_t start;
>> +	dax_entry_t cookie;
>> +
>> +	/*
>> +	 * Prevent the inode from being freed while we are interrogating
>> +	 * the address_space, typically this would be handled by
>> +	 * lock_page(), but dax pages do not use the page lock. This
>> +	 * also prevents changes to the mapping of this pfn until
>> +	 * poison signaling is complete.
>> +	 */
>> +	cookie = dax_lock(mapping, index, &pfn);
>> +	if (!cookie)
>> +		goto unlock;
> 
> Why do we need to prevent the inode from going away here? This
> function gets called by XFS after doing an xfs_iget() call to grab
> the inode that owns the block. Hence the the inode (and the mapping)
> are guaranteed to be referenced and can't go away. Hence for the
> filesystem based callers, this whole "dax_lock()" thing can go away >
> So, AFAICT, the dax_lock() stuff is only necessary when the
> filesystem can't be used to resolve the owner of physical page that
> went bad....

Yes, you are right.  I made a mistake in the calling sequence here. 
Thanks for pointing out.

--
Thanks,
Ruan Shiyang.

> 
> Cheers,
> 
> Dave.
> 