| Message ID | 20230322135013.197076-1-p.raghav@samsung.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove page_endio() | expand |
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.
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 >
>> 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
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.
>> 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.
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.