On Fri, Dec 29, 2017 at 4:21 AM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote: > On 2017/12/28 21:18, Amir Goldstein Wrote: >> On Thu, Dec 28, 2017 at 1:40 PM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote: >>> An impure directory is a directory with "trusted.overlay.impure" xattr >>> valued 'y', which indicate that this directory may contain copy-uped >>> targets from lower layers. In oredr to prevent 'd_ino' change while >>> copy-up (it create a new inode in upper layer) in getdents(2), impure >>> xattr will be set in the parent directory, letting overlay filesystem >>> check and get d_ino from lower origin target to ensure consistent d_ino. >>> >>> There are three situations of setting impure xattr: >>> 1) Copyup lower target in a directory. >>> 2) Link an origined target (already copy-uped, have origin xattr) into >>> a directory. >>> 3) Rename an origined target (include merged subdirectories) into a >>> new directory. >>> >>> So, if a direcotry which contains several origined targets or redirect >>> directories, the impure xattr should be set. If not, fix this xattr. >>> >>> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx> >>> --- [...] >>> case FTS_DEFAULT: >>> /* Check whiteouts */ >>> - err = __check_entry(sctx, sop->whiteout); >>> - ret = (err) ? err : ret; >>> + ret = scan_check_entry(sop->whiteout, sctx); >>> + if (ret) >>> + goto out; >>> + >>> + /* Check origin */ >>> + ret = scan_check_entry(sop->origin, sctx); >>> + if (ret) >>> + goto out; >> >> All the re-factoring of scan helpers and this additional origin check >> do not seem relevant to this change's subject (impure dir check). >> > Current origin check function only count origin targets in a directory > (this function can used for future check). Impure dir check use this > counts to distinguish this directory is impure or not, so this origin > check is necessary in this patch. > I see. Anyway, the cleaner the patch is, the easier it is to review it. A re-factoring patch that does not change behavior and declares this in commit message is easy to verify in review and a logic change patch that declares the logical change in commit message is easy to review. Mixing them both in a single patch without being able to declare that this is only a logical change not that this is only re-factoring makes reviewing harder. This is generally speaking. This patch was easy enough to review anyway, but other reviewers may not feel the same way. Cheers, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
