Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <98e30ed3-e9b1-2869-b31e-32b099ef3865@samsung.com>
Date:   Thu, 13 Jul 2023 12:36:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
        Thunderbird/102.13.0
Subject: Re: [PATCH 3/7] affs: Convert data read and write to use folios
Content-Language: en-US
To:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>
CC:     David Sterba <dsterba@suse.com>, <linux-fsdevel@vger.kernel.org>,
        Konstantin Komarov <almaz.alexandrovich@paragon-software.com>,
        <ntfs3@lists.linux.dev>, Theodore Tso <tytso@mit.edu>,
        Jan Kara <jack@suse.com>, <linux-ext4@vger.kernel.org>
From:   Pankaj Raghav <p.raghav@samsung.com>
In-Reply-To: <20230713035512.4139457-4-willy@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
CMS-TYPE: 201P
References: <20230713035512.4139457-1-willy@infradead.org>
        <CGME20230713035542eucas1p2cbb80eae0876527c2ba3d5b7dc46c906@eucas1p2.samsung.com>
        <20230713035512.4139457-4-willy@infradead.org>
Precedence: bulk

On 2023-07-13 05:55, Matthew Wilcox (Oracle) wrote:
> We still need to convert to/from folios in write_begin & write_end
> to fit the API, but this removes a lot of calls to old page-based
> functions, removing many hidden calls to compound_head().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good to me except one minor nit!

Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

> @@ -624,25 +623,23 @@ affs_extent_file_ofs(struct inode *inode, u32 newsize)
>  	return PTR_ERR(bh);
>  }
>  
> -static int
> -affs_read_folio_ofs(struct file *file, struct folio *folio)
> +static int affs_read_folio_ofs(struct file *file, struct folio *folio)
>  {
> -	struct page *page = &folio->page;
> -	struct inode *inode = page->mapping->host;
> -	u32 to;
> +	struct inode *inode = folio->mapping->host;
> +	size_t to;
>  	int err;
>  
> -	pr_debug("%s(%lu, %ld)\n", __func__, inode->i_ino, page->index);
> -	to = PAGE_SIZE;
> -	if (((page->index + 1) << PAGE_SHIFT) > inode->i_size) {
> -		to = inode->i_size & ~PAGE_MASK;
> -		memset(page_address(page) + to, 0, PAGE_SIZE - to);
> +	pr_debug("%s(%lu, %ld)\n", __func__, inode->i_ino, folio->index);
> +	to = folio_size(folio);
> +	if (folio_pos(folio) + to > inode->i_size) {
> +		to = inode->i_size - folio_pos(folio);
> +		folio_zero_segment(folio, to, folio_size(folio));

This change makes the code more readable!

>  	}
>  
> -	err = affs_do_readpage_ofs(page, to, 0);
> +	err = affs_do_read_folio_ofs(folio, to, 0);
>  	if (!err)
> @@ -688,6 +686,7 @@ static int affs_write_end_ofs(struct file *file, struct address_space *mapping,
>  				loff_t pos, unsigned len, unsigned copied,
>  				struct page *page, void *fsdata)
>  {
> +	struct folio *folio = page_folio(page);
>  	struct inode *inode = mapping->host;
>  	struct super_block *sb = inode->i_sb;
>  	struct buffer_head *bh, *prev_bh;
> @@ -701,18 +700,18 @@ static int affs_write_end_ofs(struct file *file, struct address_space *mapping,
>  	to = from + len;
>  	/*
>  	 * XXX: not sure if this can handle short copies (len < copied), but
> -	 * we don't have to, because the page should always be uptodate here,
> +	 * we don't have to, because the folio should always be uptodate here,
>  	 * due to write_begin.
>  	 */
>  
>  	pr_debug("%s(%lu, %llu, %llu)\n", __func__, inode->i_ino, pos,
>  		 pos + len);
>  	bsize = AFFS_SB(sb)->s_data_blksize;
> -	data = page_address(page);
> +	data = folio_address(folio);
>  
>  	bh = NULL;
>  	written = 0;
> -	tmp = (page->index << PAGE_SHIFT) + from;
> +	tmp = (folio->index << PAGE_SHIFT) + from;

Can this be made to:

tmp = folio_pos(folio) + from;

Similar to what is being done in affs_do_read_folio_ofs()

>  	bidx = tmp / bsize;
>  	boff = tmp % bsize;
>  	if (boff) {
> @@ -804,11 +803,11 @@ static int affs_write_end_ofs(struct file *file, struct address_space *mapping,
>  		from += tmp;
>  		bidx++;
>  	}
> -	SetPageUptodate(page);
> +	folio_mark_uptodate(folio);
>  
>  done:
>  	affs_brelse(bh);
> -	tmp = (page->index << PAGE_SHIFT) + from;
> +	tmp = (folio->index << PAGE_SHIFT) + from;

Same as the previous comment.

>  	if (tmp > inode->i_size)
>  		inode->i_size = AFFS_I(inode)->mmu_private = tmp;
>  
> @@ -819,8 +818,8 @@ static int affs_write_end_ofs(struct file *file, struct address_space *mapping,
>  	}
>  
>  err_first_bh:
> -	unlock_page(page);
> -	put_page(page);
> +	folio_unlock(folio);
> +	folio_put(folio);
>  
>  	return written;
>