Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200619155036.GZ8681@bombadil.infradead.org>
In-Reply-To: <20200619155036.GZ8681@bombadil.infradead.org>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Sat, 20 Jun 2020 09:19:37 +0300
Message-ID: <CAOQ4uxjy6JTAQqvK9pc+xNDfzGQ3ACefTrySXtKb_OcAYQrdzw@mail.gmail.com>
Subject: Re: [RFC] Bypass filesystems for reading cached pages
To:     Matthew Wilcox <willy@infradead.org>
Cc:     linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>,
        Andreas Gruenbacher <agruenba@redhat.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 19, 2020 at 6:52 PM Matthew Wilcox <willy@infradead.org> wrote:
>
>
> This patch lifts the IOCB_CACHED idea expressed by Andreas to the VFS.
> The advantage of this patch is that we can avoid taking any filesystem
> lock, as long as the pages being accessed are in the cache (and we don't
> need to readahead any pages into the cache).  We also avoid an indirect
> function call in these cases.
>
> I'm sure reusing the name call_read_iter() is the wrong way to go about
> this, but renaming all the callers would make this a larger patch.
> I'm happy to do it if something like this stands a chance of being
> accepted.
>
> Compared to Andreas' patch, I removed the -ECANCELED return value.
> We can happily return 0 from generic_file_buffered_read() and it's less
> code to handle that.  I bypass the attempt to read from the page cache
> for O_DIRECT reads, and for inodes which have no cached pages.  Hopefully
> this will avoid calling generic_file_buffered_read() for drivers which
> implement read_iter() (although I haven't audited them all to check that
>
> This could go horribly wrong if filesystems rely on doing work in their
> ->read_iter implementation (eg checking i_size after acquiring their
> lock) instead of keeping the page cache uptodate.  On the other hand,
> the ->map_pages() method is already called without locks, so filesystems
> should already be prepared for this.
>

XFS is taking i_rwsem lock in read_iter() for a surprising reason:
https://lore.kernel.org/linux-xfs/CAOQ4uxjpqDQP2AKA8Hrt4jDC65cTo4QdYDOKFE-C3cLxBBa6pQ@mail.gmail.com/
In that post I claim that ocfs2 and cifs also do some work in read_iter().
I didn't go back to check what, but it sounds like cache coherence among
nodes.

So filesystems will need to opt-in to this behavior.

I wonder if we should make this behavior also opt-in by userspace,
for example, RWF_OPPORTUNISTIC_CACHED.

Because if I am not mistaken, even though this change has a potential
to improve many workloads, it may also degrade some workloads in cases
where case readahead is not properly tuned. Imagine reading a large file
and getting only a few pages worth of data read on every syscall.
Or did I misunderstand your patch's behavior in that case?

Another up side of user opt-in flag - it can be used to mitigate the objection
of XFS developers against changing the "atomic write vs. read" behavior.
New flag - no commitment to an XFS specific behavior that nobody knows
if any application out there relies on.

Thanks,
Amir.