diff mbox

f2fs: introduce on-disk layout version checking functionality

Message ID 1463679966.3573.4.camel@slavad-ubuntu-14.04 (mailing list archive)
State New, archived
Headers show

Commit Message

Viacheslav Dubeyko May 19, 2016, 5:46 p.m. UTC
From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko@hgst.com>
Date: Wed, 18 May 2016 15:58:00 -0700
Subject: [PATCH] f2fs: introduce on-disk layout version checking functionality

Currently, F2FS has 16TB limitation on volume size.
But 16TB NAND-based SSDs are around the corner. Unfortunately,
support of 16TB+ volume size needs in modification of on-disk
layout metadata structures that will be incompatible with
current version of F2FS's on-disk layout.

This patch implements support of checking version of
F2FS on-disk layout. The F2FS superblock contains major_ver and
feature fields. Implemented functionality checks the major_ver
field and presence of flag that declares 16TB+ volumes support.
If file system driver is unable to support 16TB+ volume size then
mount of operation of F2FS volume with incompatible version or
feature will fail.

Signed-off-by: Vyacheslav Dubeyko <Vyacheslav.Dubeyko@hgst.com>
CC: Vyacheslav Dubeyko <slava@dubeyko.com>
CC: Jaegeuk Kim <jaegeuk@kernel.org>
CC: Cyril Guyot <Cyril.Guyot@hgst.com>
CC: Adam Manzanares <Adam.Manzanares@hgst.com>
CC: Damien Le Moal <Damien.LeMoal@hgst.com>
---
 fs/f2fs/Kconfig | 12 ++++++++++++
 fs/f2fs/f2fs.h  | 10 +++++++++-
 fs/f2fs/super.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig May 20, 2016, 7:58 a.m. UTC | #1
Please don't do the mistake of versioning the structure, but instead
use feature flags, similar to what extN and modern XFS file systems do.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viacheslav Dubeyko May 20, 2016, 6:30 p.m. UTC | #2
On Fri, 2016-05-20 at 00:58 -0700, Christoph Hellwig wrote:
> Please don't do the mistake of versioning the structure, but instead
> use feature flags, similar to what extN and modern XFS file systems do.

I am not sure that I follow to your point. The F2FS has "feature" field
(__le32 feature) into on-disk superblock (struct f2fs_super_block). The
suggested patch introduces the new F2FS_FEATURE_16TB_SUPPORT flag. And
it looks like as your comment.

So, necessary changes in on-disk layout for 16+TB volumes support will
be incompatible with current available version of F2FS driver. It means
that, anyway, we need to increase version of on-disk layout (major_ver
of struct f2fs_super_block). The presence of superblock's version and
F2FS_FEATURE_16TB_SUPPORT flag will be very useful for consistency
checking by fsck tool.

Another point is file system driver behavior. Old F2FS file system
driver (version 1) should refuse mount operation for the case of version
2 of on-disk layout and F2FS_FEATURE_16TB_SUPPORT flag presence. Or it
could mount in RO mode for the case of absence of
F2FS_FEATURE_16TB_SUPPORT flag and version 2 of on-disk layout. But,
finally, we need to change struct f2fs_inode, struct f2fs_extent and
struct direct_node for the case of version 2 of on-disk layout. It means
that file system driver of version 1 will be unable to mount a volume
with on-disk layout of version 2. The F2FS file system driver of version
2 should support (and mount) volume as for version 1 as for version 2 of
on-disk layout.

Finally, I believe that we need to increase as on-disk layout version
number as to introduce F2FS_FEATURE_16TB_SUPPORT flag. Checking of
version number and F2FS_FEATURE_16TB_SUPPORT flag will provide way of
checking file system volume consistency as for fsck tool as for file
system driver side. Let's imagine a situation that we don't change
on-disk layout version and we will receive superblock with
F2FS_FEATURE_16TB_SUPPORT flag during mount operation. What does it
mean? Does it mean that we have corrupted superblock state? For example,
we could decide that everything is OK. As a result, we will treat struct
f2fs_inode, struct f2fs_extent and struct direct_node in improper way.
It will be the reason of very sophisticated bugs and, potentially,
volume corruptions. 

Could you share your vision in more details?

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 23, 2016, 8:25 a.m. UTC | #3
On Fri, May 20, 2016 at 11:30:43AM -0700, Viacheslav Dubeyko wrote:
> I am not sure that I follow to your point. The F2FS has "feature" field
> (__le32 feature) into on-disk superblock (struct f2fs_super_block). The
> suggested patch introduces the new F2FS_FEATURE_16TB_SUPPORT flag. And
> it looks like as your comment.

It does, but at the same time you also introduce a major version
superblock
field.

> So, necessary changes in on-disk layout for 16+TB volumes support will
> be incompatible with current available version of F2FS driver. It means
> that, anyway, we need to increase version of on-disk layout (major_ver
> of struct f2fs_super_block). The presence of superblock's version and
> F2FS_FEATURE_16TB_SUPPORT flag will be very useful for consistency
> checking by fsck tool.

Why is the feature not enough for that?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viacheslav Dubeyko May 23, 2016, 8:08 p.m. UTC | #4
On Mon, 2016-05-23 at 01:25 -0700, Christoph Hellwig wrote:
> On Fri, May 20, 2016 at 11:30:43AM -0700, Viacheslav Dubeyko wrote:
> > I am not sure that I follow to your point. The F2FS has "feature" field
> > (__le32 feature) into on-disk superblock (struct f2fs_super_block). The
> > suggested patch introduces the new F2FS_FEATURE_16TB_SUPPORT flag. And
> > it looks like as your comment.
> 
> It does, but at the same time you also introduce a major version
> superblock
> field.

I think that it's some confusion. I didn't introduce any new fields in
struct f2fs_super_block. The "major_ver" and "minor_ver" fields exist in
F2FS superblock from the beginning of this file system implementation.
The content of these two fields are defined during mkfs phase. The
f2fs_format.c contains such code in f2fs_prepare_super_block():

set_sb(major_ver, F2FS_MAJOR_VERSION);
set_sb(minor_ver, F2FS_MINOR_VERSION);

The F2FS_MAJOR_VERSION and F2FS_MINOR_VERSION are defined into
configure.ac as:

m4_define([f2fs_tools_version], m4_esyscmd([sed -n '1p' VERSION | tr -d '\n']))

AC_DEFINE([F2FS_MAJOR_VERSION], m4_bpatsubst(f2fs_tools_version,
                                [\([0-9]*\)\(\w\|\W\)*], [\1]),
                                [Major version for f2fs-tools])
AC_DEFINE([F2FS_MINOR_VERSION], m4_bpatsubst(f2fs_tools_version,
                                [\([0-9]*\).\([0-9]*\)\(\w\|\W\)*], [\2]),
                                [Minor version for f2fs-tools])

Current version in VERSION file is 1.6.1. So, historically F2FS is using
version of on-disk layout. The suggested patch simply introduces the
threshold value F2FS_MAX_SUPP_MAJOR_VERSION with the purpose to refuse
the mount operation for the case of unsupported version of on-disk
layout.

> 
> > So, necessary changes in on-disk layout for 16+TB volumes support will
> > be incompatible with current available version of F2FS driver. It means
> > that, anyway, we need to increase version of on-disk layout (major_ver
> > of struct f2fs_super_block). The presence of superblock's version and
> > F2FS_FEATURE_16TB_SUPPORT flag will be very useful for consistency
> > checking by fsck tool.
> 
> Why is the feature not enough for that?

First of all, it needs to distinguish two different points. First point,
we need to increase the on-disk layout version because we are going to
change on-disk layout in the way that old (current) driver will not
support. Second point, we need to introduce the new feature flag. But
features could be different. One feature doesn't need in on-disk layout
change but another one could require in on-disk layout modification. So,
we need in version change and new feature introduction for the case of
necessity to modify the on-disk layout.

So, I think that it's something wrong to have "major_ver" and
"minor_ver" fields in the superblock, to define these fields during mkfs
phase and not to check this version during mount operation. First of
all, to check the on-disk layout version during mount operation is the
proper activity, from my point of view. Secondly, if we have
incompatible versions of on-disk layouts then the version should be
checked at first. And, finally, I believe that feature flag should exist
for 16TB+ support too. The version is more important for this
modification but the presence of special feature flag will clearly show
the support of 16TB+ volumes.

I think that F2FS architecture and code state is the reason why we need
to increase the on-disk layout version and to introduce feature flag.

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaegeuk Kim May 23, 2016, 9:13 p.m. UTC | #5
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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 24, 2016, 8:52 a.m. UTC | #6
On Mon, May 23, 2016 at 01:08:05PM -0700, Viacheslav Dubeyko wrote:
> I think that it's some confusion. I didn't introduce any new fields in
> struct f2fs_super_block. The "major_ver" and "minor_ver" fields exist in
> F2FS superblock from the beginning of this file system implementation.
> The content of these two fields are defined during mkfs phase. The
> f2fs_format.c contains such code in f2fs_prepare_super_block():

They exists, but the kernel so far never checked them, and despite
that the feature checking works fine worth other f2fs features.

> Current version in VERSION file is 1.6.1. So, historically F2FS is using
> version of on-disk layout. The suggested patch simply introduces the
> threshold value F2FS_MAX_SUPP_MAJOR_VERSION with the purpose to refuse
> the mount operation for the case of unsupported version of on-disk
> layout.

While I've never seen an actual piece of documentation for the fields it
seems so far they just document the version of mkfs used to create
the file system.  Suddenly overloading them with semantics is just
going to create problems.

> First of all, it needs to distinguish two different points. First point,
> we need to increase the on-disk layout version because we are going to
> change on-disk layout in the way that old (current) driver will not
> support.

That's exactly what most file systems use feature flags for.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 24, 2016, 8:53 a.m. UTC | #7
On Mon, May 23, 2016 at 02:13:57PM -0700, Jaegeuk Kim wrote:
> 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?

That goes on to the next question: why do we even need a config option
for 16TB+ volume support?

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viacheslav Dubeyko May 25, 2016, 12:34 a.m. UTC | #8
On Tue, 2016-05-24 at 01:53 -0700, Christoph Hellwig wrote:

[snipped]
> 
> That goes on to the next question: why do we even need a config option
> for 16TB+ volume support?
> 

I believe that it makes sense to have config option during
implementation phase. I mean that it needs to protect this experimental
"feature" from being using till finally stabilized functionality. Then
the config option could be deleted if we haven't necessity in this
option.

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viacheslav Dubeyko May 25, 2016, 12:53 a.m. UTC | #9
On Tue, 2016-05-24 at 01:52 -0700, Christoph Hellwig wrote:
> On Mon, May 23, 2016 at 01:08:05PM -0700, Viacheslav Dubeyko wrote:
> > I think that it's some confusion. I didn't introduce any new fields in
> > struct f2fs_super_block. The "major_ver" and "minor_ver" fields exist in
> > F2FS superblock from the beginning of this file system implementation.
> > The content of these two fields are defined during mkfs phase. The
> > f2fs_format.c contains such code in f2fs_prepare_super_block():
> 
> They exists, but the kernel so far never checked them, and despite
> that the feature checking works fine worth other f2fs features.
> 
> > Current version in VERSION file is 1.6.1. So, historically F2FS is using
> > version of on-disk layout. The suggested patch simply introduces the
> > threshold value F2FS_MAX_SUPP_MAJOR_VERSION with the purpose to refuse
> > the mount operation for the case of unsupported version of on-disk
> > layout.
> 
> While I've never seen an actual piece of documentation for the fields it
> seems so far they just document the version of mkfs used to create
> the file system.  Suddenly overloading them with semantics is just
> going to create problems.
> 

The best way not to create a problem is to do nothing.

The F2FS superblock has "major_ver" and "minor_ver" fields. This
metadata structure is stored into F2FS volume. So, this two fields
define the on-disk layout's version. We are trying to change the on-disk
layout. It means that we need to increase the on-disk layout's version
number and to check the version number, namely. What's wrong with my
logic?

> > First of all, it needs to distinguish two different points. First point,
> > we need to increase the on-disk layout version because we are going to
> > change on-disk layout in the way that old (current) driver will not
> > support.
> 
> That's exactly what most file systems use feature flags for.

Frankly speaking, support of 16TB+ volumes is not a "feature" but simple
bug fix. Because this issue was created during metadata structure
definitions. And we are trying to fix this issue right now. And this
issue is on-disk layout related issue. So, the key point here is not a
feature flag but the on-disk layout's version, from my point of view.

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viacheslav Dubeyko May 25, 2016, 1:05 a.m. UTC | #10
Hi Jaegeuk,

On Mon, 2016-05-23 at 14:13 -0700, Jaegeuk Kim wrote:
> 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.
> 

I didn't catch the point. Maybe, I've missed something but, as far as I
can judge, f2fs driver v.4.8 will mount as old version of on-disk layout
as the new one. But right now it doesn't make sense to discuss this
topic because we haven't consent about ideology of this patch.

> 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?
> 

So, my logic is simple. We are trying to modify the on-disk layout. As a
result, we need to check the on-disk layout version, from my viewpoint.
And this modification is not "feature" itself but simple bug fix. And I
believe that "major_ver", "minor_ver" in F2FS superblock is the on-disk
layout version.

What do you think? Do you still believe that it should be a feature
flag?

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaegeuk Kim May 25, 2016, 5:12 p.m. UTC | #11
Hello,

On Tue, May 24, 2016 at 06:05:23PM -0700, Viacheslav Dubeyko wrote:
> Hi Jaegeuk,
> 
> On Mon, 2016-05-23 at 14:13 -0700, Jaegeuk Kim wrote:
> > 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.
> > 
> 
> I didn't catch the point. Maybe, I've missed something but, as far as I
> can judge, f2fs driver v.4.8 will mount as old version of on-disk layout
> as the new one. But right now it doesn't make sense to discuss this
> topic because we haven't consent about ideology of this patch.
> 
> > 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?
> > 
> 
> So, my logic is simple. We are trying to modify the on-disk layout. As a
> result, we need to check the on-disk layout version, from my viewpoint.
> And this modification is not "feature" itself but simple bug fix. And I
> believe that "major_ver", "minor_ver" in F2FS superblock is the on-disk
> layout version.

Hmm, the versions are to indicate f2fs-tools, not on-disk layout something.
They are simply growing as whatever reasons such as bug fixes, new features,
and so on to provide debugging information.

Thanks,

> 
> What do you think? Do you still believe that it should be a feature
> flag?
> 
> Thanks,
> Vyacheslav Dubeyko.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viacheslav Dubeyko May 25, 2016, 5:46 p.m. UTC | #12
On Wed, 2016-05-25 at 10:12 -0700, Jaegeuk Kim wrote:

[snipped]
> > 
> > So, my logic is simple. We are trying to modify the on-disk layout. As a
> > result, we need to check the on-disk layout version, from my viewpoint.
> > And this modification is not "feature" itself but simple bug fix. And I
> > believe that "major_ver", "minor_ver" in F2FS superblock is the on-disk
> > layout version.
> 
> Hmm, the versions are to indicate f2fs-tools, not on-disk layout something.
> They are simply growing as whatever reasons such as bug fixes, new features,
> and so on to provide debugging information.
> 

OK. If you need to know the version of f2fs-tools then you will try to
extract the version from f2fs-tools itself. And it is possible to track
bug fixes and the new features by means of git repository. The every
release of Linus kernel has own version number. So, again, it's easy to
track bug fixes and new features by means of Linux kernel git
repository. Moreover, struct f2fs_super_block has "version" and
"init_version" fields. These fields store kernel version again. So, it's
really easy to track kernel version and the rest stuff. It's really
crappy way to duplicate information in superblock and to store
f2fs-tools version in the superblock. The really important information
is the on-disk layout version. And f2fs-tools should follow the on-disk
layout version namely.

What do you think?

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
index 1f8982a..ec6bba4 100644
--- a/fs/f2fs/Kconfig
+++ b/fs/f2fs/Kconfig
@@ -94,3 +94,15 @@  config F2FS_IO_TRACE
 	  information and block IO patterns in the filesystem level.
 
 	  If unsure, say N.
+
+config F2FS_16TB_VOLUME_SUPPORT
+	bool "Support for large (16TB+) F2FS volumes"
+	depends on F2FS_FS
+	default n
+	help
+	  Enable support of 16TB and larger F2FS volumes.
+
+	  This feature is under implementation right now. So, no real support
+	  is available yet.
+
+	  If unsure, say N.
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7a4558d..8fa3acc 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -37,6 +37,13 @@ 
 	} while (0)
 #endif
 
+#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
+
 /*
  * For mount options
  */
@@ -75,7 +82,8 @@  struct f2fs_mount_info {
 	unsigned int	opt;
 };
 
-#define F2FS_FEATURE_ENCRYPT	0x0001
+#define F2FS_FEATURE_ENCRYPT		(1 << 0)
+#define F2FS_FEATURE_16TB_SUPPORT	(1 << 1)
 
 #define F2FS_HAS_FEATURE(sb, mask)					\
 	((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 006f87d..9dcb7a4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1318,6 +1318,53 @@  int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
 	return err;
 }
 
+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) {
+		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 */