Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Dennis Zhou <dennis@kernel.org>
Cc: David Sterba <dsterba@suse.cz>, David Sterba <dsterba@suse.com>,
	Josef Bacik <josef@toxicpanda.com>, Chris Mason <clm@fb.com>,
	Omar Sandoval <osandov@osandov.com>,
	Nick Terrell <terrelln@fb.com>,
	Nikolay Borisov <nborisov@suse.com>,
	kernel-team@fb.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/12] btrfs: change set_level() to bound the level passed in
Date: Tue, 5 Feb 2019 20:54:15 +0100
Message-ID: <20190205195414.GA2900@twin.jikos.cz> (raw)
In-Reply-To: <20190205193254.GB24701@dennisz-mbp.dhcp.thefacebook.com>

On Tue, Feb 05, 2019 at 02:32:54PM -0500, Dennis Zhou wrote:
> On Tue, Feb 05, 2019 at 08:06:37PM +0100, David Sterba wrote:
> > On Mon, Feb 04, 2019 at 03:20:05PM -0500, Dennis Zhou wrote:
> > > -unsigned int btrfs_compress_str2level(const char *str)
> > > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str)
> > >  {
> > > -	if (strncmp(str, "zlib", 4) != 0)
> > > +	unsigned int level;
> > > +	int ret;
> > > +
> > > +	if (!type)
> > >  		return 0;
> > >  
> > > -	/* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */
> > > -	if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0)
> > > -		return str[5] - '0';
> > > +	if (str[0] == ':') {
> > > +		ret = kstrtouint(str + 1, 10, &level);
> > 
> > The docs kstrtouint of say that initial + is also accepted, I'd rather
> > keep the level specification strict, ie. no "zlib:+3" and no garbage
> > after the number.
> > 
> > The validation is currently missing but I think we should catch levels
> > out of range during mount/remount. The fallback to default is a safety
> > but wrong specification should be communicated to the user early.
> 
> Ok. To make sure I understand properly for improper level (ie "30",
> "+3", "+3d") set the level to default (already done) and pr_warn saying
> invalid level?

So we have (at least) two ways how to handle:

- warn and fail the mount -- catch typos and the like, continuing with
  default could cause problems later though not that severe as the
  compressionw would be different than expected, but this could be
  considered a usability bug

- only warn and continue with the default -- not mounting a root
  filesystem just because of a typo can be worse than mounting with
  wrong level; as long as there's a warning in the log, the user still
  has a working system and can fix it manually

Both have some pros and cons and initially I was more inclined to pick
the 1st option, but now that I'm thinking about that again, the other
has some merit.

Given that zlib now falls back to default for unrecognized level, we
should probably stick to that for zstd too.

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-04 20:19 [PATCH v2 00/12] btrfs: add zstd compression level support Dennis Zhou
2019-02-04 20:19 ` [PATCH 01/12] btrfs: add helpers for compression type and level Dennis Zhou
2019-02-04 20:19 ` [PATCH 02/12] btrfs: rename workspaces_list to workspace_manager Dennis Zhou
2019-02-04 20:19 ` [PATCH 03/12] btrfs: manage heuristic workspace as index 0 Dennis Zhou
2019-02-04 20:20 ` [PATCH 04/12] btrfs: unify compression ops with workspace_manager Dennis Zhou
2019-02-04 20:20 ` [PATCH 05/12] btrfs: add helper methods for workspace manager init and cleanup Dennis Zhou
2019-02-04 20:20 ` [PATCH 06/12] btrfs: add compression interface in (get/put)_workspace() Dennis Zhou
2019-02-04 20:20 ` [PATCH 07/12] btrfs: move to fn pointers for get/put workspaces Dennis Zhou
2019-02-04 20:20 ` [PATCH 08/12] btrfs: plumb level through the compression interface Dennis Zhou
2019-02-04 20:20 ` [PATCH 09/12] btrfs: change set_level() to bound the level passed in Dennis Zhou
2019-02-05 19:06   ` David Sterba
2019-02-05 19:32     ` Dennis Zhou
2019-02-05 19:54       ` David Sterba [this message]
2019-02-05 19:59         ` Dennis Zhou
2019-02-06 16:48   ` [PATCH v2 " Dennis Zhou
2019-02-04 20:20 ` [PATCH 10/12] btrfs: zstd use the passed through level instead of default Dennis Zhou
2019-02-04 20:20 ` [PATCH 11/12] btrfs: make zstd memory requirements monotonic Dennis Zhou
2019-02-04 20:20 ` [PATCH 12/12] btrfs: add zstd compression level support Dennis Zhou
2019-02-05 19:26   ` [PATCH v2 " Dennis Zhou
2019-02-06 16:47   ` [PATCH v3 " Dennis Zhou
2019-02-05 14:57 ` [PATCH v2 00/12] " David Sterba
2019-02-05 16:03   ` Dennis Zhou
2019-02-05 16:27     ` David Sterba
2019-02-05 16:30       ` Dennis Zhou
2019-02-05 16:51         ` David Sterba
2019-02-05 17:07           ` David Sterba
2019-02-05 18:27             ` David Sterba
2019-02-05 18:30               ` Dennis Zhou
2019-02-05 20:48                 ` Dennis Zhou
2019-02-06 15:15                   ` David Sterba
2019-02-06 16:51                     ` Dennis Zhou
2019-02-07 16:59 ` David Sterba
2019-02-07 17:36   ` Dennis Zhou
2019-02-08 13:22     ` David Sterba

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190205195414.GA2900@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=clm@fb.com \
    --cc=dennis@kernel.org \
    --cc=dsterba@suse.com \
    --cc=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nborisov@suse.com \
    --cc=osandov@osandov.com \
    --cc=terrelln@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox