linux-kernel - Re: [PATCH mm-unstable] ext4: Convert mext_page_double_lock() to mext_folio_double

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, 6 Dec 2022 20:50:02 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     "Vishal Moola (Oracle)" <vishal.moola@...il.com>
Cc:     linux-mm@...ck.org, tytso@....edu, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [PATCH mm-unstable] ext4: Convert mext_page_double_lock() to
 mext_folio_double_lock()

On Tue, Dec 06, 2022 at 12:41:15PM -0800, Vishal Moola (Oracle) wrote:
> Converts mext_page_double_lock() to use folios. This change saves
> 146 bytes of kernel text and removes 3 calls to compound_head().

I think it actually removes more than three ...

>  	flags = memalloc_nofs_save();
> -	page[0] = grab_cache_page_write_begin(mapping[0], index1);
> -	if (!page[0]) {
> +	folio[0] = __filemap_get_folio(mapping[0], index1, fgp_flags,
> +			mapping_gfp_mask(mapping[0]));

one

> +	if (!folio[0]) {
>  		memalloc_nofs_restore(flags);
>  		return -ENOMEM;
>  	}
>  
> -	page[1] = grab_cache_page_write_begin(mapping[1], index2);
> +	folio[1] = __filemap_get_folio(mapping[1], index2, fgp_flags,
> +			mapping_gfp_mask(mapping[1]));

two

>  	memalloc_nofs_restore(flags);
> -	if (!page[1]) {
> -		unlock_page(page[0]);
> -		put_page(page[0]);
> +	if (!folio[1]) {
> +		folio_unlock(folio[0]);
> +		folio_put(folio[0]);

four

>  		return -ENOMEM;
>  	}
>  	/*
> -	 * grab_cache_page_write_begin() may not wait on page's writeback if
> +	 * __filemap_get_folio() may not wait on folio's writeback if
>  	 * BDI not demand that. But it is reasonable to be very conservative
> -	 * here and explicitly wait on page's writeback
> +	 * here and explicitly wait on folio's writeback
>  	 */
> -	wait_on_page_writeback(page[0]);
> -	wait_on_page_writeback(page[1]);
> +	folio_wait_writeback(folio[0]);
> +	folio_wait_writeback(folio[1]);

six

>  	if (inode1 > inode2)
> -		swap(page[0], page[1]);
> +		swap(folio[0], folio[1]);
>  
>  	return 0;
>  }
> @@ -252,7 +255,6 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  		     int block_len_in_page, int unwritten, int *err)
>  {
>  	struct inode *orig_inode = file_inode(o_filp);
> -	struct page *pagep[2] = {NULL, NULL};
>  	struct folio *folio[2] = {NULL, NULL};
>  	handle_t *handle;
>  	ext4_lblk_t orig_blk_offset, donor_blk_offset;
> @@ -303,8 +305,8 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  
>  	replaced_size = data_size;
>  
> -	*err = mext_page_double_lock(orig_inode, donor_inode, orig_page_offset,
> -				     donor_page_offset, pagep);
> +	*err = mext_folio_double_lock(orig_inode, donor_inode, orig_page_offset,
> +				     donor_page_offset, folio);
>  	if (unlikely(*err < 0))
>  		goto stop_journal;
>  	/*
> @@ -314,8 +316,6 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	 * hold page's lock, if it is still the case data copy is not
>  	 * necessary, just swap data blocks between orig and donor.
>  	 */
> -	folio[0] = page_folio(pagep[0]);
> -	folio[1] = page_folio(pagep[1]);

eight.

Three are inline, which makes sense for the 146 bytes, but we're also
removing out of line calls as well as the inline calls.

Anyway, whether the description is updated or not, this looks good to me.

Reviewed-by: Matthew Wilcox (Oracle) <willy@...radead.org>