On Tue, Jan 16, 2024 at 12:23:33PM +0100, Martin Wilck wrote:
> On Tue, 2024-01-16 at 02:05 -0500, Benjamin Marzinski wrote:
> > The code acted like AUTO_RESIZE_UNDEFINED didn't exist, but since
> > conf->auto_resize was never set to AUTO_RESIZE_NEVER, the default was
> > in
> > fact AUTO_RESIZE_UNDEFINED, which ended up getting treated like
> > AUTO_RESIZE_GROW_SHRINK. Remove AUTO_RESIZE_UNDEFINED, so
> > AUTO_RESIZE_NEVER really is the default.
> >
> > Fixes: 981b83ad1 ("multipathd: Add auto_resize config option")
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
>
> The analysis is correct, but the fix differs from what we did for other
> options. We won't be able to cleanly change the default later on with
> this patch applied.
>
> I would prefer to keep AUTO_RESIZE_UNDEF and DEFAULT_AUTO_RESIZE and
> just change the condition in uev_update_path() from "auto_resize !=
> AUTO_RESIZE_NEVER" to "(auto_resize == AUTO_RESIZE_GROW_ONLY ||
> auto_resize == AUTO_RESIZE_GROW_SHRINK)", unless you have strong
> reasons not to do so.
I would argue that by removing AUTO_RESIZE_UNDEF, I am making this
option more like our existing ones. Are there any options that we have
an UNDEF value for, where we don't need it to mean that we should use a
default which is defined someplace else? We either need UNDEF to show
that an option was not included in a devices or multipaths section, or
to mean that we should use a default system value defined outside of
multipath, like for dev_loss_tmo or max_fds. I don't see what it gets us
for an option like this where mulitpath must always have some defined
default behavior. There are many options that don't have and UNDEF
value. All the yes/no values that only exist in the defaults section, as
well as defaults section only options with multiple values like
force_reload and queue_without_daemon.
I'm fine with explicitly defining a DEFAULT value and setting it in
__init_config(). There are options where 0 is not UNDEF where don't do
this, like force_reload and queue_without_daemon (as well as most of the
ones where we only have yes/no values), and there are options where we
do explicitly set the option to 0, like reassign_maps and force_sync.
Manually setting the value to 0 is unnecessary, but I agree that it's
clearer to define it explicitly.
How does that sound (removing AUTO_RESIZE_UNDEF, but keeping and
actually setting the default)?
-Ben
>
> Regards,
> Martin
