Received-SPF: pass (google.com: domain of linux-kernel+bounces-95345-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
	Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
	Kingdom.
	Registered in England and Wales under Company Registration No. 3798903
From: David Howells <dhowells@redhat.com>
In-Reply-To: <ZelGX3vVlGfEZm8H@casper.infradead.org>
References: <ZelGX3vVlGfEZm8H@casper.infradead.org> <1668172.1709764777@warthog.procyon.org.uk>
To: Matthew Wilcox <willy@infradead.org>,
    Trond Myklebust <trond.myklebust@hammerspace.com>,
    Miklos Szeredi <miklos@szeredi.hu>, Christoph Hellwig <hch@lst.de>
Cc: dhowells@redhat.com, Andrew Morton <akpm@linux-foundation.org>,
    Alexander Viro <viro@zeniv.linux.org.uk>,
    Christian Brauner <brauner@kernel.org>,
    Jeff Layton <jlayton@kernel.org>, linux-mm@kvack.org,
    linux-fsdevel@vger.kernel.org, netfs@lists.linux.dev,
    v9fs@lists.linux.dev, linux-afs@lists.infradead.org,
    ceph-devel@vger.kernel.org, linux-cifs@vger.kernel.org,
    linux-nfs@vger.kernel.org, devel@lists.orangefs.org,
    linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] mm: Replace ->launder_folio() with flush and wait
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1831808.1709807788.1@warthog.procyon.org.uk>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 07 Mar 2024 10:36:28 +0000
Message-ID: <1831809.1709807788@warthog.procyon.org.uk>

Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Mar 06, 2024 at 10:39:37PM +0000, David Howells wrote:
> > Here's a patch to have a go at getting rid of ->launder_folio().  Sinc=
e it's
> > failable and cannot guarantee that pages in the range are removed, I'v=
e tried
> > to replace laundering with just flush-and-wait, dropping the folio loc=
k around
> > the I/O.
> =

> My sense is that ->launder_folio doesn't actually need to be replaced.
> =

> commit e3db7691e9f3dff3289f64e3d98583e28afe03db
> Author: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date:   Wed Jan 10 23:15:39 2007 -0800
> =

>     [PATCH] NFS: Fix race in nfs_release_page()
> =

>         NFS: Fix race in nfs_release_page()
> =

>         invalidate_inode_pages2() may find the dirty bit has been set on=
 a page
>         owing to the fact that the page may still be mapped after it was=
 locked.
>         Only after the call to unmap_mapping_range() are we sure that th=
e page
>         can no longer be dirtied.
>         In order to fix this, NFS has hooked the releasepage() method an=
d tries
>         to write the page out between the call to unmap_mapping_range() =
and the
>         call to remove_mapping(). This, however leads to deadlocks in th=
e page
>         reclaim code, where the page may be locked without holding a ref=
erence
>         to the inode or dentry.
> =

>         Fix is to add a new address_space_operation, launder_page(), whi=
ch will
>         attempt to write out a dirty page without releasing the page loc=
k.
> =

>     Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> =

> I don't understand why this couldn't've been solved by page_mkwrite.
> NFS did later add nfs_vm_page_mkwrite in July 2007, and maybe it's just
> not needed any more?  I haven't looked into it enough to make sure,
> but my belief is that we should be able to get rid of it.

Okay, it's slightly more complex than I thought - and I'm not sure all cal=
lers
are actually using it correctly.  There are some additional interesting ca=
ses
I've found, beyond the pre-/post-DIO case:

 (1) NFS relies on it to retry the write before stripping the pages in the
     case where a writeback error occurs.  I think this can probably be de=
alt
     with by sticking a filemap_fdatawrite() call before the invalidation.
     I'm not sure if this would incur the deadlock with the page reclaim c=
ode
     of which Trond speaks.

 (2) invalidate_inode_pages2() is used in some places to effect invalidati=
on
     of the pagecache in the case where the server tells us that a third p=
arty
     modified the server copy of a file.  What the right behaviour should =
be
     here, I'm not sure, but at the moment, any dirty data will get launde=
red
     back to the server.  Possibly it should be simply invalidated locally=
 or
     the user asked how they want to handle the divergence.

     Some filesystems use invalidate_remote_inode() instead which seems to
     leave the dirty pages in place locally.

     If it is desirous to save the dirty data, then filemap_fdatawrite()
     could be deployed before invalidating the pages.

 (3) Fuse uses invalidate_inode_pages2() in fuse_do_setattr() to strip all=
 the
     pages from an inode that has had its size changed, laundering any pag=
e
     that's still dirty.  This would seem to be excessive, but maybe Miklo=
s
     had a reason for doing it that way.

There are some places that should perhaps be using kiocb_invalidate_pages(=
)
and kiocb_invalidate_post_direct_write() instead - assuming Christoph has =
no
objection to the latter function being exported.

David