[f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhoudan8]

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.

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



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhoudan8]

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.

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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Jaegeuk Kim]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhoudan]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhoudan]

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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Jaegeuk Kim]

On 12/12, zhoudan wrote:
> 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. 

I firstly lost your point since f2fs_release_compress_blocks() checked
f2fs_compressed_file().

> 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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Jaegeuk Kim]

On 12/12, zhoudan wrote:
> 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. 

I firstly lost your point since f2fs_release_compress_blocks() checked
f2fs_compressed_file().

> 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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhoudan]

However, 'f2fs_compressed_file()' only determines whether the file can 
be compressed, not whether the file has been compressed. As far as I 
know, when compress_mode is user, files marked FI_COMPRESSED_FILE 
will be compressed only after 'f2fs_ioc_compress_file()' is called.
On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> On 12/12, zhoudan wrote:
> > 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. 
> 
> I firstly lost your point since f2fs_release_compress_blocks() checked
> f2fs_compressed_file().
> 
> > 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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhoudan]

However, 'f2fs_compressed_file()' only determines whether the file can 
be compressed, not whether the file has been compressed. As far as I 
know, when compress_mode is user, files marked FI_COMPRESSED_FILE 
will be compressed only after 'f2fs_ioc_compress_file()' is called.
On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> On 12/12, zhoudan wrote:
> > 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. 
> 
> I firstly lost your point since f2fs_release_compress_blocks() checked
> f2fs_compressed_file().
> 
> > 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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhou dan]

Hi, about this patch, I haven't received any reply recently.
Maybe you have some new ideas to solve this problem?


zhoudan <zhuqiandann@gmail.com> 于2022年12月13日周二 10:21写道：
>
> However, 'f2fs_compressed_file()' only determines whether the file can
> be compressed, not whether the file has been compressed. As far as I
> know, when compress_mode is user, files marked FI_COMPRESSED_FILE
> will be compressed only after 'f2fs_ioc_compress_file()' is called.
> On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> > On 12/12, zhoudan wrote:
> > > 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.
> >
> > I firstly lost your point since f2fs_release_compress_blocks() checked
> > f2fs_compressed_file().
> >
> > > 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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhou dan]

Hi, about this patch, I haven't received any reply recently.
Maybe you have some new ideas to solve this problem?


zhoudan <zhuqiandann@gmail.com> 于2022年12月13日周二 10:21写道：
>
> However, 'f2fs_compressed_file()' only determines whether the file can
> be compressed, not whether the file has been compressed. As far as I
> know, when compress_mode is user, files marked FI_COMPRESSED_FILE
> will be compressed only after 'f2fs_ioc_compress_file()' is called.
> On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> > On 12/12, zhoudan wrote:
> > > 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.
> >
> > I firstly lost your point since f2fs_release_compress_blocks() checked
> > f2fs_compressed_file().
> >
> > > 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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Jaegeuk Kim]

On 12/16, zhou dan wrote:
> Hi, about this patch, I haven't received any reply recently.
> Maybe you have some new ideas to solve this problem?

Could you please describe the exact flow that you're suffering from?

> 
> 
> zhoudan <zhuqiandann@gmail.com> 于2022年12月13日周二 10:21写道：
> >
> > However, 'f2fs_compressed_file()' only determines whether the file can
> > be compressed, not whether the file has been compressed. As far as I
> > know, when compress_mode is user, files marked FI_COMPRESSED_FILE
> > will be compressed only after 'f2fs_ioc_compress_file()' is called.
> > On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> > > On 12/12, zhoudan wrote:
> > > > 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.
> > >
> > > I firstly lost your point since f2fs_release_compress_blocks() checked
> > > f2fs_compressed_file().
> > >
> > > > 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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Jaegeuk Kim]

On 12/16, zhou dan wrote:
> Hi, about this patch, I haven't received any reply recently.
> Maybe you have some new ideas to solve this problem?

Could you please describe the exact flow that you're suffering from?

> 
> 
> zhoudan <zhuqiandann@gmail.com> 于2022年12月13日周二 10:21写道：
> >
> > However, 'f2fs_compressed_file()' only determines whether the file can
> > be compressed, not whether the file has been compressed. As far as I
> > know, when compress_mode is user, files marked FI_COMPRESSED_FILE
> > will be compressed only after 'f2fs_ioc_compress_file()' is called.
> > On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> > > On 12/12, zhoudan wrote:
> > > > 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.
> > >
> > > I firstly lost your point since f2fs_release_compress_blocks() checked
> > > f2fs_compressed_file().
> > >
> > > > 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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhou dan]

On Android S, after the app is installed, it will judge whether the file
is allowed to be compressed and release it. In the case of compress_mode=user,
the file is not compressed at this point, causing EVNAL to be returned on
subsequent executions to release.
In the method 'isCompressionAllowed', there is also such a notice:"NOTE:
The return value does not mean if the given file, or any other file on
the same file system, is actually compressed. It merely determines whether
not files <em>may</em> be compressed."

On Wed, Jan 4, 2023 at 10:29 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>
> On 12/16, zhou dan wrote:
> > Hi, about this patch, I haven't received any reply recently.
> > Maybe you have some new ideas to solve this problem?
>
> Could you please describe the exact flow that you're suffering from?
>
> >
> >
> > zhoudan <zhuqiandann@gmail.com> 于2022年12月13日周二 10:21写道：
> > >
> > > However, 'f2fs_compressed_file()' only determines whether the file can
> > > be compressed, not whether the file has been compressed. As far as I
> > > know, when compress_mode is user, files marked FI_COMPRESSED_FILE
> > > will be compressed only after 'f2fs_ioc_compress_file()' is called.
> > > On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> > > > On 12/12, zhoudan wrote:
> > > > > 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.
> > > >
> > > > I firstly lost your point since f2fs_release_compress_blocks() checked
> > > > f2fs_compressed_file().
> > > >
> > > > > 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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[zhou dan]

On Android S, after the app is installed, it will judge whether the file
is allowed to be compressed and release it. In the case of compress_mode=user,
the file is not compressed at this point, causing EVNAL to be returned on
subsequent executions to release.
In the method 'isCompressionAllowed', there is also such a notice:"NOTE:
The return value does not mean if the given file, or any other file on
the same file system, is actually compressed. It merely determines whether
not files <em>may</em> be compressed."

On Wed, Jan 4, 2023 at 10:29 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>
> On 12/16, zhou dan wrote:
> > Hi, about this patch, I haven't received any reply recently.
> > Maybe you have some new ideas to solve this problem?
>
> Could you please describe the exact flow that you're suffering from?
>
> >
> >
> > zhoudan <zhuqiandann@gmail.com> 于2022年12月13日周二 10:21写道：
> > >
> > > However, 'f2fs_compressed_file()' only determines whether the file can
> > > be compressed, not whether the file has been compressed. As far as I
> > > know, when compress_mode is user, files marked FI_COMPRESSED_FILE
> > > will be compressed only after 'f2fs_ioc_compress_file()' is called.
> > > On Mon, Dec 12, 2022 at 03:05:08PM -0800, Jaegeuk Kim wrote:
> > > > On 12/12, zhoudan wrote:
> > > > > 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.
> > > >
> > > > I firstly lost your point since f2fs_release_compress_blocks() checked
> > > > f2fs_compressed_file().
> > > >
> > > > > 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

Re: [f2fs-dev] [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Chao Yu]

On 2022/12/10 4:19, 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.

IMO, if i_compr_blocks is zero, it's better to return -EINVAL in
f2fs_release_compress_blocks(), as there will be no benefit after the conversion.

Thanks,

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: don't set FI_COMPRESS_RELEASED if file is not compressed

[Chao Yu]

On 2022/12/10 4:19, 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.

IMO, if i_compr_blocks is zero, it's better to return -EINVAL in
f2fs_release_compress_blocks(), as there will be no benefit after the conversion.

Thanks,

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