We probably should not upgrade pools if there are incompatible filesystems

[dswartz]

Someone on a forum I hang out on had a Solaris 11.1 pool (S 11.1 supports filesystem version 6). Long story short: he booted OmniOS and got a warning about the pool being out of date (since S 11.1 doesn't support version 5000). So he did 'zpool upgrade -a' and it said it succeeded. The filesystem in question was then marked as not-mountable, since it was an unsupported filesystem version (6). I understand this is not OmniOS, but I would be surprised if we don't have the same landmine waiting to be stepped on. I don't know how feasible it is to scan the list of filesystems on the pool we are going to upgrade and bail out if any of them are an unsupported version.

[behlendorf]

@dswartz the error made sense in pre-feature flag days when all the development was happening at Sun and they controlled when and what the zfs/zpool version meant. Now that we've abandoned that model the warning as it just doesn't make sense and frankly can be dangerous.

We should definitely drop the suggestion to upgrade the pool unless zfs_zpl_version_map returned pool version supported by ZoL (i.e. v28 or older). @dswartz when you're ready go ahead and open a pull request with the patch for review. Having the code to mark up can make it easier to comment on this sort of thing.

[dswartz]

@dswartz the error made sense in pre-feature flag days when all the development was happening at Sun and they controlled when and what the zfs/zpool version meant. Now that we've abandoned that model the warning as it just doesn't make sense and frankly can be dangerous.

Agreed!

We should definitely drop the suggestion to upgrade the pool unless zfs_zpl_version_map returned pool version supported by ZoL (i.e. v28 or older). @dswartz when you're ready go ahead and open a pull request with the patch for review. Having the code to mark up can make it easier to comment on this sort of thing.

Here is my proposal: amend the current check to be 'if the fs version is < the max fs version supported by the pool, recommend an upgrade', else if it's > than the max fs version supported by the pool, complain (same message as my new message which issues if&when the fs version > the max our implementation supports.) I will also need to add a '-f' flag to 'zfs upgrade' and figure out how to pass it to the lower layers. Probably ready to push an initial commit in a couple of days...

[behlendorf]

'if the max fs version supported by the pool, recommend an upgrade',

This can never happen. Older zpl versions are supported and will mount fine so there will be no ENOTSUP error. I suppose that could change at some point as of today it would be dead code.

'else if it's > than the max fs version supported by the pool, complain'

Definitely complain.

[dswartz]

Brian, I am confused. Your previous post said 'We should definitely drop the suggestion to upgrade the pool unless zfs_zpl_version_map returned pool version supported by ZoL (i.e. v28 or older).' So I was proposing to recommend an upgrade if the fs version is less than the max supported (since you might be missing features, etc...) Of course that scenario would not get an ENOTSUP, nor was I arguing it should, just that the low-level code could recommend it. I'd much rather eliminate this check outright though (e.g. if zfs version > ZPL_VERSION, fail with ENOTSUP). So that check would replace the previous one which calls zfs_zpl_version_map.

[behlendorf]

@dswartz Yes, I agree. That would be a nice clean way to handle this.

[dswartz]

Okay, I'm ready to open a pull request. One last thought: it turns out to be non-trivial to implement a '-f' flag for 'zfs upgrade', since it would have to be passed to callbacks, etc... Somewhat of a PITA for a situation that should not happen. IMO, if the proud owner of a solaris version 6 filesystem wants to move to ZoL, they can copy the data to a version 5 filesystem, destroy the v6 fs and then go ahead. Do you agree?

[dswartz]

Added pull request #2643.

[dswartz]

Hold off on the pull request. I seem to have missed some kind of merge error...

[dswartz]

Okay, I think it's good to go now...