* [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
* 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
* [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
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