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