btrfs: allow defrag compress to override NOCOMPRESS attribute
diff mbox

Message ID 20170713131844.706-1-dsterba@suse.com
State New
Headers show

Commit Message

David Sterba July 13, 2017, 1:18 p.m. UTC
Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression on a
given file, except when the mount is force-compress. As users have
reported on IRC, this will also prevent compression when requested by
defrag (btrfs fi defrag -c file).

The nocompress flag is set automatically by filesystem when the ratios
are bad and the user would have to manually drop the bit in order to
make defrag -c work. This is not good from the usability perspective.

This patch will raise priority for the defrag -c over nocompress, ie.
any file with NOCOMPRESS bit set will get defragmented. The bit will
remain untouched.

Alternate option was to also drop the nocompress bit and keep the
decision logic as is, but I think this is not the right solution.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Anand Jain July 15, 2017, 4:52 a.m. UTC | #1
On 07/13/2017 09:18 PM, David Sterba wrote:
> Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression on a
> given file, except when the mount is force-compress. As users have
> reported on IRC, this will also prevent compression when requested by
> defrag (btrfs fi defrag -c file).
>
> The nocompress flag is set automatically by filesystem when the ratios
> are bad and the user would have to manually drop the bit in order to
> make defrag -c work. This is not good from the usability perspective.
> 
> This patch will raise priority for the defrag -c over nocompress, ie.
> any file with NOCOMPRESS bit set will get defragmented. The bit will
> remain untouched.
> 
> Alternate option was to also drop the nocompress bit and keep the
> decision logic as is, but I think this is not the right solution.


Now the compression set through property will act same as
'-o compress-force'. Before this patch is was like '-o compress'.
I am ok to fix that patch with a new patch.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/inode.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5d3c6ac960fd..ea2f02ec0394 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -399,12 +399,14 @@ static inline int inode_need_compress(struct inode *inode)
>   	/* force compress */
>   	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
>   		return 1;
> +	/* force compress when called from defrag */
> +	if (BTRFS_I(inode)->force_compress)
> +		return 1;
>   	/* bad compression ratios */
>   	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
>   		return 0;
>   	if (btrfs_test_opt(fs_info, COMPRESS) ||
> -	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
> -	    BTRFS_I(inode)->force_compress)
> +	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS)
>   		return 1;
>   	return 0;
>   }
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Jones July 15, 2017, 6:45 a.m. UTC | #2
> -----Original Message-----

> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-

> owner@vger.kernel.org] On Behalf Of Anand Jain

> Sent: Saturday, 15 July 2017 2:53 PM

> To: David Sterba <dsterba@suse.com>; linux-btrfs@vger.kernel.org

> Subject: Re: [PATCH] btrfs: allow defrag compress to override NOCOMPRESS

> attribute

> 

> 

> On 07/13/2017 09:18 PM, David Sterba wrote:

> > Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression

> on

> > a given file, except when the mount is force-compress. As users have

> > reported on IRC, this will also prevent compression when requested by

> > defrag (btrfs fi defrag -c file).

> >

> > The nocompress flag is set automatically by filesystem when the ratios

> > are bad and the user would have to manually drop the bit in order to

> > make defrag -c work. This is not good from the usability perspective.

> >

> > This patch will raise priority for the defrag -c over nocompress, ie.

> > any file with NOCOMPRESS bit set will get defragmented. The bit will

> > remain untouched.

> >

> > Alternate option was to also drop the nocompress bit and keep the

> > decision logic as is, but I think this is not the right solution.

> 

> 

> Now the compression set through property will act same as '-o compress-

> force'. Before this patch is was like '-o compress'.

> I am ok to fix that patch with a new patch.


While we are at it, would it be possible to add an option to remove compression? Ie. btrfs fi defrag -c none file
Currently this doesn't seem to exist.

Thanks,
Paul.
David Sterba July 17, 2017, 4:47 p.m. UTC | #3
On Sat, Jul 15, 2017 at 12:52:34PM +0800, Anand Jain wrote:
> On 07/13/2017 09:18 PM, David Sterba wrote:
> > Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression on a
> > given file, except when the mount is force-compress. As users have
> > reported on IRC, this will also prevent compression when requested by
> > defrag (btrfs fi defrag -c file).
> >
> > The nocompress flag is set automatically by filesystem when the ratios
> > are bad and the user would have to manually drop the bit in order to
> > make defrag -c work. This is not good from the usability perspective.
> > 
> > This patch will raise priority for the defrag -c over nocompress, ie.
> > any file with NOCOMPRESS bit set will get defragmented. The bit will
> > remain untouched.
> > 
> > Alternate option was to also drop the nocompress bit and keep the
> > decision logic as is, but I think this is not the right solution.
> 
> 
> Now the compression set through property will act same as
> '-o compress-force'. Before this patch is was like '-o compress'.

Right, I missed that. Yet I'm not sure, we actually might want this
semantics for the per-file compression option. The mount option
'compress' applies to the whole filesystem so the semantics "try to
compress and if it's not a gain, stop and mark the file nocompress".

The per-file compression option is user's intent to compress the file,
so the nocompress fallback logic goes against that.

> I am ok to fix that patch with a new patch.

Feel free to send the patch, although I think we don't want it, we still
can have a discussion. This should help to clarify the semantics of
compression behaviour further.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba July 17, 2017, 5:06 p.m. UTC | #4
On Sat, Jul 15, 2017 at 06:45:38AM +0000, Paul Jones wrote:
> > Now the compression set through property will act same as '-o compress-
> > force'. Before this patch is was like '-o compress'.
> > I am ok to fix that patch with a new patch.
> 
> While we are at it, would it be possible to add an option to remove
> compression? Ie. btrfs fi defrag -c none file Currently this doesn't
> seem to exist.

Short answer: yes. As we're going to touch the ioctl and commandline
interface for zstd anyway.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba July 17, 2017, 5:09 p.m. UTC | #5
On Mon, Jul 17, 2017 at 06:47:07PM +0200, David Sterba wrote:
> On Sat, Jul 15, 2017 at 12:52:34PM +0800, Anand Jain wrote:
> > On 07/13/2017 09:18 PM, David Sterba wrote:
> > > Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression on a
> > > given file, except when the mount is force-compress. As users have
> > > reported on IRC, this will also prevent compression when requested by
> > > defrag (btrfs fi defrag -c file).
> > >
> > > The nocompress flag is set automatically by filesystem when the ratios
> > > are bad and the user would have to manually drop the bit in order to
> > > make defrag -c work. This is not good from the usability perspective.
> > > 
> > > This patch will raise priority for the defrag -c over nocompress, ie.
> > > any file with NOCOMPRESS bit set will get defragmented. The bit will
> > > remain untouched.
> > > 
> > > Alternate option was to also drop the nocompress bit and keep the
> > > decision logic as is, but I think this is not the right solution.
> > 
> > 
> > Now the compression set through property will act same as
> > '-o compress-force'. Before this patch is was like '-o compress'.

Actually we'll need to split btrfs_inode::force_compress as defrag will
reset it to 0 at the end, which needs to be fixed so it does not
conflict with the properties.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5d3c6ac960fd..ea2f02ec0394 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -399,12 +399,14 @@  static inline int inode_need_compress(struct inode *inode)
 	/* force compress */
 	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
 		return 1;
+	/* force compress when called from defrag */
+	if (BTRFS_I(inode)->force_compress)
+		return 1;
 	/* bad compression ratios */
 	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
 		return 0;
 	if (btrfs_test_opt(fs_info, COMPRESS) ||
-	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
-	    BTRFS_I(inode)->force_compress)
+	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS)
 		return 1;
 	return 0;
 }