Re: [PATCH v10 2/3] fs: introduce super_drop_pagecache()

[Shiyang Ruan <ruansy.fnst@fujitsu.com>]

在 2023/2/19 2:27, Matthew Wilcox 写道:
> On Sat, Feb 18, 2023 at 09:16:43AM +0800, Shiyang Ruan wrote:
>> 在 2023/2/18 0:14, Matthew Wilcox 写道:
>>> On Fri, Feb 17, 2023 at 02:48:31PM +0000, Shiyang Ruan wrote:
>>>> -		invalidate_mapping_pages(inode->i_mapping, 0, -1);
>>>> -		iput(toput_inode);
>>>> -		toput_inode = inode;
>>>> -
>>>> -		cond_resched();
>>>> -		spin_lock(&sb->s_inode_list_lock);
>>>> -	}
>>>> -	spin_unlock(&sb->s_inode_list_lock);
>>>> -	iput(toput_inode);
>>>> +	super_drop_pagecache(sb, invalidate_inode_pages);
>>>
>>> I thought I explained last time that you can do this with
>>> invalidate_mapping_pages() / invalidate_inode_pages2_range() ?
>>> Then you don't need to introduce invalidate_inode_pages().
>>>
>>>> +void super_drop_pagecache(struct super_block *sb,
>>>> +	int (*invalidator)(struct address_space *))
>>>
>>> void super_drop_pagecache(struct super_block *sb,
>>> 		int (*invalidate)(struct address_space *, pgoff_t, pgoff_t))
>>>
>>>> +		invalidator(inode->i_mapping);
>>>
>>> 		invalidate(inode->i_mapping, 0, -1)
>>>
>>> ... then all the changes to mm/truncate.c and filemap.h go away.
>>
>> Yes, I tried as you suggested, but I found that they don't have same type of
>> return value.
>>
>> int invalidate_inode_pages2_range(struct address_space *mapping,
>> 				  pgoff_t start, pgoff_t end);
>>
>> unsigned long invalidate_mapping_pages(struct address_space *mapping,
>> 		pgoff_t start, pgoff_t end);
> 
> Oh, that's annoying.  Particularly annoying is that the return value
> for invalidate_mapping_pages() is used by fs/inode.c to account for
> the number of pages invalidate, and the return value for
> invalidate_inode_pages2_range() is used by several filesystems
> to know whether an error occurred.
> 
> Hm.  Shouldn't you be checking for an error from
> invalidate_inode_pages2_range()?  Seems like it can return -EBUSY for
> DAX entries.
> 
> With that in mind, the wrapper you actually want to exist is
> 
> static int invalidate_inode_pages_range(struct address_space *mapping,
> 				pgoff_t start, pgoff_t end)
> {
> 	invalidate_mapping_pages(mapping, start, end);
> 	return 0;
> }
> 
> Right?

So, I should introduce this wrapper in fs/xfs/xfs_notify_failure.c 
because it is the only one who calls this wrapper.  Ok, got it!


--
Thanks,
Ruan.