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]

Message-ID: <20240613084409.GA23371@lst.de>
Date: Thu, 13 Jun 2024 10:44:10 +0200
From: Christoph Hellwig <hch@....de>
To: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
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, hch@....de,
	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 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?

>  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.

> -	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.

>  	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?