Re: [PATCH 06/12] mm/truncate: add folio_unmap_invalidate() helper

[Jens Axboe]

On 12/20/24 9:28 AM, Jens Axboe wrote:
> On 12/20/24 9:21 AM, Matthew Wilcox wrote:
>> On Fri, Dec 20, 2024 at 08:47:44AM -0700, Jens Axboe wrote:
>>> +int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
>>> +			   gfp_t gfp)
>>>  {
>>> -	if (folio->mapping != mapping)
>>> -		return 0;
>>> +	int ret;
>>> +
>>> +	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>>>  
>>> -	if (!filemap_release_folio(folio, GFP_KERNEL))
>>> +	if (folio_test_dirty(folio))
>>>  		return 0;
>>> +	if (folio_mapped(folio))
>>> +		unmap_mapping_folio(folio);
>>> +	BUG_ON(folio_mapped(folio));
>>> +
>>> +	ret = folio_launder(mapping, folio);
>>> +	if (ret)
>>> +		return ret;
>>> +	if (folio->mapping != mapping)
>>> +		return -EBUSY;
>>
>> The position of this test confuses me.  Usually we want to test
>> folio->mapping early on, since if the folio is no longer part of this
>> file, we want to stop doing things to it, rather than go to the trouble
>> of unmapping it.  Also, why do we want to return -EBUSY in this case?
>> If the folio is no longer part of this file, it has been successfully
>> removed from this file, right?
> 
> It's simply doing what the code did before. I do agree the mapping check
> is a bit odd at that point, but that's how
> invalidate_inode_pages2_range() and folio_launder() was setup. We can
> certainly clean that up after the merge of these helpers, but I didn't
> want to introduce any potential changes with this merge.
> 
> -EBUSY was the return from a 0 return from those two helpers before.

Any further concerns with this? Trying to nudge this patchset forward...
It's not like there's a lot of time left for 6.14.

-- 
Jens Axboe