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.
--
Jens Axboe
