Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 19:09:46 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Pankaj Raghav <p.raghav@samsung.com>
Cc:     senozhatsky@chromium.org, viro@zeniv.linux.org.uk, axboe@kernel.dk,
        brauner@kernel.org, akpm@linux-foundation.org, minchan@kernel.org,
        hubcap@omnibond.com, martin@omnibond.com, mcgrof@kernel.org,
        devel@lists.orangefs.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-block@vger.kernel.org, gost.dev@samsung.com
Subject: Re: [RFC v2 0/5] remove page_endio()
Message-ID: <ZBtSevjWLybE6S07@casper.infradead.org>
References: <CGME20230322135015eucas1p2ff980e76159f0ceef7bf66934580bd6c@eucas1p2.samsung.com>
 <20230322135013.197076-1-p.raghav@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230322135013.197076-1-p.raghav@samsung.com>
Precedence: bulk

On Wed, Mar 22, 2023 at 02:50:08PM +0100, Pankaj Raghav wrote:
> It was decided to remove the page_endio() as per the previous RFC
> discussion[1] of this series and move that functionality into the caller
> itself. One of the side benefit of doing that is the callers have been
> modified to directly work on folios as page_endio() already worked on
> folios.
> 
> mpage changes were tested with a simple boot testing. zram and orangefs is
> only build tested. No functional changes were introduced as a part of
> this AFAIK.
> 
> Open questions:
> - Willy pointed out that the calls to folio_set_error() and
>   folio_clear_uptodate() are not needed anymore in the read path when an
>   error happens[2]. I still don't understand 100% why they aren't needed
>   anymore as I see those functions are still called in iomap. It will be
>   good to put that rationale as a part of the commit message.

page_endio() was generic.  It needed to handle a lot of cases.  When it's
being inlined into various completion routines, we know which cases we
need to handle and can omit all the cases which we don't.

We know the uptodate flag is clear.  If the uptodate flag is set,
we don't call the filesystem's read path.  Since we know it's clear,
we don't need to clear it.

We don't need to set the error flag.  Only some filesystems still use
the error flag, and orangefs isn't one of them.  I'd like to get rid
of the error flag altogether, and I've sent patches in the past which
get us a lot closer to that desired outcome.  Not sure we're there yet.
Regardless, generic code doesn't check the error flag.