linux-kernel - Re: [RFC][PATCH] cifs: Fix cifs_writepages

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]

Message-ID: <Y/kFnhUM5hjWM2Ae@casper.infradead.org>
Date:   Fri, 24 Feb 2023 18:44:46 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     David Howells <dhowells@...hat.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Steve French <stfrench@...rosoft.com>,
        Vishal Moola <vishal.moola@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, Paulo Alcantara <pc@....nz>,
        Huang Ying <ying.huang@...el.com>,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Xin Hao <xhao@...ux.alibaba.com>, linux-mm@...ck.org,
        mm-commits@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] cifs: Fix cifs_writepages_region()

On Fri, Feb 24, 2023 at 05:15:41PM +0000, David Howells wrote:
> Matthew Wilcox <willy@...radead.org> wrote:
> 
> > Why are you doing it this way?  What's wrong with using
> > write_cache_pages() to push all the contiguous dirty folios into a single
> > I/O object, submitting it when the folios turn out not to be contiguous,
> > or when we run out of a batch?
> > 
> > You've written an awful lot of code here and it's a different model from
> > every other filesystem.  Why is it better?
> 
> Because write_cache_pages():
> 
>  (1) Takes no account of fscache.  I can't just build knowledge of PG_fscache
>      into it because PG_private_2 may be used differently by other filesystems
>      (btrfs, for example).  (I'm also trying to phase out the use of
>      PG_private_2 and instead uses PG_writeback to cover both and the
>      difference will be recorded elsewhere - but that's not there yet).

I don't understand why waiting for fscache here is even the right thing
to do.  The folio is dirty and needs to be written back to the
server.  Why should beginning that write wait for the current write
to the fscache to complete?

>  (2) Calls ->writepage() individually for each folio - which is excessive.  In
>      AFS's implementation, we locate the first folio, then race through the
>      following folios without ever waiting until we hit something that's
>      locked or a gap and then stop and submit.
> 
>      write_cache_pages(), otoh, calls us with the next folio already undirtied
>      and set for writeback when we find out that we don't want it yet.

I think you're so convinced that your way is better that you don't see
what write_cache_pages() is actually doing.  Yes, the writepage callback
is called once for each folio, but that doesn't actually submit a write.
Instead, they're accumulated into the equivalent of a wdata and the
wdata is submitted when there's a discontiguity or we've accumulated
enough to satisfy the constraints of the wbc.

>  (3) Skips over holes, but at some point in the future we're going to need to
>      schedule adjacent clean pages (before and after) for writeback too to
>      handle transport compression and fscache updates if the granule size for
>      either is larger than the folio size.

Then we'll need it for other filesystems too, so should enhance
write_cache_pages().

> It might be better to take what's in cifs, generalise it and replace
> write_cache_pages() with it, then have a "->submit_write()" aop that takes an
> ITER_XARRAY iterator to write from.

->writepages _is_ ->submit_write.  Should write_cache_pages() be better?
Maybe!  But it works a whole lot better than what AFS was doing and
you've now ladelled into cifs.