Bug #3739: cannot set zfs quota or reservation on pool version < 22 - illumos gate - illumos

Actions

Copy link

Bug #3739

closed

cannot set zfs quota or reservation on pool version < 22

Added by Martin Matuška over 10 years ago. Updated over 10 years ago.

Status:

Resolved

Priority:

Normal

Assignee:

Category:

zfs - Zettabyte File System

Start date:

2013-04-21

Due date:

% Done:

100%

Estimated time:

Difficulty:

Medium

Tags:

Gerrit CR:

External Bug:

Description

Starting with revision 13973:4972ab336f54 (3464 zfs synctask code needs restructuring) the process of setting quota and reservation has changed. The values are now set in dsl_dir.c using dsl_prop_set_sync_impl() instead of dsl_prop_set_sync() and the resulting value (newval) is determined with dsl_prop_get_int_ds().

The function dsl_prop_set_sync_impl() does the following:

    if (version < SPA_VERSION_RECVD_PROPS) {
        zfs_prop_t prop = zfs_name_to_prop(propname);
        if (prop == ZFS_PROP_QUOTA || prop == ZFS_PROP_RESERVATION)
            return;

This causes newval to be zero and quota and reservation are not being set on pools lower than version 22. Before 13973 setting quota and reservation was not dependent on dsl_prop_set_sync_impl().

I suggest removing the check (see attached patch).

Steps to reproduce:

zpool create -o version=21 testpool /some/device_or_file
zfs set quota=200M testpool
zfs get quota testpool
zfs set reservation=200M testpool
zfs get reservation testpool

Files

Download all files

dsl_prop.c.patch (545 Bytes) dsl_prop.c.patch		Martin Matuška, 2013-04-21 08:16 PM
dsl_prop.c.2.patch (2.68 KB) dsl_prop.c.2.patch		Martin Matuška, 2013-04-22 06:56 PM
3739.patch (2.5 KB) 3739.patch		Martin Matuška, 2013-04-22 10:39 PM

Actions

Copy link

Updated by Matthew Ahrens over 10 years ago

Your patch would have use set the "quota" and "reservation" properties in dd_props_obj even on old version pools. Is there any way that can confuse us?

E.g. what if I use your code to set the quota to A on an old version pool. Then I use this pool with some old code and change the quota to B. Then use the pool with the new code, and "zpool upgrade" it. dd_props_obj has quota=A, but dd_quota=B. We usually ignore the quota setting in the dd_props_obj, which will give us the correct behavior.

The only exception would be dsl_prop_get_received(). In this case, dsl_prop_get_hasrecvd() would be false, because this is still an old version pool. So we'll call dsl_prop_get_all_ds(DSL_PROP_GET_LOCAL), which will get quota=A (the incorrect value). This could be a problem where it's called from zfs_ioc_recv() to stash the old properties. If the recv fails, we will restore the stashed properties, including quota=A.

So unfortunately, I think that this nice, simple solution won't work.

I think that, as you suggested to me privately, we may need to deal with this in dsl_dir_set_{reservation,quota}_sync(), by setting newval=ddsqura_value and avoiding the dsl_prop_* calls on old pools. This may allow us to also remove the hack in dsl_prop_set_sync_impl() as per your patch; or at least turn it into an assertion that we are not trying to set quota/resevation on old pools. As you mentioned we would also want to log the history from the syncfuncs in this case, although it wouldn't be the end of the world if we didn't.

Actions

Copy link

Updated by Martin Matuška over 10 years ago

File dsl_prop.c.2.patch dsl_prop.c.2.patch added

I am attaching an updated patch.

Actions

Copy link

Updated by Matthew Ahrens over 10 years ago

The new patch looks like it will work. I think you can simplify the code a little bit by moving the history_log into the else clause, and you don't need the separate snprintf, you can just use %s=%lld in the log_internal call.

I think we should be able to also remove the special case for quota&reservation in dsl_prop_set_sync_impl() as per your original patch.

Actions

Copy link