Message-ID: <2872796.VdNmn5OnKV@opensuse>
Date:   Tue, 26 Jul 2022 18:21:49 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Jan Kara <jack@...e.cz>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Pali Rohár <pali@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Muchun Song <songmuchun@...edance.com>,
        linux-kernel@...r.kernel.org, Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RFC PATCH] fs/isofs: Replace kmap() with kmap_local_page()

On martedì 26 luglio 2022 16:50:24 CEST Jan Kara wrote:
> On Mon 25-07-22 18:23:31, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page().
> > 
> > There are two main problems with kmap(): (1) It comes with an overhead 
as
> > mapping space is restricted and protected by a global lock for
> > synchronization and (2) it also requires global TLB invalidation when 
the
> > kmap’s pool wraps and it might block when the mapping space is fully
> > utilized until a slot becomes available.
> > 
> > With kmap_local_page() the mappings are per thread, CPU local, can take
> > page faults, and can be called from any context (including interrupts).
> > Tasks can be preempted and, when scheduled to run again, the kernel
> > virtual addresses are restored and still valid. It is faster than 
kmap()
> > in kernels with HIGHMEM enabled.
> > 
> > Since kmap_local_page() can be safely used in compress.c, it should be
> > called everywhere instead of kmap().
> > 
> > Therefore, replace kmap() with kmap_local_page() in compress.c. Where 
it
> > is needed, use memzero_page() instead of open coding kmap_local_page()
> > plus memset() to fill the pages with zeros. Delete the redundant
> > flush_dcache_page() in the two call sites of memzero_page().
> > 
> > This is an RFC because these changes have not been tested (tests are
> > welcome!), therefore I'm not entirely sure whether these conversions 
work
> > properly. I'd like to hear comments from more experienced developers
> > before sending a real patch. Suggestions about how to run tests would
> > also be much appreciated.
> > 
> > Suggested-by: Ira Weiny <ira.weiny@...el.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> 
> What you propose makes sense to me. But the lack of testing is 
troublesome.
> You can always at least test your patch without highmem on isofs image 
with
> compression (mkisofs seems to support creating such filesystems).

I understand your concerns about the lack of testing. I'll find a way to do 
those tests on a QEMU/KVM x86_32 VM with a kernel with HIGHMEM64GB enabled. 
(this is what I did for other filesystem, Btrfs to start with).

I have no experience with this particular fs and saw that xfstests have no 
special tests for isofs. However, you are talking about using mkisofs and I 
suppose it will be enough for this purpose.

> Even that
> would detect a bug in your patch ;) - see below.
> 
> > diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> > index 95a19f25d61c..a1562124bb91 100644
> > --- a/fs/isofs/compress.c
> > +++ b/fs/isofs/compress.c
> > @@ -120,8 +119,7 @@ static loff_t zisofs_uncompress_block(struct inode 
*inode, loff_t block_start,
> >  	       zerr != Z_STREAM_END) {
> >  		if (!stream.avail_out) {
> >  			if (pages[curpage]) {
> > -				stream.next_out = 
page_address(pages[curpage])
> > -						+ poffset;
> > +				stream.next_out = 
kmap_local_page(pages[curpage] + poffset);
> 
> This is wrong. Most importantly because you need to add 'poffset' to the
> final address provided by kmap_local_page(), not to the struct page 
pointer.

Sorry, this is only a typo.

I've already made dozens of conversions to kmap_local_page(), so I know 
that I should add the offset to the kernel virtual address: 

stream.next_out = kmap_local_page(pages[curpage]) + poffset;

> 
> Secondly please wrap the line to fit into 80 chars.
> 
> >  				stream.avail_out = PAGE_SIZE - 
poffset;
> >  				poffset = 0;
> >  			} else {
> > @@ -170,6 +168,12 @@ static loff_t zisofs_uncompress_block(struct inode 
*inode, loff_t block_start,
> >  			}
> >  		}
> >  
> > +		if (stream.next_out)
> > +			if (stream.next_out != (char 
*)zisofs_sink_page) {
> > +				kunmap_local(stream.next_out);
> > +				stream.next_out = NULL;
> > +			}
> > +
> 
> This looks buggy as well. If we mapped page above, we'll unmap it here 
even
> if stream.avail_out > 0 and we want to still write to it. I think you
> should unmap the page here only if stream.avail_out == 0 and we are going
> to switch to the next page...

I need to look carefully at this because I'm not 100% sure of what you are 
talking about. At this moment I cannot look at the code, however I suppose 
that when I can, I'll have no problems to figure out what I missed. 

> >  		if (!stream.avail_out) {
> >  			/* This page completed */
> >  			if (pages[curpage]) {
> > @@ -183,6 +187,9 @@ static loff_t zisofs_uncompress_block(struct inode 
*inode, loff_t block_start,
> >  	}
> >  inflate_out:
> >  	zlib_inflateEnd(&stream);
> > +	if (stream.next_out)
> > +		if (stream.next_out != (char *)zisofs_sink_page)
> > +			kunmap_local(stream.next_out);
> 
> This is correct but I'd simplify it to:
> 
> 	if (stream.next_out && stream.next_out != (char 
*)zisofs_sink_page)
> 		kunmap_local(stream.next_out);

It's shorter and more readable. I'll rewrite the code as you suggest.

Thanks,

Fabio

> 								
Honza
> -- 
> Jan Kara <jack@...e.com>
> SUSE Labs, CR
> 

Powered by blists - more mailing lists