Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Tue, 3 Oct 2023 12:52:24 +0200
From:   Jan Kara <jack@suse.cz>
To:     "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc:     Jan Kara <jack@suse.com>, linux-ext4@vger.kernel.org,
        linux-fsdevel@vger.kernel.org,
        "Fabio M . De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [PATCH 02/10] ext2: Convert ext2_check_page to ext2_check_folio
Message-ID: <20231003105224.3j47fjxpiudwvupn@quack3>
References: <20230921200746.3303942-1-willy@infradead.org>
 <20230921200746.3303942-2-willy@infradead.org>
 <20231003104017.ohuyl3fv2mobif5u@quack3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231003104017.ohuyl3fv2mobif5u@quack3>
Precedence: bulk

On Tue 03-10-23 12:40:17, Jan Kara wrote:
> On Thu 21-09-23 21:07:39, Matthew Wilcox (Oracle) wrote:
> > Support in this function for large folios is limited to supporting
> > filesystems with block size > PAGE_SIZE.  This new functionality will only
> > be supported on machines without HIGHMEM, so the problem of kmap_local
> > only being able to map a single page in the folio can be ignored.
> > We will not use large folios for ext2 directories on HIGHMEM machines.
> 
> OK, but can we perhaps enforce this with some checks & error messages
> instead of a silent failure? Like:
> 
> #ifdef CONFIG_HIGHMEM
> 	if (sb->s_blocksize > PAGE_SIZE)
> 		bail with error
> #endif
> 
> somewhere in ext2_fill_super()? Or maybe force allocation of lowmem pages
> when blocksize > PAGE_SIZE?
> 
> > @@ -195,9 +195,9 @@ static void *ext2_get_page(struct inode *dir, unsigned long n,
> >  
> >  	if (IS_ERR(folio))
> >  		return ERR_CAST(folio);
> > -	page_addr = kmap_local_folio(folio, n & (folio_nr_pages(folio) - 1));
> > +	page_addr = kmap_local_folio(folio, 0);

Oh, and I think this change breaks the code whenever we get back higher
order folio because the page_addr we get back is not for the page index
'n'.
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR