Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 30 Jan 2019 17:06:56 -0500
From:   Dennis Zhou <dennis@kernel.org>
To:     Nikolay Borisov <nborisov@suse.com>
Cc:     Dennis Zhou <dennis@kernel.org>, 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>, kernel-team@fb.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/11] btrfs: change set_level() to bound the level
 passed in
Message-ID: <20190130220656.GA50584@dennisz-mbp.dhcp.thefacebook.com>
References: <20190128212437.11597-1-dennis@kernel.org>
 <20190128212437.11597-10-dennis@kernel.org>
 <372d47b8-457d-db5d-61f4-de7b901c5e78@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <372d47b8-457d-db5d-61f4-de7b901c5e78@suse.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 29, 2019 at 10:14:18AM +0200, Nikolay Borisov wrote:
> 
> 
> On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> > Currently, the only user of set_level() is zlib which sets an internal
> > workspace parameter. As level is now plumbed into get_workspace(), this
> > can be handled there rather than separately.
> > 
> > This repurposes set_level() to bound the level passed in so it can be
> > used when setting the mounts compression level and as well as verifying
> > the level before getting a workspace. The other benefit is this divides
> > the meaning of compress(0) and get_workspace(0). The former means we
> > want to use the default compression level of the compression type. The
> > latter means we can use any workspace available.
> > 
> > Signed-off-by: Dennis Zhou <dennis@kernel.org>
> > ---
 >  }
> > diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> > index e3627139bc5c..d607be40aa0e 100644
> > --- a/fs/btrfs/compression.h
> > +++ b/fs/btrfs/compression.h
> > @@ -90,7 +90,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
> >  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> >  				 int mirror_num, unsigned long bio_flags);
> >  
> > -unsigned btrfs_compress_str2level(const char *str);
> > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
> >  
> >  enum btrfs_compression_type {
> >  	BTRFS_COMPRESS_NONE  = 0,
> > @@ -149,7 +149,7 @@ struct btrfs_compress_op {
> >  			  unsigned long start_byte,
> >  			  size_t srclen, size_t destlen);
> >  
> > -	void (*set_level)(struct list_head *ws, unsigned int type);
> > +	unsigned int (*set_level)(unsigned int level);
> 
> It might be good to document the return value since this is an
> interface. AFAICS implementations are required to return the actual
> level set irrespective of what level was passed in, no?
> 
> >  };

Yeah that makes sense. I've added a comment about it in
btrfs_compress_op.

> >  
> >  static void zlib_put_workspace(struct list_head *ws)
> > @@ -413,15 +418,14 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
> >  	return ret;
> >  }
> >  
> > -static void zlib_set_level(struct list_head *ws, unsigned int type)
> > +static unsigned int zlib_set_level(unsigned int level)
> >  {
> > -	struct workspace *workspace = list_entry(ws, struct workspace, list);
> > -	unsigned int level = BTRFS_COMPRESS_LEVEL(type);
> > -
> > -	if (level > 9)
> > +	if (!level)
> > +		level = BTRFS_ZLIB_DEFAULT_LEVEL;
> > +	else if (level > 9)
> >  		level = 9;
> 
> nit:  This makes it a bit more obvious (IMO) that you are essentially
> doing max:
> 
> if (!level)
>    level = BTRFS_ZLIB_DEFAULT_LEVEL;
> level = max(level, 9);
> 

I agree. I've updated it in both places. It should be min instead of max
though.

Thanks,
Dennis