From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail.kernel.org ([198.145.29.136]:54759 "EHLO mail.kernel.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751923AbcEWVOA (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 23 May 2016 17:14:00 -0400
Date: Mon, 23 May 2016 14:13:57 -0700
From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Viacheslav Dubeyko <slava@dubeyko.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	Vyacheslav.Dubeyko@hgst.com, Cyril.Guyot@hgst.com,
	Adam.Manzanares@hgst.com, Damien.LeMoal@hgst.com
Subject: Re: [PATCH] f2fs: introduce on-disk layout version checking
 functionality
Message-ID: <20160523211357.GA17297@jaegeuk.gateway>
References: <1463679966.3573.4.camel@slavad-ubuntu-14.04>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1463679966.3573.4.camel@slavad-ubuntu-14.04>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

Hi Slava,

On Thu, May 19, 2016 at 10:46:06AM -0700, Viacheslav Dubeyko wrote:
...
>  
> +#ifdef CONFIG_F2FS_16TB_VOLUME_SUPPORT
> +#define F2FS_MAX_SUPP_MAJOR_VERSION		(2)
> +#define F2FS_MIN_16TB_VOLUME_SUPPORT_VERSION	(2)
> +#else
> +#define F2FS_MAX_SUPP_MAJOR_VERSION		(1)
> +#endif
> +
...
>  
> +static int f2fs_check_version_and_features(struct super_block *sb,
> +					   struct f2fs_super_block *raw_super)
> +{
> +	u16 major_ver = le16_to_cpu(raw_super->major_ver);
> +	u32 feature = le32_to_cpu(raw_super->feature);
> +
> +	if (major_ver > F2FS_MAX_SUPP_MAJOR_VERSION) {

This means, for example, f2fs driver in v4.8 will deny to mount a partition
formatted by mkfs.f2fs v3.x, which doesn't make sense, IIUC.

As Christoph mentioned, how about checking the feature only like this?

1. if the feature is ON,
 - go 64 bits   , when compiled w/  F2FS_MIN_16TB_VOLUME_SUPPORT
 - fail to mount, when compiled w/o F2FS_MIN_16TB_VOLUME_SUPPORT

2. if the feature is OFF,
 - fail to mount, when compiled w/  F2FS_MIN_16TB_VOLUME_SUPPORT
 - go 32 bits   , when compiled w/o F2FS_MIN_16TB_VOLUME_SUPPORT

Thoughts?

Thanks,

> +		f2fs_msg(sb, KERN_CRIT,
> +			 "Failed to mount volume: "
> +			 "major version %u, max supported version %u",
> +			 major_ver,
> +			 F2FS_MAX_SUPP_MAJOR_VERSION);
> +		return -EOPNOTSUPP;
> +	}
> +
> +#ifdef CONFIG_F2FS_16TB_VOLUME_SUPPORT
> +
> +	if (major_ver < F2FS_MIN_16TB_VOLUME_SUPPORT_VERSION) {
> +		if (feature & F2FS_FEATURE_16TB_SUPPORT) {
> +			f2fs_msg(sb, KERN_CRIT,
> +				 "Failed to mount corrupted volume. "
> +				 "Please, check the volume by FSCK utility.");
> +			return -EOPNOTSUPP;
> +		}
> +	} else {
> +		if (!(feature & F2FS_FEATURE_16TB_SUPPORT)) {
> +			f2fs_msg(sb, KERN_CRIT,
> +				 "Failed to mount corrupted volume. "
> +				 "Please, check the volume by FSCK utility.");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +#else
> +
> +	if (feature & F2FS_FEATURE_16TB_SUPPORT) {
> +		f2fs_msg(sb, KERN_CRIT,
> +			 "Failed to mount corrupted volume. "
> +			 "Please, check the volume by FSCK utility.");
> +		return -EOPNOTSUPP;
> +	}
> +
> +#endif
> +
> +	return 0;
> +}
> +
>  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  {
>  	struct f2fs_sb_info *sbi;
> @@ -1360,6 +1407,10 @@ try_onemore:
>  	if (err)
>  		goto free_sbi;
>  
> +	err = f2fs_check_version_and_features(sb, raw_super);
> +	if (err)
> +		goto free_sbi;
> +
>  	sb->s_fs_info = sbi;
>  	default_options(sbi);
>  	/* parse mount options */
> -- 
> 1.9.1
> 
>