在 2018年4月1日，下午6:02，Amir Goldstein <amir73il@xxxxxxxxx> 写道：
>
> On Sun, Apr 1, 2018 at 11:22 AM, cgxu519@xxxxxxx <cgxu519@xxxxxxx> wrote:
>>
>>> 在 2018年4月1日，下午3:39，Amir Goldstein <amir73il@xxxxxxxxx> 写道：
>>>
>>> On Sun, Apr 1, 2018 at 7:57 AM, Chengguang Xu <cgxu519@xxxxxxx> wrote:
>>>> Check remount options and return error if there is any unchangeable
>>>> option.
>>>>
>>>> Signed-off-by: Chengguang Xu <cgxu519@xxxxxxx>
>>>> ---
>>>> fs/overlayfs/super.c | 110 +++++++++++++++++++++++++++++++++++++++++----------
>>>> 1 file changed, 89 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>>>> index 5c8944b..ba00295 100644
>>>> --- a/fs/overlayfs/super.c
>>>> +++ b/fs/overlayfs/super.c
>>>> @@ -353,27 +353,6 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>>>> return 0;
>>>> }
>>>>
>>>> -static int ovl_remount(struct super_block *sb, int *flags, char *data)
>>>> -{
>>>> - struct ovl_fs *ofs = sb->s_fs_info;
>>>> -
>>>> - if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>>>> - return -EROFS;
>>>> -
>>>> - return 0;
>>>> -}
>>>> -
>>>> -static const struct super_operations ovl_super_operations = {
>>>> - .alloc_inode = ovl_alloc_inode,
>>>> - .destroy_inode = ovl_destroy_inode,
>>>> - .drop_inode = generic_delete_inode,
>>>> - .put_super = ovl_put_super,
>>>> - .sync_fs = ovl_sync_fs,
>>>> - .statfs = ovl_statfs,
>>>> - .show_options = ovl_show_options,
>>>> - .remount_fs = ovl_remount,
>>>> -};
>>>> -
>>>> enum {
>>>> OPT_LOWERDIR,
>>>> OPT_UPPERDIR,
>>>> @@ -534,6 +513,95 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config, bool remount)
>>>> return ovl_parse_redirect_mode(config, config->redirect_mode);
>>>> }
>>>>
>>>> +static bool ovl_string_option_equal(const char *new, const char *old)
>>>> +{
>>>> + if (new) {
>>>> + if (old) {
>>>> + if (!strcmp(new, old))
>>>> + return true;
>>>> + else
>>>> + return false;
>>>> + } else {
>>>> + return false;
>>>> + }
>>>> + } else {
>>>> + if (old)
>>>> + return false;
>>>> + else
>>>> + return true;
>>>> + }
>>>> +}
>>>> +
>>>
>>> Or maybe just:
>>> { return old && !strcmp(new, old); }
>>>
>>> You really only need to check when new is non NULL.
>>>
>>> But you don't even need this much if you follow my suggestion below.
>>
>> Yes, you are right. I just think to make a common string option compare function,
>> so it is not optimized and it is maybe useful for further case(ro->rw?), in this
>> case upperdir/wordir are from NULL to non NULL. If there is no other place need
>> it in the future then we will need to modify it to better fit for existing case.
>>
>
> So for generic:
>
> {
> if (old == new)
> return true;
> if (!old || !new)
> return false;
> return !strcmp(new, old);
> }
>
> and I wouldn't be surprised if you look for such an existing helper
> in the kernel you will find it.
OK, I’ll optimize this in V2.
>
>>
>>>
>>>> +static int ovl_remount(struct super_block *sb, int *flags, char *data)
>>>> +{
>>>> + struct ovl_fs *ofs = sb->s_fs_info;
>>>> + struct ovl_config *old_config = &ofs->config;
>>>> + struct ovl_config *new_config;
>>>> + bool remount = true;
>>>
>>> No need for helper var.
>>>
>>>> + int err;
>>>> +
>>>> + if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>>>> + return -EROFS;
>>>> +
>>>> + new_config = kzalloc(sizeof(struct ovl_config), GFP_KERNEL);
>>>> + if (!new_config)
>>>> + return -ENOMEM;
>>>> +
>>>> + new_config->index = old_config->index;
>>>> + new_config->nfs_export = old_config->nfs_export;
>>>> +
>>>> + err = ovl_parse_opt((char *)data, new_config, remount);
>>>> + if (err)
>>>> + goto out_err;
>>>> +
>>>> + err = -EINVAL;
>>>> + if (new_config->lowerdir &&
>>>> + !ovl_string_option_equal(new_config->lowerdir,
>>>> + old_config->lowerdir))
>>>> + goto out_err;
>>>> +
>>>> + if (new_config->upperdir &&
>>>> + !ovl_string_option_equal(new_config->upperdir,
>>>> + old_config->upperdir))
>>>> + goto out_err;
>>>> +
>>>> + if (new_config->workdir &&
>>>> + !ovl_string_option_equal(new_config->workdir,
>>>> + old_config->workdir))
>>>> + goto out_err;
>>>> +
>>>> + if (new_config->redirect_mode &&
>>>> + !ovl_string_option_equal(new_config->redirect_mode,
>>>> + old_config->redirect_mode))
>>>> + goto out_err;
>>>> +
>>>> + if (new_config->index != old_config->index)
>>>> + goto out_err;
>>>> +
>>>> + if (new_config->nfs_export != old_config->nfs_export)
>>>> + goto out_err;
>>>> +
>>>> + return 0;
>>>> +out_err:
>>>
>>> This error really calls for error in dmesg preferably pointing user
>>> to the offending mount option.
>>>
>>> For this reason, I think it might be better to implement those checks
>>> inside ovl_parse_opt() while parsing each option.
>>>
>>> Instead of passing bool remount, you can pass ovl_config *oldconfig
>>> to signify remount.
>>>
>>> For string options, you can replace the following code with a small helper
>>> or macro to also include the remount logic:
>>>
>>> kfree(config->upperdir);
>>> config->upperdir = match_strdup(&args[0]);
>>> if (!config->upperdir)
>>> return -ENOMEM;
>>> if (oldconfig && old->config->upperdir &&
>>> !strcmp(config->upperdir, old->config->upperdir)
>>> goto remount_err; // where you can
>>> print an error with args[0]
>>>
>>
>> Actually, in my initial version I did just like what you wrote here,
>> but considering some future potential abilities(for example ro -> rw)
>> that we can add later, I decided to get new value through parsing.
>>
>
> I don't see how my suggestion limits to ability to allow for setting
> new options on remount. I just gave an example of a parameter that
> cannot be changed from NULL to non-NULL.
I think your suggestion will work and probably effective way to do it
if we prohibit changing anything in parsing. but if we allow changeable
options, I just worry put too many logics of remount into option parsing.
>
>> I haven’t got time to check closely, but ro -> rw in remount seems doable
>> if we carefully handle readdir related things, and what about other options?
>> Are all of them really unchangeable?
>>
>
> It's tricky, need to look closely at every option, because there are some
> inter dependency between options and settings that are stored on
> mount time and not changes later on.
>
> Off the top of mt head, the change redirect_dir=[on|off|nofollow|follow]
> should be pretty much standalone and independent of the rest of
> the filesystem's state, so I don't see a reason why it cannot be changed
> on remount.
>
> For index and nfs_export, don't go there.
>
> For adding upperdir/workdir (ro->rw) I can't think of a reason why
> it shouldn't work, but need to see a patch.
Thanks for your analyzation, maybe ro->rw is a good starting point
after this supporting infrastructure.
>
> What was the motivation of this work for you?
> Do you have a specific set of options you want to change in remount?
The initial motivation is from dynamically updating container image without downtime,
dynamic updating will significantly improve productivity(specially on communication for downtime)
when we change and update base image. overlayfs is only a part work in total solution
and actually we may need to reorganize/replace layers(maybe include lowers), but I think
analyzing and implementing remount will be good staring point.
By the way, don’t you think it will be special competitive feature of overlayfs compare to
other docker storage backend?
Thanks,
Chengguang.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
