Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Dec 2022 20:21:09 +0800
From:   zhoudan <zhuqiandann@gmail.com>
To:     Jaegeuk Kim <jaegeuk@kernel.org>
Cc:     chao@kernel.org, linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, zhoudan8@xiaomi.com
Subject: Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not
 compressed
Message-ID: <20221212122109.GA714122@mi-ThinkStation-K>
References: <20221208050808.2448146-1-zhoudan8@xiaomi.com>
 <Y5OYYJYx9G2LbRmc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y5OYYJYx9G2LbRmc@google.com>
Precedence: bulk

Maybe I'm not describing it clearly enough, but I think there is 
something wrong with the logic here.The 'f2fs_release_compress_blocks'
method does not determine if the file is compressed, but simply adds 
the FI_COMPRESS_RELEASED flag. 
In particular, in the current Android system, when the application is 
installed, the release interface is called by default to release the 
storage marked as compressed,  without checking whether the file is 
actually compressed. In this case, when compress_mode is set to user, 
calling the compress interface returns ENVAL and the file cannot be 
compressed.
So I think the implementation of release needs to be modified, and 
only set FI_COMPRESS_RELEASED when it's really compressed and the 
storage is released.

On Fri, Dec 09, 2022 at 12:19:44PM -0800, Jaegeuk Kim wrote:
> On 12/08, zhoudan8 wrote:
> > In compress_mode=user, f2fs_release_compress_blocks()
> >  does not verify whether it has been compressed and
> >  sets FI_COMPRESS_RELEASED directly. which will lead to
> > return -EINVAL after calling compress.
> > To fix it,let's do not set FI_COMPRESS_RELEASED if file
> > is not compressed.
> 
> Do you mean you want to avoid EINVAL on a file having FI_COMPRESS_RELEASED
> with zero i_compr_blokcs?
> 
> I think the current logic is giving the error on a released file already.
> 
> > 
> > Signed-off-by: zhoudan8 <zhoudan8@xiaomi.com>
> > ---
> >  fs/f2fs/file.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 82cda1258227..f32910077df6 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -3451,14 +3451,13 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
> >  	ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> >  	if (ret)
> >  		goto out;
> > -
> > -	set_inode_flag(inode, FI_COMPRESS_RELEASED);
> >  	inode->i_ctime = current_time(inode);
> >  	f2fs_mark_inode_dirty_sync(inode, true);
> >  
> >  	if (!atomic_read(&F2FS_I(inode)->i_compr_blocks))
> >  		goto out;
> >  
> > +	set_inode_flag(inode, FI_COMPRESS_RELEASED);
> >  	f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >  	filemap_invalidate_lock(inode->i_mapping);
> >  
> > -- 
> > 2.38.1