Re: [PATCH 0/8] Convert reiserfs from b_page to b_folio

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On Sun, 18 Dec 2022, 00:40 Ira Weiny, <ira.weiny@intel.com> wrote:

On Sat, Dec 17, 2022 at 09:43:11PM +0100, Fabio M. De Francesco wrote:
> On venerdì 16 dicembre 2022 21:53:39 CET Matthew Wilcox (Oracle) wrote:
> > > These patches apply on top of
> > > https://lore.kernel.org/linux-fsdevel/20221215214402.3522366-1-willy@infradead
> > > .org/
> > > 
> > > The non-trivial ones mostly revolve around uses of kmap()/kmap_atomic(),
> > > so review from the experts on those would be welcome.
> >
> > I took a quick look at your conversions and they made me recall that 
> > months ago you converted to kmap_local_folio() a previous conversion from 
> > kmap() to kmap_local_page() in ext2_get_page(): commit 37ce0b319b287 
> > ("ext2: Use a folio in ext2_get_page()").
> >
> > So I just saw kmap_local_folio() again. Unfortunately, because of my 
> > inexperience, I'm not able to understand on my own why we should prefer 
> > the use of this function instead of kmap_local_page().
> > 
> > Can you please tell me why and when we should prefer kmap_local_folio() in 
> > those cases too where kmap_local_page() can work properly? I'm asking
> > because these days I'm converting other *_get_page() from kmap()
> > (including the series to fs/ufs that I sent today).

> Fabio kmap_local_folio() works on folios and handles determining which page 
> in the folio is the correct one to map.

Ira, I understand that pages are parts of folios and that, for mapping, we 
need to determine the right page to map. Correct?

I think that I was not able to ask my question clearly. Please, let me rework 
with other words how I went to my question and reword the question itself...

It all started when months ago I saw a patch from Matthew about the conversion 
from kmap_local_page() to kmap_local_folio() in ext2_get_page().

Here I wanted to comment on the xfstests failures but, when I read patch 2/8 
of this series and saw kmap() converted to kmap_local_folio(), I thought to 
also use this opportunity to ask about why and when kmap_local_folio() should 
be preferred over kmap_local_page().

Obviously, I have nothing against these conversions. I would only like to 
understand what are the reasons behind the preference for the folio function.

Mine is a general question about design, necessity, opportunity: what were the 
reasons why, in the above-mentioned cases, the use of kmap_local_folio() has 
been preferred over kmap_local_page()? 

I saw that this series is about converting from b_page to b_folio, therefore 
kmap_local_folio() is the obvious choice here.

But my mind went back again to ext2_get_page :-)

It looks to me that ext2_get_page() was working properly with 
kmap_local_page() (since you made the conversion from kmap()). Therefore I 
could not understand why it is preferred to call read_mapping_folio() to get a 
folio and then map a page of that folio with kmap_local_folio(). 

I used to think that read_mapping_page() + kmap_local_page() was all we 
needed. ATM I have not enough knowledge of VFS/filesystems to understand on my 
own what we gain from the other way to local map pages.    

I hope to having been clearer this time...
Can you and/or Matthew please say some words about this? 

> AFAICT (from a quick grep) fs/ufs does not have folio support.

I was not specifically talking about the fs/ufs conversion. Other conversions  
are in my queue (e.g., fs/sysv is next according to Al's suggestions, and in 
January others will be added to the same queue).

> I am sure Mathew would appreciate converting fs/ufs to folios if you have 
> the time and want to figure it out.

About a year ago Matthew provided me with precious help when I was converting  
a Unisys driver from IDR to XArray, so I guess he would be helpful with this 
task too :-)

I'd really like to work on converting fs/ufs to folios but you know that I'll 
have enough time to work on other projects only starting by the end of 
January. 

AFAIK this task has mainly got to do with the conversions of the address space 
operations (correct?). I know too little to be able to estimate how much time 
it takes but I'm pretty sure it needs more than I currently can set aside.

Instead I could easily devolve the time it is needed for making the  
memcpy_{to|from}_folio() helpers you talked about in a patch of this series, 
unless you or Matthew prefer to do yourselves. Please let me know.

Thanks,

Fabio

> Ira
> 
> > If these all look
> > good to people, I can pass them off to Andrew for the 6.3 merge window.
> > 
> > Running xfstests against reiserfs gives me 313/701 failures before and
> > after this set of patches.
> 
> It has happened several times to me too. Some patches of mine have failures 
> from xfstests whose amounts and types don't change with or without my 
changes.
> 
> Several of them have already been merged. I guess that if they don't add 
> further failures everything is alright.
> 
> However, something is broken for sure... xfstests or the filesystems? :-/ 
> 
> Thanks,
> 
> Fabio
> 
> > I don't have a huge amount of confidence
> > that we're really getting good coverage from that test run!
> > 
> > Matthew Wilcox (Oracle) (8):
> >   reiserfs: use b_folio instead of b_page in some obvious cases
> >   reiserfs: use kmap_local_folio() in _get_block_create_0()
> >   reiserfs: Convert direct2indirect() to call folio_zero_range()
> >   reiserfs: Convert reiserfs_delete_item() to use kmap_local_folio()
> >   reiserfs: Convert do_journal_end() to use kmap_local_folio()
> >   reiserfs: Convert map_block_for_writepage() to use kmap_local_folio()
> >   reiserfs: Convert convert_tail_for_hole() to use folios
> >   reiserfs: Use flush_dcache_folio() in reiserfs_quota_write()
> > 
> >  fs/reiserfs/inode.c           | 73 +++++++++++++++++------------------
> >  fs/reiserfs/journal.c         | 12 +++---
> >  fs/reiserfs/prints.c          |  4 +-
> >  fs/reiserfs/stree.c           |  9 +++--
> >  fs/reiserfs/super.c           |  2 +-
> >  fs/reiserfs/tail_conversion.c | 19 ++++-----
> >  6 files changed, 59 insertions(+), 60 deletions(-)
> > 
> > --
> > 2.35.1
> 
> 
> 
>