svn commit: r276123 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Steven Hartland]

On 31/12/2014 02:12, Xin Li wrote:
> On 12/23/14 01:31, Steven Hartland wrote:
>> Author: smh
>> Date: Tue Dec 23 09:31:24 2014
>> New Revision: 276123
>> URL: https://svnweb.freebsd.org/changeset/base/276123
>>
>> Log:
>>    Always sync the global ZFS config cache to reflect the new mosconfig
>>    
>>    This fixes out of date zpool.cache for root pools, which can cause issues
>>    such as confusion of zdb etc.
>>    
>>    MFC after:	1 month
>>
>> Modified:
>>    head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
>>
>> Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
>> ==============================================================================
>> --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c	Tue Dec 23 08:51:30 2014	(r276122)
>> +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c	Tue Dec 23 09:31:24 2014	(r276123)
>> @@ -536,8 +536,7 @@ spa_config_update(spa_t *spa, int what)
>>   	/*
>>   	 * Update the global config cache to reflect the new mosconfig.
>>   	 */
>> -	if (!spa->spa_is_root)
>> -		spa_config_sync(spa, B_FALSE, what != SPA_CONFIG_UPDATE_POOL);
>> +	spa_config_sync(spa, B_FALSE, what != SPA_CONFIG_UPDATE_POOL);
>>   
>>   	if (what == SPA_CONFIG_UPDATE_POOL)
>>   		spa_config_update(spa, SPA_CONFIG_UPDATE_VDEVS);
> It seems like that this change breaks systems where not all pools are
> available (e.g. some of pools are encrypted) at boot time, by removing
> all these pools from the cache file.
>
> As a result, on the next boot, these pools would not be imported even
> when the devices are available (geli attached), and reverting this
> change would restore the system to its previous behavior.
>
> Perhaps it have exposed an existing bug?

I've managed to reproduce this here with mdX backed test pools, and 
looking into it this is due to spa_config_sync:

                         /*
                          * Skip over our own pool if we're about to remove
                          * ourselves from the spa namespace or any pool 
that
                          * is readonly. Since we cannot guarantee that a
                          * readonly pool would successfully import upon 
reboot,
                          * we don't allow them to be written to the 
cache file.
                          */
                         if ((spa == target && removing) ||
                             !spa_writeable(spa)) {
                                 continue;
                         }

This was added by upstream:
https://github.com/illumos/illumos-gate/commit/fb02ae025247e3b662600e5a9c1b4c33ecab7d72
https://www.illumos.org/issues/3639

It seems the desired behavior was to exclude read only pools, to prevent 
them being mounted writable on reboot, but the check is too wide and 
also excludes unavailable pools too.

The attached patch corrects the test to only check writable on active 
pools so I believe should fix the issue your seeing.

If you can test it and lmk that would be great.

     Regards
     Steve


-------------- next part --------------
Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
===================================================================
--- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c	(revision 276123)
+++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c	(working copy)
@@ -241,7 +241,8 @@ spa_config_sync(spa_t *target, boolean_t removing,
 			 * we don't allow them to be written to the cache file.
 			 */
 			if ((spa == target && removing) ||
-			    !spa_writeable(spa))
+			    (spa_state(spa) == POOL_STATE_ACTIVE &&
+			    !spa_writeable(spa)))
 				continue;
 
 			mutex_enter(&spa->spa_props_lock);