* [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@vger.kernel.org
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@vger.kernel.org
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@vger.kernel.org
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox