linux-kernel - Re: [PATCH v7 03/11] filemap: allocate 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: Mon, 17 Jun 2024 09:58:37 +0000
From: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
To: Christoph Hellwig <hch@....de>
Cc: david@...morbit.com, djwong@...nel.org, chandan.babu@...cle.com,
	brauner@...nel.org, akpm@...ux-foundation.org, willy@...radead.org,
	mcgrof@...nel.org, linux-mm@...ck.org, hare@...e.de,
	linux-kernel@...r.kernel.org, yang@...amperecomputing.com,
	Zi Yan <zi.yan@...t.com>, linux-xfs@...r.kernel.org,
	p.raghav@...sung.com, linux-fsdevel@...r.kernel.org,
	gost.dev@...sung.com, cl@...amperecomputing.com,
	john.g.garry@...cle.com
Subject: Re: [PATCH v7 03/11] filemap: allocate mapping_min_order folios in
 the page cache

On Thu, Jun 13, 2024 at 10:44:10AM +0200, Christoph Hellwig wrote:
> On Fri, Jun 07, 2024 at 02:58:54PM +0000, Pankaj Raghav (Samsung) wrote:
> > +static inline unsigned long mapping_min_folio_nrpages(struct address_space *mapping)
> > +{
> > +	return 1UL << mapping_min_folio_order(mapping);
> > +}
> 
> Overly long line here, just line break after the return type.
> 
> Then again it only has a single user just below and no documentation
> so maybe just fold it into the caller?

I do use it in later patches. I will adjust the long line here :)

> 
> >  no_page:
> >  	if (!folio && (fgp_flags & FGP_CREAT)) {
> > -		unsigned order = FGF_GET_ORDER(fgp_flags);
> > +		unsigned int min_order = mapping_min_folio_order(mapping);
> > +		unsigned int order = max(min_order, FGF_GET_ORDER(fgp_flags));
> >  		int err;
> > +		index = mapping_align_start_index(mapping, index);
> 
> I wonder if at some point splitting this block that actually allocates
> a new folio into a separate helper would be nice.  It just keep growing
> in size and complexity.
> 

I agree with that. I will put it in my future todo backlog.

> > -	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), 0);
> > +	folio = filemap_alloc_folio(mapping_gfp_mask(mapping),
> > +				    min_order);
> 
> Nit: no need to split this into multiple lines.

Ok.
> 
> >  	if (!folio)
> >  		return -ENOMEM;
> >  
> > @@ -2471,6 +2478,8 @@ static int filemap_create_folio(struct file *file,
> >  	 * well to keep locking rules simple.
> >  	 */
> >  	filemap_invalidate_lock_shared(mapping);
> > +	/* index in PAGE units but aligned to min_order number of pages. */
> 
> in PAGE_SIZE units?  Maybe also make this a complete sentence?
Yes, will do.
>