linux-kernel - Re: [RFC] Bypass filesystems for reading cached pages

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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 20 Jun 2020 09:19:37 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] Bypass filesystems for reading cached pages

On Fri, Jun 19, 2020 at 6:52 PM Matthew Wilcox <willy@...radead.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.