On Sunday, June 11, 2017 9:03:54 PM IST Amir Goldstein wrote:
> On Sun, Jun 11, 2017 at 4:44 PM, Chandan Rajendra
> <chandan@xxxxxxxxxxxxxxxxxx> wrote:
> > The following sequence of commands causes a lowerdir file and an
> > overlay file to have the same values for st_ino/st_dev,
>
> You meant different st_dev values...
>
> All in all, looks very good to me. Nice work!
> See some nits and suggestions below.
>
> Did you get any progress with tests yet?
Thanks for the review comments.
I spent some time to read and figure out the workings of
unionmount-testsuite framework. I should be able to post the
the new test along with the non-RFC version of this patch.
>
> >
> > $ xfs_io -f -c "pwrite 0 4k" /mnt/lowerdir/testfile -c sync
> > $ xfs_io -a -c "pwrite 4k 4k" /mnt/ovl/testfile # Copy-up occurs here
>
> Honestly, I think you could find a simpler example and also one that
> demonstrates
> the bug, something like:
>
> $ echo 123 > /mnt/lowerdir/testfile
> $ echo 456 > /mnt/ovl/testfile
> $ cat /mnt/lowerdir/testfile /mnt/ovl/testfile
> 123
> 456
> $ diff /mnt/lowerdir/testfile /mnt/ovl/testfile && echo "123 == 456 ???"
>
> But that diff example will only demonstrate a bug on top of my branch,
> and I don't think that is the right order for this patch. it should
> come *before*
> the relax samefs patch, so maybe just drop the example?
Ok. Understood. I will do that.
>
> >
> > This commit solves the bug by reporting pseudo device numbers for files
> > on lowerdir.
> >
> > Signed-off-by: Chandan Rajendra <chandan@xxxxxxxxxxxxxxxxxx>
> > ---
> > fs/overlayfs/inode.c | 23 ++++++++++++++++++++---
> > fs/overlayfs/ovl_entry.h | 8 +++++++-
> > fs/overlayfs/super.c | 33 +++++++++++++++++++++++++--------
> > 3 files changed, 52 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 5f285c1..154c87892 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -13,6 +13,19 @@
> > #include <linux/xattr.h>
> > #include <linux/posix_acl.h>
> > #include "overlayfs.h"
> > +#include "ovl_entry.h"
> > +
> > +static dev_t get_lowerdir_pseudo_dev(struct ovl_fs *ufs, dev_t dev)
>
> I suggest to call this ovl_get_pseudo_dev (see below why) and
> I suggest to check if dev == upper real dev and return the overlay dev.
Ok. I agree.
>
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ufs->numlower; i++) {
> > + if (ufs->lower_mnt[i].real_dev == dev)
> > + return ufs->lower_mnt[i].pseudo_dev;
> > + }
> > +
> > + BUG();
>
> No need to BUG() here WARN() and return dev will not cause any
> catastrophic bug. overlay will just behave as it does today..
I agree.
>
> > +}
> >
> > int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> > {
> > @@ -61,6 +74,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> > u32 request_mask, unsigned int flags)
> > {
> > struct dentry *dentry = path->dentry;
> > + struct ovl_fs *ufs;
>
> I prefer ofs. The use of ufs is mostly when the info is not yet attached to sb.
> Not sure if this was intentional, but its convenient.
I will fix this.
>
> > enum ovl_path_type type;
> > struct path realpath;
> > const struct cred *old_cred;
> > @@ -103,10 +117,13 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> > if (is_dir || lowerstat.nlink == 1)
> > stat->ino = lowerstat.ino;
> >
> > - if (samefs)
> > + if (samefs) {
> > WARN_ON_ONCE(stat->dev != lowerstat.dev);
> > - else
> > - stat->dev = lowerstat.dev;
> > + } else {
> > + ufs = dentry->d_sb->s_fs_info;
> > + stat->dev = get_lowerdir_pseudo_dev(ufs,
> > + lowerstat.dev);
>
> if (!is_dir && !samefs) { stat->dev = ovl_get_pseudo_dev ... }
> should be outside the OVL_TYPE_ORIGIN condition, because
> it also applies to pure lower and pure upper inodes (not copied up).
>
> For pure upper it is just nicer and du -xs will work better on
> pure upper dirs.
> For lower, it is a must, because overlay inode needs to preserve
> st_dev across copy up, so it needs to have the pseudo lower st_dev
> before that lower becomes the origin of a copy up.
I will fix this.
>
>
> > + }
> > }
> > if (samefs)
> > stat->dev = dentry->d_sb->s_dev;
>
>
--
chandan
--
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
