On Fri, May 5, 2017 at 12:03 AM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Thu, May 4, 2017 at 4:14 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
>> One minor review comment.
>>
>> I used uuid_le_cmp(*uuid, NULL_UUID_LE) arbitrarily in my original patch,
>> but if we want to stick to semantic sb->s_uuid is probably more accurately
>> described as uuid_be, because filesystems most likely copy it in raw format
>> from disk.
>>
>> This is purely semantic of course, but if you think that matters, may as well
>> replace uuid_le with uuid_be.
>
> Okay.
>
> More changes:
>
> - use vfs_getattr() to get the lower inode number instead of
> lower->d_inode->i_ino. For one i_ino is 32bit on 32bit archs, while
> kstat.ino is always 64bit
>
> - merge the dir and non-dir getattr, they look very similar now
Nice, see suggestion below for 'improvement'.
You also forgot to mention in changes since v6:
- store 'null' fh instead of 'invalid' fh
Reviewed and tested v7.
Regarding ovl_getattr(), it is a good example for code that could make
use of OVL_TYPE_COPYUP() for better readability.
OVL_TYPE_COPYUP() was originally meant to mark also dentries
with null origin fh, but it is useful IMO also to mark dentries for which
we hold an origin (and maybe grow the original meaning later).
Attached a patch to add OVL_TYPE_COPYUP() with diminished
interpretation, which also makes use of it in ovl_getattr().
If you take that patch, you probably want to apply it before the
ovl_getattr() change though.
BTW, I tested with that patch.
Cheers,
Amir.
From cb0b6dacef3cf3b1c65459d174db2392095da9fa Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sun, 23 Apr 2017 23:12:34 +0300
Subject: [PATCH v7] ovl: set the COPYUP type flag for non-dirs
For directory entries, non zero oe->numlower implies OVL_TYPE_MERGE.
Define a new type flag OVL_TYPE_COPYUP to indicate that an entry holds
a reference to its lower copy up origin.
For directory entries COPYUP := MERGE && UPPER. For non-dir entries
COPYUP means that a lower type dentry has been recently copied up or
that we were able to find the copy up origin from overlay.fh xattr.
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
fs/overlayfs/inode.c | 5 ++---
fs/overlayfs/overlayfs.h | 2 ++
fs/overlayfs/util.c | 10 ++++++----
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index fa202cf..1195a2c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -84,12 +84,11 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
* persistent st_ino across mount cycle.
*/
if (ovl_same_sb(dentry->d_sb)) {
- ovl_path_lower(dentry, &realpath);
-
- if (OVL_TYPE_UPPER(type) && realpath.dentry) {
+ if (OVL_TYPE_COPYUP(type)) {
struct kstat lowerstat;
u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
+ ovl_path_lower(dentry, &realpath);
err = vfs_getattr(&realpath, &lowerstat,
lowermask, flags);
if (err)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f3dd5f3..d3f2ee8 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -13,10 +13,12 @@
enum ovl_path_type {
__OVL_PATH_UPPER = (1 << 0),
__OVL_PATH_MERGE = (1 << 1),
+ __OVL_PATH_COPYUP = (1 << 2),
};
#define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
#define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
+#define OVL_TYPE_COPYUP(type) ((type) & __OVL_PATH_COPYUP)
#define OVL_XATTR_PREFIX XATTR_TRUSTED_PREFIX "overlay."
#define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 4a7e5c8..fd4f78c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -83,11 +83,13 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
type = __OVL_PATH_UPPER;
/*
- * Non-dir dentry can hold lower dentry from previous
- * location.
+ * Non-dir dentry can hold lower dentry of its copy up origin.
*/
- if (oe->numlower && d_is_dir(dentry))
- type |= __OVL_PATH_MERGE;
+ if (oe->numlower) {
+ type |= __OVL_PATH_COPYUP;
+ if (d_is_dir(dentry))
+ type |= __OVL_PATH_MERGE;
+ }
} else {
if (oe->numlower > 1)
type |= __OVL_PATH_MERGE;
--
2.7.4
