Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
To:     Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <yuchao0@huawei.com>
References: <20191022171602.93637-2-jaegeuk@kernel.org>
 <20191027225006.GA321938@sol.localdomain>
 <da214cdc-0074-b7bf-7761-d4c4ad3d4f6a@huawei.com>
 <20191030025512.GA4791@sol.localdomain>
 <97c33fa1-15af-b319-29a1-22f254a26c0a@huawei.com>
 <20191030170246.GB693@sol.localdomain>
 <899f99e9-fdc7-a84b-14ec-623fa3a5e164@huawei.com>
 <20191118161146.GB41670@jaegeuk-macbookpro.roam.corp.google.com>
 <20191118205822.GA57882@jaegeuk-macbookpro.roam.corp.google.com>
 <20191125174204.GB71634@jaegeuk-macbookpro.roam.corp.google.com>
 <20191211012723.GA57416@jaegeuk-macbookpro.roam.corp.google.com>
Cc:     Eric Biggers <ebiggers@kernel.org>, linux-kernel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net
From:   Chao Yu <chao@kernel.org>
Message-ID: <8b1eccb2-d791-c349-d6c2-ec2c5b33eb1d@kernel.org>
Date:   Thu, 12 Dec 2019 23:07:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20191211012723.GA57416@jaegeuk-macbookpro.roam.corp.google.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jaegeuk,

On 2019-12-11 9:27, Jaegeuk Kim wrote:
> Hi Chao,
>
> Let me know, if it's okay to integrate compression patch all together.
> I don't have a critical bug to fix w/ them now.

Cool, let me send a new RFC with below fix applied.

Thanks,

>
> Another fix:
> ---
>  fs/f2fs/compress.c | 101 ++++++++++++++++++++++++++++-----------------
>  fs/f2fs/data.c     |  15 ++++---
>  fs/f2fs/f2fs.h     |   1 -
>  3 files changed, 72 insertions(+), 45 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 7ebd2bc018bd..af23ed6deffd 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -73,6 +73,17 @@ static void f2fs_put_compressed_page(struct page *page)
>  	put_page(page);
>  }
>
> +static void f2fs_put_rpages(struct compress_ctx *cc)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < cc->cluster_size; i++) {
> +		if (!cc->rpages[i])
> +			continue;
> +		put_page(cc->rpages[i]);
> +	}
> +}
> +
>  struct page *f2fs_compress_control_page(struct page *page)
>  {
>  	return ((struct compress_io_ctx *)page_private(page))->rpages[0];
> @@ -93,7 +104,10 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
>  void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
>  {
>  	kfree(cc->rpages);
> -	f2fs_reset_compress_ctx(cc);
> +	cc->rpages = NULL;
> +	cc->nr_rpages = 0;
> +	cc->nr_cpages = 0;
> +	cc->cluster_idx = NULL_CLUSTER;
>  }
>
>  void f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
> @@ -536,14 +550,6 @@ static bool cluster_may_compress(struct compress_ctx *cc)
>  	return __cluster_may_compress(cc);
>  }
>
> -void f2fs_reset_compress_ctx(struct compress_ctx *cc)
> -{
> -	cc->rpages = NULL;
> -	cc->nr_rpages = 0;
> -	cc->nr_cpages = 0;
> -	cc->cluster_idx = NULL_CLUSTER;
> -}
> -
>  static void set_cluster_writeback(struct compress_ctx *cc)
>  {
>  	int i;
> @@ -602,13 +608,13 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
>  		ret = f2fs_read_multi_pages(cc, &bio, cc->cluster_size,
>  						&last_block_in_bio, false);
>  		if (ret)
> -			return ret;
> +			goto release_pages;
>  		if (bio)
>  			f2fs_submit_bio(sbi, bio, DATA);
>
>  		ret = f2fs_init_compress_ctx(cc);
>  		if (ret)
> -			return ret;
> +			goto release_pages;
>  	}
>
>  	for (i = 0; i < cc->cluster_size; i++) {
> @@ -638,9 +644,11 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
>
>  		for (i = cc->cluster_size - 1; i > 0; i--) {
>  			ret = f2fs_get_block(&dn, start_idx + i);
> -			if (ret)
> +			if (ret) {
>  				/* TODO: release preallocate blocks */
> -				goto release_pages;
> +				i = cc->cluster_size;
> +				goto unlock_pages;
> +			}
>
>  			if (dn.data_blkaddr != NEW_ADDR)
>  				break;
> @@ -769,7 +777,11 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>  	cic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
>  	cic->inode = inode;
>  	refcount_set(&cic->ref, 1);
> -	cic->rpages = cc->rpages;
> +	cic->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) <<
> +			cc->log_cluster_size, GFP_NOFS);
> +	if (!cic->rpages)
> +		goto out_put_cic;
> +
>  	cic->nr_rpages = cc->cluster_size;
>
>  	for (i = 0; i < cc->nr_cpages; i++) {
> @@ -793,7 +805,7 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>
>  		blkaddr = datablock_addr(dn.inode, dn.node_page,
>  							dn.ofs_in_node);
> -		fio.page = cc->rpages[i];
> +		fio.page = cic->rpages[i] = cc->rpages[i];
>  		fio.old_blkaddr = blkaddr;
>
>  		/* cluster header */
> @@ -819,7 +831,6 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>
>  		f2fs_bug_on(fio.sbi, blkaddr == NULL_ADDR);
>
> -
>  		if (fio.encrypted)
>  			fio.encrypted_page = cc->cpages[i - 1];
>  		else if (fio.compressed)
> @@ -859,17 +870,22 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>  			fi->last_disk_size = psize;
>  		up_write(&fi->i_sem);
>  	}
> -	f2fs_reset_compress_ctx(cc);
> +	f2fs_put_rpages(cc);
> +	f2fs_destroy_compress_ctx(cc);
>  	return 0;
>
>  out_destroy_crypt:
> -	for (i -= 1; i >= 0; i--)
> +	kfree(cic->rpages);
> +
> +	for (--i; i >= 0; i--)
>  		fscrypt_finalize_bounce_page(&cc->cpages[i]);
>  	for (i = 0; i < cc->nr_cpages; i++) {
>  		if (!cc->cpages[i])
>  			continue;
>  		f2fs_put_page(cc->cpages[i], 1);
>  	}
> +out_put_cic:
> +	kfree(cic);
>  out_put_dnode:
>  	f2fs_put_dnode(&dn);
>  out_unlock_op:
> @@ -963,37 +979,39 @@ int f2fs_write_multi_pages(struct compress_ctx *cc,
>  	struct f2fs_inode_info *fi = F2FS_I(cc->inode);
>  	const struct f2fs_compress_ops *cops =
>  			f2fs_cops[fi->i_compress_algorithm];
> -	int err = -EAGAIN;
> +	bool compressed = false;
> +	int err;
>
>  	*submitted = 0;
>  	if (cluster_may_compress(cc)) {
>  		err = f2fs_compress_pages(cc);
> -		if (err) {
> -			err = -EAGAIN;
> +		if (err == -EAGAIN)
>  			goto write;
> -		}
> +		else if (err)
> +			goto put_out;
> +
>  		err = f2fs_write_compressed_pages(cc, submitted,
>  							wbc, io_type);
>  		cops->destroy_compress_ctx(cc);
> +		if (!err)
> +			return 0;
> +		f2fs_bug_on(F2FS_I_SB(cc->inode), err != -EAGAIN);
>  	}
>  write:
> -	if (err == -EAGAIN) {
> -		bool compressed = false;
> -
> -		f2fs_bug_on(F2FS_I_SB(cc->inode), *submitted);
> +	f2fs_bug_on(F2FS_I_SB(cc->inode), *submitted);
>
> -		if (is_compressed_cluster(cc))
> -			compressed = true;
> +	if (is_compressed_cluster(cc))
> +		compressed = true;
>
> -		err = f2fs_write_raw_pages(cc, submitted, wbc,
> -						io_type, compressed);
> -		if (compressed) {
> -			stat_sub_compr_blocks(cc->inode, *submitted);
> -			F2FS_I(cc->inode)->i_compressed_blocks -= *submitted;
> -			f2fs_mark_inode_dirty_sync(cc->inode, true);
> -		}
> -		f2fs_destroy_compress_ctx(cc);
> +	err = f2fs_write_raw_pages(cc, submitted, wbc, io_type, compressed);
> +	if (compressed) {
> +		stat_sub_compr_blocks(cc->inode, *submitted);
> +		F2FS_I(cc->inode)->i_compressed_blocks -= *submitted;
> +		f2fs_mark_inode_dirty_sync(cc->inode, true);
>  	}
> +put_out:
> +	f2fs_put_rpages(cc);
> +	f2fs_destroy_compress_ctx(cc);
>  	return err;
>  }
>
> @@ -1055,7 +1073,13 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc)
>  		dic->tpages[i] = cc->rpages[i];
>  	}
>
> -	dic->rpages = cc->rpages;
> +	dic->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) <<
> +			cc->log_cluster_size, GFP_NOFS);
> +	if (!dic->rpages)
> +		goto out_free;
> +
> +	for (i = 0; i < dic->cluster_size; i++)
> +		dic->rpages[i] = cc->rpages[i];
>  	dic->nr_rpages = cc->cluster_size;
>  	return dic;
>
> @@ -1072,8 +1096,7 @@ void f2fs_free_dic(struct decompress_io_ctx *dic)
>  		for (i = 0; i < dic->cluster_size; i++) {
>  			if (dic->rpages[i])
>  				continue;
> -			unlock_page(dic->tpages[i]);
> -			put_page(dic->tpages[i]);
> +			f2fs_put_page(dic->tpages[i], 1);
>  		}
>  		kfree(dic->tpages);
>  	}
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 7046b222e8de..19cd03450066 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2099,7 +2099,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>  							false);
>  				f2fs_free_dic(dic);
>  				f2fs_put_dnode(&dn);
> -				f2fs_reset_compress_ctx(cc);
> +				f2fs_destroy_compress_ctx(cc);
>  				*bio_ret = bio;
>  				return ret;
>  			}
> @@ -2117,7 +2117,7 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>
>  	f2fs_put_dnode(&dn);
>
> -	f2fs_reset_compress_ctx(cc);
> +	f2fs_destroy_compress_ctx(cc);
>  	*bio_ret = bio;
>  	return 0;
>
> @@ -2125,7 +2125,6 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>  	f2fs_put_dnode(&dn);
>  out:
>  	f2fs_decompress_end_io(cc->rpages, cc->cluster_size, true, false);
> -	f2fs_destroy_compress_ctx(cc);
>  	*bio_ret = bio;
>  	return ret;
>  }
> @@ -2192,8 +2191,10 @@ int f2fs_mpage_readpages(struct address_space *mapping,
>  							max_nr_pages,
>  							&last_block_in_bio,
>  							is_readahead);
> -				if (ret)
> +				if (ret) {
> +					f2fs_destroy_compress_ctx(&cc);
>  					goto set_error_page;
> +				}
>  			}
>  			ret = f2fs_is_compressed_cluster(inode, page->index);
>  			if (ret < 0)
> @@ -2229,11 +2230,14 @@ int f2fs_mpage_readpages(struct address_space *mapping,
>  #ifdef CONFIG_F2FS_FS_COMPRESSION
>  		if (f2fs_compressed_file(inode)) {
>  			/* last page */
> -			if (nr_pages == 1 && !f2fs_cluster_is_empty(&cc))
> +			if (nr_pages == 1 && !f2fs_cluster_is_empty(&cc)) {
>  				ret = f2fs_read_multi_pages(&cc, &bio,
>  							max_nr_pages,
>  							&last_block_in_bio,
>  							is_readahead);
> +				if (ret)
> +					f2fs_destroy_compress_ctx(&cc);
> +			}
>  		}
>  #endif
>  	}
> @@ -2856,6 +2860,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
>
>  #ifdef CONFIG_F2FS_FS_COMPRESSION
>  			if (f2fs_compressed_file(inode)) {
> +				get_page(page);
>  				f2fs_compress_ctx_add_page(&cc, page);
>  				continue;
>  			}
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 26a4cc1fd686..5d55cef66410 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3765,7 +3765,6 @@ static inline bool f2fs_post_read_required(struct inode *inode)
>  #ifdef CONFIG_F2FS_FS_COMPRESSION
>  bool f2fs_is_compressed_page(struct page *page);
>  struct page *f2fs_compress_control_page(struct page *page);
> -void f2fs_reset_compress_ctx(struct compress_ctx *cc);
>  int f2fs_prepare_compress_overwrite(struct inode *inode,
>  			struct page **pagep, pgoff_t index, void **fsdata);
>  bool f2fs_compress_write_end(struct inode *inode, void *fsdata,
>