linux-kernel - Re: [PATCH 03/13] filemap: align the index to mapping_min

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 27 Feb 2024 17:36:09 +0100
From: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: Matthew Wilcox <willy@...radead.org>, linux-xfs@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, david@...morbit.com, 
	chandan.babu@...cle.com, akpm@...ux-foundation.org, mcgrof@...nel.org, ziy@...dia.com, 
	hare@...e.de, djwong@...nel.org, gost.dev@...sung.com, linux-mm@...ck.org, 
	Pankaj Raghav <p.raghav@...sung.com>
Subject: Re: [PATCH 03/13] filemap: align the index to mapping_min_order in
 the page cache

On Tue, Feb 27, 2024 at 11:22:24AM -0500, Kent Overstreet wrote:
> On Tue, Feb 27, 2024 at 11:06:37AM +0100, Pankaj Raghav (Samsung) wrote:
> > On Mon, Feb 26, 2024 at 02:40:42PM +0000, Matthew Wilcox wrote:
> > > On Mon, Feb 26, 2024 at 10:49:26AM +0100, Pankaj Raghav (Samsung) wrote:
> > > > From: Luis Chamberlain <mcgrof@...nel.org>
> > > > 
> > > > Supporting mapping_min_order implies that we guarantee each folio in the
> > > > page cache has at least an order of mapping_min_order. So when adding new
> > > > folios to the page cache we must ensure the index used is aligned to the
> > > > mapping_min_order as the page cache requires the index to be aligned to
> > > > the order of the folio.
> > > 
> > > This seems like a remarkably complicated way of achieving:
> > > 
> > > diff --git a/mm/filemap.c b/mm/filemap.c
> > > index 5603ced05fb7..36105dad4440 100644
> > > --- a/mm/filemap.c
> > > +++ b/mm/filemap.c
> > > @@ -2427,9 +2427,11 @@ static int filemap_update_page(struct kiocb *iocb,
> > >  }
> > >  
> > >  static int filemap_create_folio(struct file *file,
> > > -		struct address_space *mapping, pgoff_t index,
> > > +		struct address_space *mapping, loff_t pos,
> > >  		struct folio_batch *fbatch)
> > >  {
> > > +	pgoff_t index;
> > > +	unsigned int min_order;
> > >  	struct folio *folio;
> > >  	int error;
> > >  
> > > @@ -2451,6 +2453,8 @@ static int filemap_create_folio(struct file *file,
> > >  	 * well to keep locking rules simple.
> > >  	 */
> > >  	filemap_invalidate_lock_shared(mapping);
> > > +	min_order = mapping_min_folio_order(mapping);
> > > +	index = (pos >> (min_order + PAGE_SHIFT)) << min_order;
> > 
> > That is some cool mathfu. I will add a comment here as it might not be
> > that obvious to some people (i.e me).
> 
> you guys are both wrong, just use rounddown()

Umm, what do you mean just use rounddown? rounddown to ...?

We need to get index that are in PAGE units but aligned to min_order
pages.

The original patch did this:

index = mapping_align_start_index(mapping, iocb->ki_pos >> PAGE_SHIFT);

Which is essentially a rounddown operation (probably this is what you
are suggesting?).

So what willy is proposing will do the same. To me, what I proposed is
less complicated but to willy it is the other way around.