Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set

[Anand Jain <anand.jain@oracle.com>]

On 3/14/19 1:45 AM, David Sterba wrote:
> On Wed, Mar 13, 2019 at 06:33:50PM +0800, Anand Jain wrote:
>>
>>
>>
>> On 3/13/19 1:36 PM, Anand Jain wrote:
>>> The compression property resets to NULL, instead of the old value if we
>>> fail to set the new compression parameter.
>>>
>>> btrfs prop get /btrfs compression
>>>     compression=lzo
>>> btrfs prop set /btrfs compression zli
>>>     ERROR: failed to set compression for /btrfs: Invalid argument
>>> btrfs prop get /btrfs compression
>>>
>>> This is because the compression property ->validate() is successful for
>>> 'zli' as the strncmp() used the len passed from the userland.
>>>
>>> Fix it by using the expected string length in strncmp().
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>    fs/btrfs/props.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>>> index ef6502a94712..7aa362c2fbcf 100644
>>> --- a/fs/btrfs/props.c
>>> +++ b/fs/btrfs/props.c
>>> @@ -277,11 +277,11 @@ static int prop_compression_validate(struct inode *inode, const char *value,
>>>    	if (!value)
>>>    		return 0;
>>>    
>>> -	if (!strncmp("lzo", value, len))
>>> +	if (!strncmp("lzo", value, 3))
>>>    		return 0;
>>> -	else if (!strncmp("zlib", value, len))
>>> +	else if (!strncmp("zlib", value, 4))
>>>    		return 0;
>>> -	else if (!strncmp("zstd", value, len))
>>> +	else if (!strncmp("zstd", value, 4))
>>>    		return 0;
>>>
>>
>>
>>     Nack.
>> Now some junk value after expected string is not an error.
>> such as..
>>
>> btrfs prop set /btrfs compression lzo110
> 
> This was intentional when the zlib levels were introduced so older
> kernels can understand newer compression specifier but still have a sane
> fallback (ie use the default level). Now it would be better to extend
> the validation to parse the method:level format at least. But as
> mentioned in the other mail, the level needs to be propagated elsewhere
> too so it's not just a change to the validation.
> 

  Compression levels properties can be implemented in a newer set of the
  patches? So I believe this set ok to merge.

  The other way to fix this was to save and rewrite the old parameter,
  but that's not a good idea as well, because when parameter is not valid
  we should rather fail without any update (generation/transid should
  also remain unchanged, there is a bug [1] in the original code which
  shall be fixed separately).

  [1]
  with this patch,
  btrfs in dump-super /dev/sdb | grep "^generation"; btrfs prop set 
/btrfs compression zli; sync; btrfs in dump-super /dev/sdb | grep 
"^generation"

generation		21
ERROR: failed to set compression for /btrfs: Invalid argument
generation		22


Thanks, Anand