From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E197EC7EE23 for ; Fri, 24 Feb 2023 18:45:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2AD456B0071; Fri, 24 Feb 2023 13:45:07 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 285416B0073; Fri, 24 Feb 2023 13:45:07 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 14D796B0074; Fri, 24 Feb 2023 13:45:07 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 044AD6B0071 for ; Fri, 24 Feb 2023 13:45:07 -0500 (EST) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 9AAEBC0A76 for ; Fri, 24 Feb 2023 18:45:06 +0000 (UTC) X-FDA: 80503062612.20.4580BF6 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf15.hostedemail.com (Postfix) with ESMTP id DB342A0011 for ; Fri, 24 Feb 2023 18:45:03 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=jplJ5yxA; spf=none (imf15.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1677264304; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=KrxxPv4XvLx2RtKsee2RWloWUMeXGZheVNKJtMabags=; b=RUhOgI0QWUVuDTf3jIgXgulyKOlcqAhHPoTrfFlA42CGFvqwNDBSO2xsDxH+lYxtvc88Dd 1IEN9yvUKmwViBQuh5O4EY9XeaU+C8N/hJylx05RVqtlChnDI/NYtkox5i00BD6pPDB+Z+ gLM1uMfOsd9UrIttBBalD8kwX2uOH6o= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=jplJ5yxA; spf=none (imf15.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1677264304; a=rsa-sha256; cv=none; b=bRkxTY/W8cZvCH3wYtXvhFZvvpi4HuTujZo2MrdvGEkd3rVXLMSN8CfjE6MXc5kvCxTgDV Lve8inWxBjn6yx4UXKR3GwJgTgaSc/h96Zbx+aiTQPPDccnYycWo1KKNzrmDCZ6R5iqu38 J6n9HU4oXT7EYOW9vV2gzAFfA1wJu/U= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=KrxxPv4XvLx2RtKsee2RWloWUMeXGZheVNKJtMabags=; b=jplJ5yxAebN1rP7bepdDpvim3J qwaEDxAqa8jGhLU2xsBmcOLPsbTQfx0yfmyHY2nR4yRAp1aZId0mjEaNFsLjlJ7M8x7dMElLs1fhR tU1bEh8mTbx15omA53EmgduU5NuQf1MMFkHRoCp1T+6o8f3zZDL+3GioKL3ld30g3Uv+gVIb/Bk4r q+VMOAQIfnKSLm9UrKdw09y0Lxv7qp3eiwvy2UKncyVP+YJpgajz+I1Nb96uJ3G6Yw9sWAn+dieUo tOlYBSsJIoBYTDRsDyJuRVkFXKB7IEi8T7MEVT2ZiApqSgQAY6GvtAWRcpAq7TjcViYBYtSmS04gW ClPAldHw==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1pVd3m-00FU4d-25; Fri, 24 Feb 2023 18:44:46 +0000 Date: Fri, 24 Feb 2023 18:44:46 +0000 From: Matthew Wilcox To: David Howells Cc: Linus Torvalds , Steve French , Vishal Moola , Andrew Morton , Jan Kara , Paulo Alcantara , Huang Ying , Baolin Wang , Xin Hao , linux-mm@kvack.org, mm-commits@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC][PATCH] cifs: Fix cifs_writepages_region() Message-ID: References: <2134430.1677240738@warthog.procyon.org.uk> <2009825.1677229488@warthog.procyon.org.uk> <20230220135225.91b0f28344c01d5306c31230@linux-foundation.org> <2213409.1677249075@warthog.procyon.org.uk> <2385089.1677258941@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2385089.1677258941@warthog.procyon.org.uk> X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: DB342A0011 X-Stat-Signature: m1rsnsnkzr6oqd4gyg8rrnqgrruybgau X-HE-Tag: 1677264303-521947 X-HE-Meta: U2FsdGVkX1++EPvWeRgeqn1vOQ51ujSomHw22K36D/mW704QF5mrBwfsBOwuUNV/jrkqs2RrEghPJKdlmCU+WQweqPnIGA4SuS38LzsQ/bc2Zxjc1YUfFA+gRyZYAnC6pbIrBnhpd8F+nTwr9lZEbyjSlRz6kHtMZLw0Ws50Yud0a9TH830hHwVBT/YdgVW3eW2NN+BnAARwWRoUTwBJqdmyiceIh/VZ212YEBte/MaT1dV8zA8+OpDNXPsEjgTR6sepe/YAO7wW2EQ8TdDUeNyiB2nx7kGqxnaA63bmGFUi5Arvj8E/6hNAaB+25W1iBU+H0drB3b6Wj6kwrvZ9KvRiauxa/+4fFs68fEvR/82blRR9RoSAG64UnSb6ddS5yXFf+fTX050xaQIEJBgdtJYrAd/8KYHqfEik5GZVQphwAHkPi/Qi7pNSoodTtmLS6LDNZ1uXgTd3HP8j5TudrH++c58tf5ulGKNzroVxnP8cDaiYUR61hDCIWD52VSK7R1aLdZPU/DhLE3q7VsscOTMg5njtM0PkOiODJq/hQvSxHenOa1wsqEWTGenvbo/Cdl1sJdH4qMX6fMZaO1xumtpnusGlVi2cQ4rtIuE26h+QZqeW2BkYLSmCHeJXaV78xu/C3c2MwYOs50tzlwmWW1a8V9/iTdiQMg+0lJl4RlwTrOFN9cGsNM5yl91nUwBdCmN0sa0Jr3Ad4wPtZopvEg8e3odKGZ7IB3ItMYcPCJftSYk+3V38dUoScwDyeAMC9S75RKlRIjyCl8FwJrRHvHqQnO/PwKZxznzlqG23R8+Wpe36eazQdjVInizbuJ49j+p7Gaikt0aOEGkLViXVDfOVU7G4CI5QBft3vUx8RZqSq++53dQJ+4WNG5Tfav+VJ1qjreuRSh+XI3JUPPRH4P5wciA0gULynHxxet19YGQyfkRXJr+3SENTzH2z+qf+p65ISsC8cGTTivsvZqF sqUfHTLx AHdNdStkl0tgU09rOGyIag23KEwBRCBwQh+zkChMz+Ghdch9e4wydbKaPlu56HXtYrbpGxATnsBmggSV0yY5y9BIhC7QNcoIH9A6ttcQP6JJYp5/p+RLkEdAZCitIFizd2FcR+4Hsg/FODtJWvfQrTm4KrvnUWpQQOmpu7rIgxgRy6Gr4cB7FT0Z20uksWZcRbQs2KgndxBbKU4f75IeRT5R6Y6E0tsAjgLMID92iy17+AEZ/mEeBn6sXq0/76Wq3ydPF6rNrI/8S4P7I0cydsIfXFJDmfypBvLGDdHM0oFPyGi6dO3/n17MVwkba36OFIPxD86uwdQUsM2w= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Feb 24, 2023 at 05:15:41PM +0000, David Howells wrote: > Matthew Wilcox 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.