linux-kernel - Re: [PATCH 5/6] iomap: drop unnecessary state

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: <ed3c1368-766b-9a54-ec88-b0cde033775f@huaweicloud.com>
Date: Wed, 7 Aug 2024 19:39:05 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.cz>
Cc: Dave Chinner <david@...morbit.com>, linux-xfs@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 djwong@...nel.org, hch@...radead.org, brauner@...nel.org,
 yi.zhang@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH 5/6] iomap: drop unnecessary state_lock when setting ifs
 uptodate bits

On 2024/8/5 23:48, Matthew Wilcox wrote:
> On Mon, Aug 05, 2024 at 04:00:23PM +0200, Jan Kara wrote:
>> Actually add Matthew to CC ;)
> 
> It's OK, I was reading.
> 
> FWIW, I agree with Dave; the locking complexity in this patch was
> horrendous.  I was going to get to the same critique he had, but I first
> wanted to understand what the thought process was.

Yes, I'd like to change to use the solution as Dave suggested.

> 
>>>> Ha, right, I missed the comments of this function, it means that there are
>>>> some special callers that hold table lock instead of folio lock, is it
>>>> pte_alloc_map_lock?
>>>>
>>>> I checked all the filesystem related callers and didn't find any real
>>>> caller that mark folio dirty without holding folio lock and that could
>>>> affect current filesystems which are using iomap framework, it's just
>>>> a potential possibility in the future, am I right?
> 
> Filesystems are normally quite capable of taking the folio lock to
> prevent truncation.  It's the MM code that needs the "or holding the
> page table lock" get-out clause.  I forget exactly which callers it
> is; I worked through them a few times.  It's not hard to put a
> WARN_ON_RATELIMIT() into folio_mark_dirty() and get a good sampling.
> 
> There's also a "or holding a buffer_head locked" get-out clause that
> I'm not sure is documented anywhere, but obviously that doesn't apply
> to the iomap code.

Thanks for your answer, I've found some callers.

Thanks,
Yi.

> 
>>> There used to be quite a few places doing that. Now that I've checked all
>>> places I was aware of got actually converted to call folio_mark_dirty() under
>>> a folio lock (in particular all the cases happening on IO completion, folio
>>> unmap etc.). Matthew, are you aware of any place where folio_mark_dirty()
>>> would be called for regular file page cache (block device page cache is in a
>>> different situation obviously) without folio lock held?
> 
> Yes, the MM code definitely applies to regular files as well as block
> devices.
>