On 3/27/26 16:05, Zi Yan wrote:
> On 27 Mar 2026, at 10:23, Lorenzo Stoakes (Oracle) wrote:
>
>> On Fri, Mar 27, 2026 at 02:58:12PM +0100, David Hildenbrand (Arm) wrote:
>>>
>>> There could now be a race between collapsing and the file getting opened
>>> r/w.
>>>
>>> Are we sure that all code can really deal with that?
>>>
>>> IOW, "they already had to handle it separately" -- is that true?
>>> khugepaged would have never collapse in writable files, so I wonder if
>>> all code paths are prepared for that.
>>
>> OK I guess I overlooked a part of this code... :) see below.
>>
>> This is fine and would be a no-op anyway
>>
>> - if (f->f_mode & FMODE_WRITE) {
>> - /*
>> - * Depends on full fence from get_write_access() to
>> synchronize
>> - * against collapse_file() regarding i_writecount and nr_thps
>> - * updates. Ensures subsequent insertion of THPs into the
>> page
>> - * cache will fail.
>> - */
>> - if (filemap_nr_thps(inode->i_mapping)) {
>>
>> But this:
>>
>> - if (!is_shmem) {
>> - filemap_nr_thps_inc(mapping);
>> - /*
>> - * Paired with the fence in do_dentry_open() ->
>> get_write_access()
>> - * to ensure i_writecount is up to date and the update to
>> nr_thps
>> - * is visible. Ensures the page cache will be truncated if
>> the
>> - * file is opened writable.
>> - */
>> - smp_mb();
>>
>> We can drop barrier
>>
>> - if (inode_is_open_for_write(mapping->host)) {
>> - result = SCAN_FAIL;
>>
>> But this is a functional change!
>>
>> Yup missed this.
>
> But I added
>
> + if (!is_shmem && inode_is_open_for_write(mapping->host))
> + result = SCAN_FAIL;
>
> That keeps the original bail out, right?
Independent of that, are we sure that the possible race we allow is ok?
--
Cheers,
David
