On 2017/12/28 22:22, Amir Goldstein Wrote:
> On Thu, Dec 28, 2017 at 1:40 PM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
>> When we found a valid redirect xattr and the lower origin targed was
>> covered by a directory, it could be 1) an another redirect directory,
>> or 2) an opaque directory created after rename in overlayfs or 3) a
>> general directory created in the underlying directory when overlay is
>> offline. Currently, we only handled case 2, this patch cover the other
>> two cases.
>>
>> This patch ask user if we find a covering directory whitout opaque
>> and another redirect xattr.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx>
>> ---
>> README.md | 31 ++++++++++++++-------
>> check.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-----------------
>> 2 files changed, 90 insertions(+), 35 deletions(-)
>>
>> diff --git a/README.md b/README.md
>> index a8d0b1c..2e6146f 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -25,16 +25,27 @@ If an redirect directory is found, the following must be met:
>>
>> 1) The directory path pointed by redirect xattr should exist in one of lower
>> layers.
>> -2) The origin directory should be redirected only once in one layer, which means
>> - there is only one redirect xattr point to this origin directory in the
>> - specified layer.
>> -3) A whiteout or an opaque directory with the same name to origin should exist
>> - in the same directory as the redirect directory.
>> -
>> -If not, 1) The redirect xattr is invalid and need to remove 2) One of the
>> -redirect xattr is redundant but not sure which one is, ask user or warn in
>> -auto mode 3) Create a whiteout device or set opaque xattr to an existing
>> -directory if the parent directory was meregd or remove xattr if not.
>> +2) The origin directory should be redirected only once in one layer, which
>> + means there is only one redirect xattr point to this origin directory in
>> + the specified layer.
>> +3) There must be something with the same name to the rename origin in its
>> + origin place, maybe a whiteout/file or a(an) opaque/redirect directory
>> + covering the lower target, maybe a new created directory merging with
>> + lower origin directory.
>> +
>> +If not,
>> +1) The redirect xattr is invalid and need to remove.
>> +2) One of the redirect xattr is redundant but not sure which one is, ask user
>> + or warn in auto mode.
>> +3) Create a whiteout device if there nothing exists in its place. If the lower
>> + targed was covered by a directory which is not an another redirect directory,
>> + it could be created after rename in overlayfs or created in underlying
>> + directory when overlay is offline, not sure this directory is merged or not,
>> + ask user by default.
>> +
>> +If an invalid redirect xattr was removed by fsck and there is a corresponding
>> +lower directory with the same name exists, not sure this directory is merged
>> +or not, ask user to make a decision.
>>
>> Usage
>> =====
>> diff --git a/check.c b/check.c
>> index 0b5d49b..6db41cd 100644
>> --- a/check.c
>> +++ b/check.c
>> @@ -152,12 +152,29 @@ static inline int ovl_create_whiteout(const char *pathname)
>> return ret;
>> }
>>
>> -static inline int ovl_ask_invalid(const char *question, const char *pathname,
>> - int action)
>> +static inline bool ovl_is_redirect(const char *pathname)
>> +{
>> + bool exist = false;
>> + ssize_t ret;
>> +
>> + ret = get_xattr(pathname, OVL_REDIRECT_XATTR, NULL, &exist);
>> + return ret ? false : exist;
>> +}
>> +
>> +static inline int ovl_ask_action(const char *description, const char *pathname,
>> + const char *question, int action)
>> +{
>> + print_info(_("%s: %s "), description, pathname);
>> +
>> + return ask_question(question, action);
>> +}
>> +
>> +static inline int ovl_ask_question(const char *question, const char *pathname,
>> + int action)
>> {
>> print_info(_("%s: %s "), question, pathname);
>>
>> - return ask_question("Remove", action);
>> + return ask_question("", action);
>> }
>>
>> /*
>> @@ -233,7 +250,7 @@ remove:
>> sctx->i_whiteouts++;
>>
>> /* Remove orphan whiteout directly or ask user */
>> - if (!ovl_ask_invalid("Orphan whiteout", pathname, 1))
>> + if (!ovl_ask_action("Orphan whiteout", pathname, "Remove", 1))
>> return 0;
>>
>> ret = unlink(pathname);
>> @@ -323,15 +340,21 @@ static void ovl_redirect_free(void)
>> * 2) Count how many directories the origin directory was redirected by.
>> * If more than one in the same layer, there must be some inconsistency
>> * but not sure which one is invalid, ask user or warn in auto mode.
>> - * 3) Check and fix the missing whiteout or opaque in redierct parent dir.
>> + * 3) Check and fix the missing whiteout that covering the redirect lower
>> + * target. If the lower targed was covered by a directory which is not
>> + * an another redirect directory, it could be created after rename in
>> + * overlayfs or created in underlying directory when overlay is offline,
>> + * check opaque xattr and ask user by default.
>> + * 4) When an invalid redirect xattr is removed, and the lower layers have
>> + * directory with same name, not sure this is a merged directory, ask
>> + * user by default.
>> */
>> static int ovl_check_redirect(struct scan_ctx *sctx)
>> {
>> const char *pathname = sctx->pathname;
>> struct ovl_lower_check chk = {0};
>> - char redirect_rpath[PATH_MAX];
>> struct stat rst;
>> - char *redirect = NULL;
>> + char *redirect = NULL, *cover_path = NULL;
>> int start;
>> int ret = 0;
>>
>> @@ -345,8 +368,10 @@ static int ovl_check_redirect(struct scan_ctx *sctx)
>> sctx->t_redirects++;
>>
>> /* Redirect dir in last lower dir ? */
>> - if (sctx->dirtype == OVL_LOWER && sctx->num == lower_num-1)
>> + if (sctx->dirtype == OVL_LOWER && sctx->num == lower_num-1) {
>> + start = lower_num;
>> goto remove;
>> + }
>>
>> /* Scan lower directories to check redirect dir exist or not */
>> start = (sctx->dirtype == OVL_LOWER) ? sctx->num + 1 : 0;
>> @@ -359,14 +384,14 @@ static int ovl_check_redirect(struct scan_ctx *sctx)
>> /* Check duplicate in same layer */
>> if (ovl_redirect_entry_find(chk.path, sctx->dirtype,
>> sctx->num, &tmp)) {
>> - print_info("Duplicate redirect directories found:\n");
>> - print_info("origin:%s current:%s last:%s\n",
>> - chk.path, pathname, tmp);
>> + print_info("Duplicate redirect directory found:%s\n", pathname);
>> + print_info("Origin:%s Previous:%s\n", chk.path, tmp);
>>
>> sctx->i_redirects++;
>>
>> /* Don't remove in auto mode */
>> - if (ovl_ask_invalid("Duplicate redirect xattr", pathname, 0))
>> + if (ovl_ask_action("Duplicate redirect xattr", pathname,
>> + "Remove", 0))
>> goto remove_d;
>> }
>>
>> @@ -374,25 +399,32 @@ static int ovl_check_redirect(struct scan_ctx *sctx)
>> sctx->num, chk.path);
>>
>> /* Check and fix whiteout or opaque dir */
>> - path_pack(redirect_rpath, sizeof(redirect_rpath),
>> - sctx->dirname, redirect);
>> - if (lstat(redirect_rpath, &rst) != 0) {
>> + cover_path = path_pack(cover_path, 0, sctx->dirname, redirect);
>> + if (lstat(cover_path, &rst) != 0) {
>> if (errno != ENOENT && errno != ENOTDIR) {
>> print_err(_("Cannot stat %s: %s\n"),
>> - redirect_rpath, strerror(errno));
>> + cover_path, strerror(errno));
>> goto out;
>> }
>>
>> /* Found nothing, create a whiteout */
>> - if ((ret = ovl_create_whiteout(redirect_rpath)))
>> + if ((ret = ovl_create_whiteout(cover_path)))
>> goto out;
>>
>> sctx->t_whiteouts++;
>>
>> - } else if (is_dir(&rst) && !ovl_is_opaque(redirect_rpath)) {
>> - /* Found a directory but not opaqued, fix opaque xattr */
>> - if ((ret = ovl_set_opaque(redirect_rpath)))
>> - goto out;
>> + } else if (is_dir(&rst) && !ovl_is_opaque(cover_path) &&
>> + !ovl_is_redirect(cover_path)) {
>> +
>> + /*
>> + * Found a directory but not opaqued and not even
>> + * an another redirected directory, maybe created
>> + * when overlayfs is offline, ask user to add the
>> + * opaque xattr if this is not a merged dir.
>> + */
>> + if (!ovl_ask_question("Is merged dir", cover_path, 1))
>
> I am not sure what user will understand from this question,
> but maybe I don't see the full flow of info/questions in my mind.
> Anyway, if I am not mistaken, you seem to hold the opinion that
> default behavior if user says (-y := fix it I don't care how) should be
> to set the opaque xattr. If you agree, then you should ask:
> if (ovl_ask_question("Should set opaque dir", cover_path, 1))
>
Sorry, the behavior of '-y' and '-p' is not set opaque xattr, because I
think it's more likely that user create a new directory than simply remove
opaque xattr when overlay is offline for this case.
>> + if ((ret = ovl_set_opaque(cover_path)))
>> + goto out;
>> }
>>
>> goto out;
>> @@ -402,13 +434,25 @@ remove:
>> sctx->i_redirects++;
>>
>> /* Remove redirect xattr or ask user */
>> - if (!ovl_ask_invalid("Invalid redirect xattr", pathname, 1))
>> + if (!ovl_ask_action("Invalid redirect xattr", pathname, "Remove", 1))
>> goto out;
>> remove_d:
>> - ret = ovl_remove_redirect(pathname);
>> - if (!ret)
>> - sctx->i_redirects--;
>> + if ((ret = ovl_remove_redirect(pathname)))
>> + goto out;
>> +
>> + sctx->i_redirects--;
>> +
>> + /* If lower directory exist, ask user to add opaque xattr to cover it */
>> + if (start < lower_num) {
>> + if ((ret = ovl_check_lower(path_pick(pathname, sctx->dirlen),
>> + start, &chk, false)))
>> + goto out;
>> + if (chk.exist && is_dir(&chk.st) &&
>> + !ovl_ask_question("Is merged dir", pathname, 1))
>> + ret = ovl_set_opaque(pathname);
>
> Same here. Better that fsck -y will set opaque right?
> So flip the question to "Should set opaque dir"
>
Here is also not set opaque xattr in '-y' and '-p' mode.
I guess general user may not care about "invalid/duplicate redirect xattr"
when they modify underlying layers (eg: call "cp -a" or remove redirect origin
directory), but they probably know directories with the same name will merge
in overlayfs by default, and they should know the distribution of directories
after modification. So I prefer to merge directories. Anyway, neither will
affect consistency. :)
Thanks,
Yi.
>> + }
>> out:
>> + free(cover_path);
>> free(redirect);
>> return ret;
>> }
>> --
>> 2.9.5
>>
>
> .
>
--
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
