Re: [PATCH v5 2/5] lib: Add zstd modules

[Austin S. Hemmelgarn]

On 2017-08-10 15:25, Hugo Mills wrote:
> On Thu, Aug 10, 2017 at 01:41:21PM -0400, Chris Mason wrote:
> > On 08/10/2017 04:30 AM, Eric Biggers wrote:
> > > Theses benchmarks are misleading because they compress the whole file as a
> > > single stream without resetting the dictionary, which isn't how data will
> > > typically be compressed in kernel mode.  With filesystem compression the data
> > > has to be divided into small chunks that can each be decompressed independently.
> > > That eliminates one of the primary advantages of Zstandard (support for large
> > > dictionary sizes).
> >
> > I did btrfs benchmarks of kernel trees and other normal data sets as
> > well.  The numbers were in line with what Nick is posting here.
> > zstd is a big win over both lzo and zlib from a btrfs point of view.
> >
> > It's true Nick's patches only support a single compression level in
> > btrfs, but that's because btrfs doesn't have a way to pass in the
> > compression ratio.  It could easily be a mount option, it was just
> > outside the scope of Nick's initial work.
>
>     Could we please not add more mount options? I get that they're easy
> to implement, but it's a very blunt instrument. What we tend to see
> (with both nodatacow and compress) is people using the mount options,
> then asking for exceptions, discovering that they can't do that, and
> then falling back to doing it with attributes or btrfs properties.
> Could we just start with btrfs properties this time round, and cut out
> the mount option part of this cycle.
AFAIUI, the intent is to extend the compression type specification for 
both the mount options and the property, not to add a new mount option. 
I think we all agree that `mount -o compress=zstd3` is a lot better than 
`mount -o compress=zstd,compresslevel=3`.
>     In the long run, it'd be great to see most of the btrfs-specific
> mount options get deprecated and ultimately removed entirely, in
> favour of attributes/properties, where feasible.
Are properties set on the root subvolume inherited properly?  Because 
unless they are, we can't get the same semantics.

Two other counter arguments on completely removing BTRFS-specific mount 
options:
1. It's a lot easier and a lot more clearly defined to change things 
that affect global behavior of the FS by a remount than having to 
iterate everything in the FS to update properties.  If I'm disabling 
autodefrag, I'd much rather just `mount -o remount,noautodefrag` than 
`find / -xdev -exec btrfs property set \{\} autodefrag false`, as the 
first will take effect for everything simultaneously and run 
exponentially quicker.
2. There are some things that don't make sense as per-object settings or 
are otherwise nonsensical on objects.  Many, but not all, of the BTRFS 
specific mount options fall into this category IMO, with the notable 
exception of compress[-force], [no]autodefrag, [no]datacow, and 
[no]datasum.  Some other options do make sense as properties of the 
filesystem (commit, flushoncommit, {inode,space}_cache, max_inline, 
metadata_ratio, [no]ssd, and [no]treelog are such options), but many are 
one-off options that affect behavior on mount (like skip_balance, 
clear_cache, nologreplay, norecovery, usebbackuproot, and subvol).