mbox series

[RFC,v2,0/5] remove page_endio()

Message ID 20230322135013.197076-1-p.raghav@samsung.com
Headers
Series remove page_endio() |

Message

Pankaj Raghav March 22, 2023, 1:50 p.m. UTC 
  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.

[1] https://lore.kernel.org/linux-mm/ZBHcl8Pz2ULb4RGD@infradead.org/
[2] https://lore.kernel.org/linux-mm/ZBSH6Uq6IIXON%2Frh@casper.infradead.org/

Pankaj Raghav (5):
  zram: remove zram_page_end_io function
  orangefs: use folios in orangefs_readahead
  mpage: split bi_end_io callback for reads and writes
  mpage: use folios in bio end_io handler
  filemap: remove page_endio()

 drivers/block/zram/zram_drv.c | 13 +----------
 fs/mpage.c                    | 44 ++++++++++++++++++++++++++++-------
 fs/orangefs/inode.c           |  9 +++----
 include/linux/pagemap.h       |  2 --
 mm/filemap.c                  | 30 ------------------------
 5 files changed, 42 insertions(+), 56 deletions(-)

Comments

Matthew Wilcox March 22, 2023, 7:09 p.m. UTC | #1 
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.
Mike Marshall March 23, 2023, 2:30 p.m. UTC | #2 
I have tested this patch on orangefs on top of 6.3.0-rc3, no
regressions.

It is very easy to build a single host orangefs test system on
a vm. There are instructions in orangefs.rst, and also I'd
be glad to help make them better...

-Mike

On Wed, Mar 22, 2023 at 9:50 AM Pankaj Raghav <p.raghav@samsung.com> 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.
>
> [1] https://lore.kernel.org/linux-mm/ZBHcl8Pz2ULb4RGD@infradead.org/
> [2] https://lore.kernel.org/linux-mm/ZBSH6Uq6IIXON%2Frh@casper.infradead.org/
>
> Pankaj Raghav (5):
>   zram: remove zram_page_end_io function
>   orangefs: use folios in orangefs_readahead
>   mpage: split bi_end_io callback for reads and writes
>   mpage: use folios in bio end_io handler
>   filemap: remove page_endio()
>
>  drivers/block/zram/zram_drv.c | 13 +----------
>  fs/mpage.c                    | 44 ++++++++++++++++++++++++++++-------
>  fs/orangefs/inode.c           |  9 +++----
>  include/linux/pagemap.h       |  2 --
>  mm/filemap.c                  | 30 ------------------------
>  5 files changed, 42 insertions(+), 56 deletions(-)
>
> --
> 2.34.1
>
Pankaj Raghav March 23, 2023, 3 p.m. UTC | #3 
>> 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.
> 
Got 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.

Thanks for the explanation. I think found the series you are referring here.

https://lore.kernel.org/linux-mm/20220527155036.524743-1-willy@infradead.org/#t

I see orangefs is still setting the error flag in orangefs_read_folio(), so
it should be removed at some point?

I also changed mpage to **not set** the error flag in the read path. It does beg
the question whether block_read_full_folio() and iomap_finish_folio_read() should
also follow the suit.

--
Pankaj
Matthew Wilcox March 23, 2023, 3:33 p.m. UTC | #4 
On Thu, Mar 23, 2023 at 04:00:37PM +0100, Pankaj Raghav wrote:
> > 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.
> 
> Thanks for the explanation. I think found the series you are referring here.
> 
> https://lore.kernel.org/linux-mm/20220527155036.524743-1-willy@infradead.org/#t
> 
> I see orangefs is still setting the error flag in orangefs_read_folio(), so
> it should be removed at some point?

Yes, OrangeFS only sets the error flag, it never checks it, so it never
needs to set it.

> I also changed mpage to **not set** the error flag in the read path. It does beg
> the question whether block_read_full_folio() and iomap_finish_folio_read() should
> also follow the suit.

Wrong.  mpage is used by filesystems which *DO* check the error flag.
You can't remove it being set until they're fixed to not check it.
Pankaj Raghav March 23, 2023, 4:16 p.m. UTC | #5 
>> I also changed mpage to **not set** the error flag in the read path. It does beg
>> the question whether block_read_full_folio() and iomap_finish_folio_read() should
>> also follow the suit.
> 
> Wrong.  mpage is used by filesystems which *DO* check the error flag.
> You can't remove it being set until they're fixed to not check it.
Got it. I think after your explanation on the Error flag, it makes sense why mpage
needs to set the error flag, for now. I will change it as a part of the next version
along with the other comment on Patch 4.
Pankaj Raghav March 23, 2023, 4:22 p.m. UTC | #6 
On 2023-03-23 15:30, Mike Marshall wrote:
> I have tested this patch on orangefs on top of 6.3.0-rc3, no
> regressions.
> 

Perfect. Thanks a lot. Could I get a Tested-by from you for the
current change?

> It is very easy to build a single host orangefs test system on
> a vm. There are instructions in orangefs.rst, and also I'd
> be glad to help make them better...
> 
Sounds good. I can do it next time if I change the current code.