On Wed, Oct 25, 2017 at 10:09 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> If a file is copied up metadata only and later when same file is opened
> for WRITE, then data copy up takes place. We copy up data, remove METACOPY
> xattr and then set the UPPERDATA flag in ovl_entry->flags. While all
> these operations happen with oi->lock held, read side of oi->flags is
> lockless. That is another thread on another cpu can check if UPPERDATA
> flag is set or not.
>
> So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if
> another cpu sees UPPERDATA flag set, then it should be guaranteed that
> effects of data copy up and remove xattr operations are also visible.
>
> For example.
>
> CPU1 CPU2
> ovl_copy_up_flags() acquire(oi->lock)
> ovl_dentry_needs_data_copy_up() ovl_copy_up_data()
> ovl_test_flag(OVL_UPPERDATA) vfs_removexattr()
> ovl_set_flag(OVL_UPPERDATA)
> release(oi->lock)
>
> Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if
> CPU1 perceives the effects of setting UPPERDATA flag but not effects of
> preceeding operations, that would be a problem.
Why would that be a problem?
What can go wrong?
If you try to answer the question instead of referring to a vague "problem"
you will see that only the ovl_d_real() code path can be a problem and maybe
(I did not check) ovl_getattr. Please change your example above to ovl_d_real()
code path of CPU1
>
> Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation
> and smp_rmb() on UPPERDATA flag test operation.
>
> May be some other lock or barrier is already covering it. But I am not sure
> what that is and is it obvious enough that we will not break it in future.
>
> So hence trying to be safe here and introducing barriers explicitly for
> UPPERDATA flag/bit.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/overlayfs/copy_up.c | 7 ++++++-
> fs/overlayfs/super.c | 13 ++++++++++---
> fs/overlayfs/util.c | 11 ++++++++++-
> 3 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index a6cda02..4876ae4 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -466,7 +466,12 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> err= vfs_removexattr(upperpath.dentry, OVL_XATTR_METACOPY);
> if (err)
> return err;
> -
> + /*
> + * Pairs with smp_rmb() in ovl_dentry_needs_data_copy_up(). Make sure
Nope. only pairs with smp_rmpb() in ovl_d_real() (or in a new helper
you need to create)
> + * if OVL_UPPERDATA flag is visible, then all the write operations
> + * before it are visible as well.
> + */
> + smp_wmb();
> ovl_set_flag(OVL_UPPERDATA, d_inode(c->dentry));
> return err;
> }
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4cf1f98..e97dccb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -102,9 +102,16 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> if (err)
> return ERR_PTR(err);
>
> - if (ovl_dentry_check_upperdata(dentry) &&
> - !ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> - goto lower;
> + if (ovl_dentry_check_upperdata(dentry)) {
> + if (!ovl_test_flag(OVL_UPPERDATA,
> + d_inode(dentry)))
> + goto lower;
> + /*
> + * Pairs with smp_wmb in
> + * ovl_copy_up_meta_inode_data()
> + */
> + smp_rmb();
> + }
> }
> return real;
> }
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index ef720a9..d0f3bf7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -238,6 +238,8 @@ bool ovl_dentry_check_upperdata(struct dentry *dentry) {
> }
>
> bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
> + bool upperdata;
> +
> if (!ovl_dentry_check_upperdata(dentry))
> return false;
>
> @@ -250,7 +252,14 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
> if (!(OPEN_FMODE(flags) & FMODE_WRITE))
> return false;
>
> - if (likely(ovl_test_flag(OVL_UPPERDATA, d_inode(dentry))))
> + upperdata = ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
> + /*
> + * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
> + * if setting of OVL_UPPERDATA is visible, then effects of writes
> + * before that are visible too.
> + */
> + smp_rmb();
> + if (upperdata)
Nope. smp_rmb() is not needed here, because most of the places that
use this helper
will take a lock and call it again under lock.
You may need an explicit smp_rmb() also in getattr() though, so you
can create a new
helper that does exactly what the hunk in ovl_d_real does and reuse
the helper in ovl_getattr
> return false;
>
> return true;
> --
> 2.5.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
