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