| Message ID | 1493025256-27188-2-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 24, 2017 at 12:14:06PM +0300, Amir Goldstein wrote: [..] > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index c072a0c..671bac0 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -961,6 +961,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, true); > > 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 1953986..6a857fb 100644 > --- a/fs/overlayfs/util.c > +++ b/fs/overlayfs/util.c > @@ -70,21 +70,38 @@ 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; > + enum ovl_path_type type = oe->__type; > > - if (oe->__upperdentry) { > - type = __OVL_PATH_UPPER; > + /* Matches smp_wmb() in ovl_update_type() */ > + smp_rmb(); > + return type; Hi Amir, I never manage to understand barriers so I will ask. Why this barrier is required and what can go wrong if we don't use this barrier. Vivek
On Mon, Apr 24, 2017 at 3:59 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Mon, Apr 24, 2017 at 12:14:06PM +0300, Amir Goldstein wrote: > > [..] > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > index c072a0c..671bac0 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -961,6 +961,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, true); > > > > 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 1953986..6a857fb 100644 > > --- a/fs/overlayfs/util.c > > +++ b/fs/overlayfs/util.c > > @@ -70,21 +70,38 @@ 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; > > + enum ovl_path_type type = oe->__type; > > > > - if (oe->__upperdentry) { > > - type = __OVL_PATH_UPPER; > > + /* Matches smp_wmb() in ovl_update_type() */ > > + smp_rmb(); > > + return type; > > Hi Amir, > > I never manage to understand barriers so I will ask. Why this barrier is > required and what can go wrong if we don't use this barrier. > Hi Vivek, Miklos was kind enough to answer that question for me when he made the comment about missing memmory barrier on v1 of the patch: http://www.spinics.net/lists/linux-unionfs/msg01687.html Whether or not I got it right, we shall see shortly ;-) Amir.
On Mon, Apr 24, 2017 at 04:10:30PM +0300, Amir Goldstein wrote: > On Mon, Apr 24, 2017 at 3:59 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > > > > On Mon, Apr 24, 2017 at 12:14:06PM +0300, Amir Goldstein wrote: > > > > [..] > > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > > index c072a0c..671bac0 100644 > > > --- a/fs/overlayfs/super.c > > > +++ b/fs/overlayfs/super.c > > > @@ -961,6 +961,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, true); > > > > > > 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 1953986..6a857fb 100644 > > > --- a/fs/overlayfs/util.c > > > +++ b/fs/overlayfs/util.c > > > @@ -70,21 +70,38 @@ 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; > > > + enum ovl_path_type type = oe->__type; > > > > > > - if (oe->__upperdentry) { > > > - type = __OVL_PATH_UPPER; > > > + /* Matches smp_wmb() in ovl_update_type() */ > > > + smp_rmb(); > > > + return type; > > > > Hi Amir, > > > > I never manage to understand barriers so I will ask. Why this barrier is > > required and what can go wrong if we don't use this barrier. > > > > Hi Vivek, > > Miklos was kind enough to answer that question for me when he made > the comment about missing memmory barrier on v1 of the patch: > http://www.spinics.net/lists/linux-unionfs/msg01687.html > > Whether or not I got it right, we shall see shortly ;-) Hi Amir, Thanks. Ok, so we are making sure if other cpu sees updated ->type, then it is guaranteed that it isses updated ->upperdentry as well. I feel it is worth to put a shortened version of explanation from miklos in the comments. It will help to recall why did we put it. But it is just me. May be it is obvious to others. Vivek
On Mon, Apr 24, 2017 at 4:36 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Mon, Apr 24, 2017 at 04:10:30PM +0300, Amir Goldstein wrote: >> On Mon, Apr 24, 2017 at 3:59 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >> > >> > On Mon, Apr 24, 2017 at 12:14:06PM +0300, Amir Goldstein wrote: >> > >> > [..] >> > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c >> > > index c072a0c..671bac0 100644 >> > > --- a/fs/overlayfs/super.c >> > > +++ b/fs/overlayfs/super.c >> > > @@ -961,6 +961,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, true); >> > > >> > > 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 1953986..6a857fb 100644 >> > > --- a/fs/overlayfs/util.c >> > > +++ b/fs/overlayfs/util.c >> > > @@ -70,21 +70,38 @@ 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; >> > > + enum ovl_path_type type = oe->__type; >> > > >> > > - if (oe->__upperdentry) { >> > > - type = __OVL_PATH_UPPER; >> > > + /* Matches smp_wmb() in ovl_update_type() */ >> > > + smp_rmb(); >> > > + return type; >> > >> > Hi Amir, >> > >> > I never manage to understand barriers so I will ask. Why this barrier is >> > required and what can go wrong if we don't use this barrier. >> > >> >> Hi Vivek, >> >> Miklos was kind enough to answer that question for me when he made >> the comment about missing memmory barrier on v1 of the patch: >> http://www.spinics.net/lists/linux-unionfs/msg01687.html >> >> Whether or not I got it right, we shall see shortly ;-) > > Hi Amir, > > Thanks. Ok, so we are making sure if other cpu sees updated ->type, then > it is guaranteed that it isses updated ->upperdentry as well. > > I feel it is worth to put a shortened version of explanation from miklos > in the comments. It will help to recall why did we put it. But it is just > me. May be it is obvious to others. > I recon memory barriers are obvious to few. However, I think the documentation I left is quite standard practice: - smp_rmb() has a comment to point to the matching smp_wmb() - smp_wmb() has a comment to explain what the barrier protects: * Make sure type is consistent with __upperdentry before making it * visible to ovl_path_type() (i.e. to lockless readers of oe->__type) Amir.
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index b8b0778..8788fd7 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -338,6 +338,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.is_dir); d_add(dentry, inode); return NULL; diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 741dc0b..e90a548 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -155,6 +155,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, bool is_dir); 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 59614fa..293be5f 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -31,6 +31,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; @@ -44,6 +46,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 c072a0c..671bac0 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -961,6 +961,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, true); 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 1953986..6a857fb 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -70,21 +70,38 @@ 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; + enum ovl_path_type type = oe->__type; - if (oe->__upperdentry) { - type = __OVL_PATH_UPPER; + /* Matches smp_wmb() in ovl_update_type() */ + smp_rmb(); + return type; +} + +enum ovl_path_type ovl_update_type(struct dentry *dentry, bool is_dir) +{ + struct ovl_entry *oe = dentry->d_fsdata; + enum ovl_path_type type = oe->__type; + /* Update UPPER/MERGE flags and preserve the rest */ + type &= ~(__OVL_PATH_UPPER | __OVL_PATH_MERGE); + if (oe->__upperdentry) { + type |= __OVL_PATH_UPPER; /* - * Non-dir dentry can hold lower dentry from previous - * location. + * Non-dir dentry can hold lower dentry from before + * copy-up. */ - if (oe->numlower && d_is_dir(dentry)) + if (oe->numlower && is_dir) type |= __OVL_PATH_MERGE; } else { if (oe->numlower > 1) type |= __OVL_PATH_MERGE; } + /* + * Make sure type is consistent with __upperdentry before making it + * visible to ovl_path_type(). + */ + smp_wmb(); + oe->__type = type; return type; } @@ -220,6 +237,7 @@ void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry) */ smp_wmb(); oe->__upperdentry = upperdentry; + ovl_update_type(dentry, d_is_dir(dentry)); } void ovl_inode_init(struct inode *inode, struct inode *realinode, bool is_upper)
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. Store the type value in ovl_entry and update the UPPER and MERGE type flags when needed, so ovl_path_type() just returns the stored value. 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 | 30 ++++++++++++++++++++++++------ 5 files changed, 30 insertions(+), 6 deletions(-)