* [RFC][PATCH 0/2] ovl: make room to store state flags in dentry
@ 2017-03-21 16:16 Amir Goldstein
2017-03-21 16:16 ` [RFC][PATCH 1/2] ovl: store path type " Amir Goldstein
2017-03-21 16:16 ` [RFC][PATCH 2/2] ovl: cram opaque boolean into type flags Amir Goldstein
0 siblings, 2 replies; 6+ messages in thread
From: Amir Goldstein @ 2017-03-21 16:16 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-unionfs
Miklos,
This is a prep work I have done some time ago to make room
for some extra state flags on the dentry.
I converted the boolean opaque field to a flag as an example.
An example for more flags could be:
- this is a copy up file which needs to get st_ino from lower
- this is an O_TEMP copy up from open for read that needs to be
linked to upper dir on open for write
Amir Goldstein (2):
ovl: store path type in dentry
ovl: cram opaque boolean into type flags
fs/overlayfs/namei.c | 10 ++++++----
fs/overlayfs/overlayfs.h | 3 +++
fs/overlayfs/ovl_entry.h | 4 +++-
fs/overlayfs/super.c | 1 +
fs/overlayfs/util.c | 25 +++++++++++++++----------
5 files changed, 28 insertions(+), 15 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH 1/2] ovl: store path type in dentry
2017-03-21 16:16 [RFC][PATCH 0/2] ovl: make room to store state flags in dentry Amir Goldstein
@ 2017-03-21 16:16 ` Amir Goldstein
2017-03-28 14:03 ` Miklos Szeredi
2017-03-21 16:16 ` [RFC][PATCH 2/2] ovl: cram opaque boolean into type flags Amir Goldstein
1 sibling, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2017-03-21 16:16 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-unionfs
ovl_path_type() is called quite often (e.g.: on every file open)
to calculate the path type flags of overlay dentry. Those flags
are rarely changed after dentry instantiation and when changed,
flags can only be added. (e.g.: on copyup, rename and create).
Store the type value in ovl_entry and update the flags when
needed, so ovl_path_type() just returns the stored value.
The old ovl_path_type() took care of clearing OVL_TYPE_MERGE
for a non-dir entry. It did so for a reason that seem obsolete.
In any case, the code never checks OVL_TYPE_MERGE on non-dir entry,
so merge flag for non-dir is harmless.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/namei.c | 1 +
fs/overlayfs/overlayfs.h | 1 +
fs/overlayfs/ovl_entry.h | 3 +++
fs/overlayfs/super.c | 1 +
fs/overlayfs/util.c | 20 ++++++++++++--------
5 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 70a6b64..c230ee1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -481,6 +481,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
kfree(stack);
kfree(d.redirect);
dentry->d_fsdata = oe;
+ ovl_update_type(dentry);
d_add(dentry, inode);
return NULL;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 2ef53f0..d37ec4c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -168,6 +168,7 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
bool ovl_dentry_remote(struct dentry *dentry);
bool ovl_dentry_weird(struct dentry *dentry);
enum ovl_path_type ovl_path_type(struct dentry *dentry);
+enum ovl_path_type ovl_update_type(struct dentry *dentry);
void ovl_path_upper(struct dentry *dentry, struct path *path);
void ovl_path_lower(struct dentry *dentry, struct path *path);
enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 85c886d..b07c29f 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -33,6 +33,8 @@ struct ovl_fs {
wait_queue_head_t copyup_wq;
};
+enum ovl_path_type;
+
/* private information held for every overlayfs dentry */
struct ovl_entry {
struct dentry *__upperdentry;
@@ -46,6 +48,7 @@ struct ovl_entry {
};
struct rcu_head rcu;
};
+ enum ovl_path_type type;
unsigned numlower;
struct path lowerstack[];
};
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f771366..bdbed27 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -977,6 +977,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
kfree(stack);
root_dentry->d_fsdata = oe;
+ ovl_update_type(root_dentry);
realinode = d_inode(ovl_dentry_real(root_dentry));
ovl_inode_init(d_inode(root_dentry), realinode, !!upperpath.dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index aaac1ac..f9693a8 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -70,21 +70,24 @@ bool ovl_dentry_weird(struct dentry *dentry)
enum ovl_path_type ovl_path_type(struct dentry *dentry)
{
struct ovl_entry *oe = dentry->d_fsdata;
- enum ovl_path_type type = 0;
- if (oe->__upperdentry) {
- type = __OVL_PATH_UPPER;
+ return oe->type;
+}
+
+enum ovl_path_type ovl_update_type(struct dentry *dentry)
+{
+ struct ovl_entry *oe = dentry->d_fsdata;
+ enum ovl_path_type type = oe->type;
- /*
- * Non-dir dentry can hold lower dentry from previous
- * location.
- */
- if (oe->numlower && d_is_dir(dentry))
+ if (oe->__upperdentry) {
+ type |= __OVL_PATH_UPPER;
+ if (oe->numlower)
type |= __OVL_PATH_MERGE;
} else {
if (oe->numlower > 1)
type |= __OVL_PATH_MERGE;
}
+ oe->type = type;
return type;
}
@@ -248,6 +251,7 @@ void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
*/
smp_wmb();
oe->__upperdentry = upperdentry;
+ ovl_update_type(dentry);
}
void ovl_inode_init(struct inode *inode, struct inode *realinode, bool is_upper)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC][PATCH 2/2] ovl: cram opaque boolean into type flags
2017-03-21 16:16 [RFC][PATCH 0/2] ovl: make room to store state flags in dentry Amir Goldstein
2017-03-21 16:16 ` [RFC][PATCH 1/2] ovl: store path type " Amir Goldstein
@ 2017-03-21 16:16 ` Amir Goldstein
1 sibling, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2017-03-21 16:16 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-unionfs
Like the other type flags, 'opaque' is a state discovered in lookup,
set on certain ops (e.g.: create, rename) and never cleared.
We would like to add more state info to ovl_entry soon (for const ino)
and this state info would be added as type flags. It makes little sense
to have the 'opaque' state occupy a boolean, so drop the boolean member
of ovl_entry and use a type bit to represent opaqueness.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/namei.c | 9 +++++----
fs/overlayfs/overlayfs.h | 2 ++
fs/overlayfs/ovl_entry.h | 1 -
fs/overlayfs/util.c | 5 +++--
4 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index c230ee1..ac6dc9c 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -356,7 +356,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
struct dentry *upperdir, *upperdentry = NULL;
unsigned int ctr = 0;
struct inode *inode = NULL;
- bool upperopaque = false;
+ enum ovl_path_type type = 0;
char *upperredirect = NULL;
struct dentry *this;
unsigned int i;
@@ -404,7 +404,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
poe = dentry->d_sb->s_root->d_fsdata;
d.by_name = false;
}
- upperopaque = d.opaque;
+ if (d.opaque)
+ type |= __OVL_PATH_OPAQUE;
}
if (!d.stop && poe->numlower) {
@@ -474,7 +475,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
}
revert_creds(old_cred);
- oe->opaque = upperopaque;
+ oe->type = type;
oe->redirect = upperredirect;
oe->__upperdentry = upperdentry;
memcpy(oe->lowerstack, stack, sizeof(struct path) * ctr);
@@ -515,7 +516,7 @@ bool ovl_lower_positive(struct dentry *dentry)
* whiteout.
*/
if (!dentry->d_inode)
- return oe->opaque;
+ return OVL_TYPE_OPAQUE(oe->type);
/* Negative upper -> positive lower */
if (!oe->__upperdentry)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d37ec4c..bb8861c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -12,10 +12,12 @@
enum ovl_path_type {
__OVL_PATH_UPPER = (1 << 0),
__OVL_PATH_MERGE = (1 << 1),
+ __OVL_PATH_OPAQUE = (1 << 2),
};
#define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
#define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
+#define OVL_TYPE_OPAQUE(type) ((type) & __OVL_PATH_OPAQUE)
#define OVL_XATTR_PREFIX XATTR_TRUSTED_PREFIX "overlay."
#define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index b07c29f..985e706 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -43,7 +43,6 @@ struct ovl_entry {
struct {
u64 version;
const char *redirect;
- bool opaque;
bool copying;
};
struct rcu_head rcu;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f9693a8..95cbc3d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -167,7 +167,8 @@ void ovl_set_dir_cache(struct dentry *dentry, struct ovl_dir_cache *cache)
bool ovl_dentry_is_opaque(struct dentry *dentry)
{
struct ovl_entry *oe = dentry->d_fsdata;
- return oe->opaque;
+
+ return OVL_TYPE_OPAQUE(oe->type);
}
bool ovl_dentry_is_whiteout(struct dentry *dentry)
@@ -179,7 +180,7 @@ void ovl_dentry_set_opaque(struct dentry *dentry)
{
struct ovl_entry *oe = dentry->d_fsdata;
- oe->opaque = true;
+ oe->type |= __OVL_PATH_OPAQUE;
}
bool ovl_redirect_dir(struct super_block *sb)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH 1/2] ovl: store path type in dentry
2017-03-21 16:16 ` [RFC][PATCH 1/2] ovl: store path type " Amir Goldstein
@ 2017-03-28 14:03 ` Miklos Szeredi
2017-03-28 16:59 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2017-03-28 14:03 UTC (permalink / raw)
To: Amir Goldstein; +Cc: linux-unionfs
On Tue, Mar 21, 2017 at 5:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> ovl_path_type() is called quite often (e.g.: on every file open)
> to calculate the path type flags of overlay dentry. Those flags
> are rarely changed after dentry instantiation and when changed,
> flags can only be added. (e.g.: on copyup, rename and create).
>
> Store the type value in ovl_entry and update the flags when
> needed, so ovl_path_type() just returns the stored value.
Since accessing __upperdentry is lockless we need to be careful with
memory ordering issues.
For example in the new version of ovl_dentry_update() we first store
oe->__upperdentry then oe->type. In a concurrent ovl_path_real() we
first read oe->type and then based on the OVL_TYPE_UPPER flag read
oe->__upperdentry. Without any barriers the above could very easily
result in NULL upperdentry being read. The compiler or the CPU could
reorder the stores in ovl_dentry_update(), etc...
For the gory details see Documentation/memory-barriers.txt
Thanks,
Miklos
>
> The old ovl_path_type() took care of clearing OVL_TYPE_MERGE
> for a non-dir entry. It did so for a reason that seem obsolete.
> In any case, the code never checks OVL_TYPE_MERGE on non-dir entry,
> so merge flag for non-dir is harmless.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/namei.c | 1 +
> fs/overlayfs/overlayfs.h | 1 +
> fs/overlayfs/ovl_entry.h | 3 +++
> fs/overlayfs/super.c | 1 +
> fs/overlayfs/util.c | 20 ++++++++++++--------
> 5 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 70a6b64..c230ee1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -481,6 +481,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> kfree(stack);
> kfree(d.redirect);
> dentry->d_fsdata = oe;
> + ovl_update_type(dentry);
> d_add(dentry, inode);
>
> return NULL;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 2ef53f0..d37ec4c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -168,6 +168,7 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
> bool ovl_dentry_remote(struct dentry *dentry);
> bool ovl_dentry_weird(struct dentry *dentry);
> enum ovl_path_type ovl_path_type(struct dentry *dentry);
> +enum ovl_path_type ovl_update_type(struct dentry *dentry);
> void ovl_path_upper(struct dentry *dentry, struct path *path);
> void ovl_path_lower(struct dentry *dentry, struct path *path);
> enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 85c886d..b07c29f 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -33,6 +33,8 @@ struct ovl_fs {
> wait_queue_head_t copyup_wq;
> };
>
> +enum ovl_path_type;
> +
> /* private information held for every overlayfs dentry */
> struct ovl_entry {
> struct dentry *__upperdentry;
> @@ -46,6 +48,7 @@ struct ovl_entry {
> };
> struct rcu_head rcu;
> };
> + enum ovl_path_type type;
> unsigned numlower;
> struct path lowerstack[];
> };
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f771366..bdbed27 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -977,6 +977,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> kfree(stack);
>
> root_dentry->d_fsdata = oe;
> + ovl_update_type(root_dentry);
>
> realinode = d_inode(ovl_dentry_real(root_dentry));
> ovl_inode_init(d_inode(root_dentry), realinode, !!upperpath.dentry);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index aaac1ac..f9693a8 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -70,21 +70,24 @@ bool ovl_dentry_weird(struct dentry *dentry)
> enum ovl_path_type ovl_path_type(struct dentry *dentry)
> {
> struct ovl_entry *oe = dentry->d_fsdata;
> - enum ovl_path_type type = 0;
>
> - if (oe->__upperdentry) {
> - type = __OVL_PATH_UPPER;
> + return oe->type;
> +}
> +
> +enum ovl_path_type ovl_update_type(struct dentry *dentry)
> +{
> + struct ovl_entry *oe = dentry->d_fsdata;
> + enum ovl_path_type type = oe->type;
>
> - /*
> - * Non-dir dentry can hold lower dentry from previous
> - * location.
> - */
> - if (oe->numlower && d_is_dir(dentry))
> + if (oe->__upperdentry) {
> + type |= __OVL_PATH_UPPER;
> + if (oe->numlower)
> type |= __OVL_PATH_MERGE;
> } else {
> if (oe->numlower > 1)
> type |= __OVL_PATH_MERGE;
> }
> + oe->type = type;
> return type;
> }
>
> @@ -248,6 +251,7 @@ void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
> */
> smp_wmb();
> oe->__upperdentry = upperdentry;
> + ovl_update_type(dentry);
> }
>
> void ovl_inode_init(struct inode *inode, struct inode *realinode, bool is_upper)
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH 1/2] ovl: store path type in dentry
2017-03-28 14:03 ` Miklos Szeredi
@ 2017-03-28 16:59 ` Amir Goldstein
2017-03-28 20:13 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2017-03-28 16:59 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-unionfs
On Tue, Mar 28, 2017 at 10:03 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Mar 21, 2017 at 5:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> ovl_path_type() is called quite often (e.g.: on every file open)
>> to calculate the path type flags of overlay dentry. Those flags
>> are rarely changed after dentry instantiation and when changed,
>> flags can only be added. (e.g.: on copyup, rename and create).
>>
>> Store the type value in ovl_entry and update the flags when
>> needed, so ovl_path_type() just returns the stored value.
>
> Since accessing __upperdentry is lockless we need to be careful with
> memory ordering issues.
>
> For example in the new version of ovl_dentry_update() we first store
> oe->__upperdentry then oe->type. In a concurrent ovl_path_real() we
> first read oe->type and then based on the OVL_TYPE_UPPER flag read
> oe->__upperdentry. Without any barriers the above could very easily
> result in NULL upperdentry being read. The compiler or the CPU could
> reorder the stores in ovl_dentry_update(), etc...
>
Right.. thanks for pointing that out. I'll take better care.
FYI, I am just in the midst of adding oe->__tempdentry and
OVL_TYPE_RO_UPPER for copy up on O_RDONLY.
I started by trying to "upgrade" oe->__upperdentry from tmpfile
to linked file, but that was real messy.
> For the gory details see Documentation/memory-barriers.txt
>
I know about them, but still easy for me to get them wrong...
Hopefully will have something for you to review by tomorrow.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH 1/2] ovl: store path type in dentry
2017-03-28 16:59 ` Amir Goldstein
@ 2017-03-28 20:13 ` Amir Goldstein
0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2017-03-28 20:13 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-unionfs
On Tue, Mar 28, 2017 at 12:59 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 10:03 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Mar 21, 2017 at 5:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> ovl_path_type() is called quite often (e.g.: on every file open)
>>> to calculate the path type flags of overlay dentry. Those flags
>>> are rarely changed after dentry instantiation and when changed,
>>> flags can only be added. (e.g.: on copyup, rename and create).
>>>
>>> Store the type value in ovl_entry and update the flags when
>>> needed, so ovl_path_type() just returns the stored value.
>>
>> Since accessing __upperdentry is lockless we need to be careful with
>> memory ordering issues.
>>
>> For example in the new version of ovl_dentry_update() we first store
>> oe->__upperdentry then oe->type. In a concurrent ovl_path_real() we
>> first read oe->type and then based on the OVL_TYPE_UPPER flag read
>> oe->__upperdentry. Without any barriers the above could very easily
>> result in NULL upperdentry being read. The compiler or the CPU could
>> reorder the stores in ovl_dentry_update(), etc...
>>
>
> Right.. thanks for pointing that out. I'll take better care.
>
> FYI, I am just in the midst of adding oe->__tempdentry and
> OVL_TYPE_RO_UPPER for copy up on O_RDONLY.
> I started by trying to "upgrade" oe->__upperdentry from tmpfile
> to linked file, but that was real messy.
>
>> For the gory details see Documentation/memory-barriers.txt
>>
>
> I know about them, but still easy for me to get them wrong...
> Hopefully will have something for you to review by tomorrow.
>
There's a WIP here
https://github.com/amir73il/linux/commits/consistent_fd-wip
It's still buggy, but passes most of the overlay/quick xfstests including
overlay/016 ("Test ro/rw fd data inconsistencies")
It still doesn't have the fixes to safe access oe->type, but if you want to
take a look and stop me if I'm heading in the wrong direction...
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-28 20:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-21 16:16 [RFC][PATCH 0/2] ovl: make room to store state flags in dentry Amir Goldstein
2017-03-21 16:16 ` [RFC][PATCH 1/2] ovl: store path type " Amir Goldstein
2017-03-28 14:03 ` Miklos Szeredi
2017-03-28 16:59 ` Amir Goldstein
2017-03-28 20:13 ` Amir Goldstein
2017-03-21 16:16 ` [RFC][PATCH 2/2] ovl: cram opaque boolean into type flags Amir Goldstein
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.