Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 12 Jan 2023 00:41:17 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Dave Chinner <david@fromorbit.com>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Christoph Hellwig <hch@infradead.org>,
        Andreas Gruenbacher <agruenba@redhat.com>,
        Dave Chinner <dchinner@redhat.com>,
        "Darrick J . Wong" <djwong@kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-ext4@vger.kernel.org, cluster-devel@redhat.com,
        Christoph Hellwig <hch@lst.de>
Subject: Re: [RFC v6 04/10] iomap: Add iomap_get_folio helper
Message-ID: <Y7/HrZCARD9zRvEe@infradead.org>
References: <20230108213305.GO1971568@dread.disaster.area>
 <20230108194034.1444764-1-agruenba@redhat.com>
 <20230108194034.1444764-5-agruenba@redhat.com>
 <20230109124642.1663842-1-agruenba@redhat.com>
 <Y70l9ZZXpERjPqFT@infradead.org>
 <Y71pWJ0JHwGrJ/iv@casper.infradead.org>
 <Y72DK9XuaJfN+ecj@infradead.org>
 <Y78PunroeYbv2qgH@casper.infradead.org>
 <20230111205241.GA360264@dread.disaster.area>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230111205241.GA360264@dread.disaster.area>
Precedence: bulk

On Thu, Jan 12, 2023 at 07:52:41AM +1100, Dave Chinner wrote:
> Exposing internal implementation details in the API
> that is supposed to abstract away the internal implementation
> details from users doesn't seem like a great idea to me.

While I somewhat agree with the concern of leaking the xarray
internals, at least they are clearly documented and easy to find..

> Exactly what are we trying to fix here?  Do we really need to punch
> a hole through the abstraction layers like this just to remove half
> a dozen lines of -slow path- context specific error handling from a
> single caller?

While the current code (which is getting worse with your fix) leaks
completely undocumented and internal decision making.  So what this
fixes is a real leak of internatal logic inside of __filemap_get_folio
into the callers.

So as far as I'm concerned we really do need the helper, and anyone
using !GFP_CREATE or FGP_NOWAIT should be using it.  The only question
to me is if exposing the xarray internals is worth it vs the
less optimal calling conventions of needing an extra argument for
the error code.