Date:   Thu, 25 May 2023 20:39:32 -0400
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Andreas Grünbacher 
        <andreas.gruenbacher@...il.com>
Cc:     Christoph Hellwig <hch@...radead.org>, Jan Kara <jack@...e.cz>,
        cluster-devel@...hat.com, "Darrick J . Wong" <djwong@...nel.org>,
        linux-kernel@...r.kernel.org, dhowells@...hat.com,
        linux-bcachefs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Kent Overstreet <kent.overstreet@...il.com>
Subject: Re: [Cluster-devel] [PATCH 06/32] sched: Add
 task_struct->faults_disabled_mapping

On Fri, May 26, 2023 at 02:05:26AM +0200, Andreas Grünbacher wrote:
> Oh, it's just that gfs2 uses one dlm lock per inode to control access
> to that inode. In the code, this is called the "inode glock" ---
> glocks being an abstraction above dlm locks --- but it boils down to
> dlm locks in the end. An additional layer of locking will only work
> correctly if all cluster nodes use the new locks consistently, so old
> cluster nodes will become incompatible. Those kinds of changes are
> hard.
> 
> But the additional lock taking would also hurt performance, forever,
> and I'd really like to avoid taking that hit.
> 
> It may not be obvious to everyone, but allowing page faults during
> reads and writes (i.e., while holding dlm locks) can lead to
> distributed deadlocks. We cannot just pretend to be a local
> filesystem.

Ah, gotcha. Same basic issue then, dio -> page fault means you're taking
two DLM locks simulateously, so without lock ordering you get deadlock.

So that means promoting this to the VFS won't be be useful to you
(because the VFS lock will be a local lock, not your DLM lock).

Do you have trylock() and a defined lock ordering you can check for or
glocks (inode number)? Here's the new and expanded commit message, in
case my approach is of interest to you:

commit 32858d0fb90b503c8c39e899ad5155e44639d3b5
Author: Kent Overstreet <kent.overstreet@...il.com>
Date:   Wed Oct 16 15:03:50 2019 -0400

    sched: Add task_struct->faults_disabled_mapping
    
    There has been a long standing page cache coherence bug with direct IO.
    This provides part of a mechanism to fix it, currently just used by
    bcachefs but potentially worth promoting to the VFS.
    
    Direct IO evicts the range of the pagecache being read or written to.
    
    For reads, we need dirty pages to be written to disk, so that the read
    doesn't return stale data. For writes, we need to evict that range of
    the pagecache so that it's not stale after the write completes.
    
    However, without a locking mechanism to prevent those pages from being
    re-added to the pagecache - by a buffered read or page fault - page
    cache inconsistency is still possible.
    
    This isn't necessarily just an issue for userspace when they're playing
    games; filesystems may hang arbitrary state off the pagecache, and so
    page cache inconsistency may cause real filesystem bugs, depending on
    the filesystem. This is less of an issue for iomap based filesystems,
    but e.g. buffer heads caches disk block mappings (!) and attaches them
    to the pagecache, and bcachefs attaches disk reservations to pagecache
    pages.
    
    This issue has been hard to fix, because
     - we need to add a lock (henceforth calld pagecache_add_lock), which
       would be held for the duration of the direct IO
     - page faults add pages to the page cache, thus need to take the same
       lock
     - dio -> gup -> page fault thus can deadlock
    
    And we cannot enforce a lock ordering with this lock, since userspace
    will be controlling the lock ordering (via the fd and buffer arguments
    to direct IOs), so we need a different method of deadlock avoidance.
    
    We need to tell the page fault handler that we're already holding a
    pagecache_add_lock, and since plumbing it through the entire gup() path
    would be highly impractical this adds a field to task_struct.
    
    Then the full method is:
     - in the dio path, when we take first pagecache_add_lock, note the
       mapping in task_struct
     - in the page fault handler, if faults_disabled_mapping is set, we
       check if it's the same mapping as the one taking a page fault for,
       and if so return an error.
    
       Then we check lock ordering: if there's a lock ordering violation and
       trylock fails, we'll have to cycle the locks and return an error that
       tells the DIO path to retry: faults_disabled_mapping is also used for
       signalling "locks were dropped, please retry".
    
    Also relevant to this patch: mapping->invalidate_lock.
    mapping->invalidate_lock provides most of the required semantics - it's
    used by truncate/fallocate to block pages being added to the pagecache.
    However, since it's a rwsem, direct IOs would need to take the write
    side in order to block page cache adds, and would then be exclusive with
    each other - we'll need a new type of lock to pair with this approach.
    
    Signed-off-by: Kent Overstreet <kent.overstreet@...ux.dev>
    Cc: Jan Kara <jack@...e.cz>
    Cc: Darrick J. Wong <djwong@...nel.org>
    Cc: linux-fsdevel@...r.kernel.org
    Cc: Andreas Grünbacher <andreas.gruenbacher@...il.com>

Powered by blists - more mailing lists