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: <Y/jhwuTCaOgOTLp2@casper.infradead.org>
References: <Y/jhwuTCaOgOTLp2@casper.infradead.org> <2134430.1677240738@warthog.procyon.org.uk> <2009825.1677229488@warthog.procyon.org.uk> <CAHk-=whAAOVBrzwb2uMjCmdRrtudGesYj0tuqdUgi8X_gbw1jw@mail.gmail.com> <20230220135225.91b0f28344c01d5306c31230@linux-foundation.org> <2213409.1677249075@warthog.procyon.org.uk>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     dhowells@redhat.com,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Steve French <stfrench@microsoft.com>,
        Vishal Moola <vishal.moola@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jan Kara <jack@suse.cz>, Paulo Alcantara <pc@cjr.nz>,
        Huang Ying <ying.huang@intel.com>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Xin Hao <xhao@linux.alibaba.com>, linux-mm@kvack.org,
        mm-commits@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC][PATCH] cifs: Fix cifs_writepages_region()
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2385088.1677258941.1@warthog.procyon.org.uk>
Date:   Fri, 24 Feb 2023 17:15:41 +0000
Message-ID: <2385089.1677258941@warthog.procyon.org.uk>
Precedence: bulk

Matthew Wilcox <willy@infradead.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).

 (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.

 (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.

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.

David