linux-kernel - Re: [PATCH 06/12] mm/truncate: add folio_unmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5cb98ddb-744a-4fc8-b793-9dbe56e16f35@kernel.dk>
Date: Fri, 20 Dec 2024 09:28:46 -0700
From: Jens Axboe <axboe@...nel.dk>
To: Matthew Wilcox <willy@...radead.org>
Cc: linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, hannes@...xchg.org,
 clm@...a.com, linux-kernel@...r.kernel.org, kirill@...temov.name,
 bfoster@...hat.com
Subject: Re: [PATCH 06/12] mm/truncate: add folio_unmap_invalidate() helper

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