Project

General

Profile

Bug #3739

cannot set zfs quota or reservation on pool version < 22

Added by Martin Matuška over 6 years ago. Updated about 6 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:

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

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

History

#1

Updated by Matthew Ahrens over 6 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.

#2

Updated by Martin Matuška over 6 years ago

I am attaching an updated patch.

#3

Updated by Matthew Ahrens over 6 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.

#4

Updated by Martin Matuška over 6 years ago

I am attaching a combined simplified patch, including removal of the three lines from dsl_prop.c

#5

Updated by Rich Lowe about 6 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100
  • Tags deleted (needs-triage)

Resolved in 013023d

Also available in: Atom PDF