| Message ID | 20200131115004.17410-5-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ovl: allow virtiofs as upper | expand |
On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > No reason to prevent upper layer being a remote filesystem. Do the > revalidation in that case, just as we already do for lower layers. > No reason to prevent upper layer from being a remote filesystem, but the !remote criteria for upper fs kept away a lot of filesystems from upper. Those filesystems have never been tested as upper and many of them are probably not fit for upper. The goal is to lift the !remote limitation, not to allow for lots of new types of upper fs's. What can we do to minimize damages? We can assert that is upper is remote, it must qualify for a more strict criteria as upper fs, that is: - support d_type - support xattr - support RENAME_EXCHANGE|RENAME_WHITEOUT I have a patch on branch ovl-strict which implements those restrictions. Now I know fuse doesn't support RENAME_WHITEOUT, but it does support RENAME_EXCHANGE, which already proves to be a very narrow filter for remote fs: afs, fuse, gfs2. Did not check if afs, gfs2 qualify for the rest of the criteria. Is it simple to implement RENAME_WHITEOUT for fuse/virtiofs? Is it not a problem to rely on an upper fs for modern systems that does not support RENAME_WHITEOUT? Thanks, Amir.
On Fri, Jan 31, 2020 at 4:30 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > > > No reason to prevent upper layer being a remote filesystem. Do the > > revalidation in that case, just as we already do for lower layers. > > > > No reason to prevent upper layer from being a remote filesystem, but > the !remote criteria for upper fs kept away a lot of filesystems from > upper. Those filesystems have never been tested as upper and many > of them are probably not fit for upper. > > The goal is to lift the !remote limitation, not to allow for lots of new > types of upper fs's. > > What can we do to minimize damages? > > We can assert that is upper is remote, it must qualify for a more strict > criteria as upper fs, that is: > - support d_type > - support xattr > - support RENAME_EXCHANGE|RENAME_WHITEOUT > > I have a patch on branch ovl-strict which implements those restrictions. Sounds good. Not sure how much this is this going to be a compatibility headache. If it does, then we can conditionally enable this with a config/module option. > > Now I know fuse doesn't support RENAME_WHITEOUT, but it does > support RENAME_EXCHANGE, which already proves to be a very narrow > filter for remote fs: afs, fuse, gfs2. > Did not check if afs, gfs2 qualify for the rest of the criteria. > > Is it simple to implement RENAME_WHITEOUT for fuse/virtiofs? Trivial. > Is it not a problem to rely on an upper fs for modern systems > that does not support RENAME_WHITEOUT? Limited r/w overlay functionality is still available without whiteout/xattr support, so it could turn out to be something people already rely on. Can't tell without trying... Thanks, Miklos
On Fri, Jan 31, 2020 at 5:38 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Fri, Jan 31, 2020 at 4:30 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > revalidation in that case, just as we already do for lower layers. > > > > > > > No reason to prevent upper layer from being a remote filesystem, but > > the !remote criteria for upper fs kept away a lot of filesystems from > > upper. Those filesystems have never been tested as upper and many > > of them are probably not fit for upper. > > > > The goal is to lift the !remote limitation, not to allow for lots of new > > types of upper fs's. > > > > What can we do to minimize damages? > > > > We can assert that is upper is remote, it must qualify for a more strict > > criteria as upper fs, that is: > > - support d_type > > - support xattr > > - support RENAME_EXCHANGE|RENAME_WHITEOUT > > > > I have a patch on branch ovl-strict which implements those restrictions. > > Sounds good. Not sure how much this is this going to be a > compatibility headache. If it does, then we can conditionally enable > this with a config/module option. > No headache at all: - For now, do not change criteria for !remote fs - Only remote fs needs to meet the most strict criteria - We can add the 'strict' config later if we want impose same criteria also for local fs > > > > Now I know fuse doesn't support RENAME_WHITEOUT, but it does > > support RENAME_EXCHANGE, which already proves to be a very narrow > > filter for remote fs: afs, fuse, gfs2. > > Did not check if afs, gfs2 qualify for the rest of the criteria. > > I checked - afs has d_automount and gfs2 is d_hash. They do not qualify as any layer. > > Is it simple to implement RENAME_WHITEOUT for fuse/virtiofs? > > Trivial. > So that leaves only fuse after implementing RENAME_WHITEOUT. We are back in control ;-) Thanks, Amir.
On Fri, Jan 31, 2020 at 4:50 PM Amir Goldstein <amir73il@gmail.com> wrote: > > I checked - afs has d_automount and gfs2 is d_hash. > They do not qualify as any layer. DCACHE_NEED_AUTOMOUNT doesn't work that way: it's set on specific automount-point dentries only. So AFS won't be disqualified based on that criteria. But afs also does not have RENAME_WHITEOUT, so that's fine. Thanks, MIklos
On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > No reason to prevent upper layer being a remote filesystem. Do the > revalidation in that case, just as we already do for lower layers. > > This lets virtiofs be used as upper layer, which appears to be a real use > case. Hi Miklos, I have couple of very basic questions. - So with this change, we will allow NFS to be upper layer also? - What does revalidation on lower/upper mean? Does that mean that lower/upper can now change underneath overlayfs and overlayfs will cope with it. If we still expect underlying layers not to change, then what's the point of calling ->revalidate(). Thanks Vivek > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/overlayfs/namei.c | 3 +-- > fs/overlayfs/super.c | 8 ++++++-- > fs/overlayfs/util.c | 2 ++ > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index 76e61cc27822..0db23baf98e7 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -845,8 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > if (err) > goto out; > > - if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL || > - unlikely(ovl_dentry_remote(upperdentry)))) { > + if (upperdentry && upperdentry->d_flags & DCACHE_OP_REAL) { > dput(upperdentry); > err = -EREMOTE; > goto out; > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 26d4153240a8..ed3a11db9039 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -135,9 +135,14 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry, > unsigned int flags, bool weak) > { > struct ovl_entry *oe = dentry->d_fsdata; > + struct dentry *upper; > unsigned int i; > int ret = 1; > > + upper = ovl_dentry_upper(dentry); > + if (upper) > + ret = ovl_revalidate_real(upper, flags, weak); > + > for (i = 0; ret > 0 && i < oe->numlower; i++) { > ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags, > weak); > @@ -747,8 +752,7 @@ static int ovl_mount_dir(const char *name, struct path *path) > ovl_unescape(tmp); > err = ovl_mount_dir_noesc(tmp, path); > > - if (!err && (ovl_dentry_remote(path->dentry) || > - path->dentry->d_flags & DCACHE_OP_REAL)) { > + if (!err && path->dentry->d_flags & DCACHE_OP_REAL) { > pr_err("filesystem on '%s' not supported as upperdir\n", > tmp); > path_put_init(path); > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > index 3ad8fb291f7d..c793722739e1 100644 > --- a/fs/overlayfs/util.c > +++ b/fs/overlayfs/util.c > @@ -96,6 +96,8 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry, > struct ovl_entry *oe = OVL_E(dentry); > unsigned int i, flags = 0; > > + if (upperdentry) > + flags |= upperdentry->d_flags; > for (i = 0; i < oe->numlower; i++) > flags |= oe->lowerstack[i].dentry->d_flags; > > -- > 2.21.1 >
On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > No reason to prevent upper layer being a remote filesystem. Do the > > revalidation in that case, just as we already do for lower layers. > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > case. > > Hi Miklos, > > I have couple of very basic questions. > > - So with this change, we will allow NFS to be upper layer also? I haven't tested, but I think it will fail on the d_type test. > - What does revalidation on lower/upper mean? Does that mean that > lower/upper can now change underneath overlayfs and overlayfs will > cope with it. No, that's a more complicated thing. Especially with redirected layers (i.e. revalidating a redirect actually means revalidating all the path components of that redirect). > If we still expect underlying layers not to change, then > what's the point of calling ->revalidate(). That's a good question; I guess because that's what the filesystem expects. OTOH, it's probably unnecessary in most cases, since the path could come from an open file descriptor, in which case the vfs will not do any revalidation on that path. So this is basically done to be on the safe side, but it might not be necessary. Thanks, Miklos > Thanks > Vivek > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > --- > > fs/overlayfs/namei.c | 3 +-- > > fs/overlayfs/super.c | 8 ++++++-- > > fs/overlayfs/util.c | 2 ++ > > 3 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > > index 76e61cc27822..0db23baf98e7 100644 > > --- a/fs/overlayfs/namei.c > > +++ b/fs/overlayfs/namei.c > > @@ -845,8 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > > if (err) > > goto out; > > > > - if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL || > > - unlikely(ovl_dentry_remote(upperdentry)))) { > > + if (upperdentry && upperdentry->d_flags & DCACHE_OP_REAL) { > > dput(upperdentry); > > err = -EREMOTE; > > goto out; > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > index 26d4153240a8..ed3a11db9039 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -135,9 +135,14 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry, > > unsigned int flags, bool weak) > > { > > struct ovl_entry *oe = dentry->d_fsdata; > > + struct dentry *upper; > > unsigned int i; > > int ret = 1; > > > > + upper = ovl_dentry_upper(dentry); > > + if (upper) > > + ret = ovl_revalidate_real(upper, flags, weak); > > + > > for (i = 0; ret > 0 && i < oe->numlower; i++) { > > ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags, > > weak); > > @@ -747,8 +752,7 @@ static int ovl_mount_dir(const char *name, struct path *path) > > ovl_unescape(tmp); > > err = ovl_mount_dir_noesc(tmp, path); > > > > - if (!err && (ovl_dentry_remote(path->dentry) || > > - path->dentry->d_flags & DCACHE_OP_REAL)) { > > + if (!err && path->dentry->d_flags & DCACHE_OP_REAL) { > > pr_err("filesystem on '%s' not supported as upperdir\n", > > tmp); > > path_put_init(path); > > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > > index 3ad8fb291f7d..c793722739e1 100644 > > --- a/fs/overlayfs/util.c > > +++ b/fs/overlayfs/util.c > > @@ -96,6 +96,8 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry, > > struct ovl_entry *oe = OVL_E(dentry); > > unsigned int i, flags = 0; > > > > + if (upperdentry) > > + flags |= upperdentry->d_flags; > > for (i = 0; i < oe->numlower; i++) > > flags |= oe->lowerstack[i].dentry->d_flags; > > > > -- > > 2.21.1 > > >
On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > No reason to prevent upper layer being a remote filesystem. Do the > > > revalidation in that case, just as we already do for lower layers. > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > case. > > > > Hi Miklos, > > > > I have couple of very basic questions. > > > > - So with this change, we will allow NFS to be upper layer also? > > I haven't tested, but I think it will fail on the d_type test. But we do not fail mount on no d_type support... Besides, I though you were going to add the RENAME_WHITEOUT test to avert untested network fs as upper. > > > - What does revalidation on lower/upper mean? Does that mean that > > lower/upper can now change underneath overlayfs and overlayfs will > > cope with it. > > No, that's a more complicated thing. Especially with redirected > layers (i.e. revalidating a redirect actually means revalidating all > the path components of that redirect). > > > If we still expect underlying layers not to change, then > > what's the point of calling ->revalidate(). > > That's a good question; I guess because that's what the filesystem > expects. OTOH, it's probably unnecessary in most cases, since the > path could come from an open file descriptor, in which case the vfs > will not do any revalidation on that path. > Note that ovl_dentry_revalidate() never returns 0 and therefore, vfs will never actually redo the lookup, but instead return -ESTALE to userspace. Right? This makes some sense considering that underlying layers are not expected to change. The question is, with virtiofs, can we know that the server/host will not invalidate entries? And if it does, should it cause a permanent error in overlayfs or a transient error? If we do not want a permanent error, then ->revalidate() needs to be called to invalidate the overlay dentry. No? Thanks, Amir.
On Tue, Feb 04, 2020 at 07:02:05PM +0200, Amir Goldstein wrote: > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > case. > > > > > > Hi Miklos, > > > > > > I have couple of very basic questions. > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > I haven't tested, but I think it will fail on the d_type test. > > But we do not fail mount on no d_type support... > Besides, I though you were going to add the RENAME_WHITEOUT > test to avert untested network fs as upper. > > > > > > - What does revalidation on lower/upper mean? Does that mean that > > > lower/upper can now change underneath overlayfs and overlayfs will > > > cope with it. > > > > No, that's a more complicated thing. Especially with redirected > > layers (i.e. revalidating a redirect actually means revalidating all > > the path components of that redirect). > > > > > If we still expect underlying layers not to change, then > > > what's the point of calling ->revalidate(). > > > > That's a good question; I guess because that's what the filesystem > > expects. OTOH, it's probably unnecessary in most cases, since the > > path could come from an open file descriptor, in which case the vfs > > will not do any revalidation on that path. > > > > Note that ovl_dentry_revalidate() never returns 0 and therefore, vfs > will never actually redo the lookup, but instead return -ESTALE > to userspace. Right? This makes some sense considering that underlying > layers are not expected to change. > > The question is, with virtiofs, can we know that the server/host will not > invalidate entries? I don't think virtiofs will ensure that server/host will not invalidate entries. It will be more of a configuration thing where we will expect users to configure things in such a way that shared directory does not change. So that means, if user does not configure it properly and things change unexpectedly, then overlayfs should be able to detect it and flag error to user space? > And if it does, should it cause a permanent error > in overlayfs or a transient error? If we do not want a permanent error, > then ->revalidate() needs to be called to invalidate the overlay dentry. No? So as of now user space will get -ESTALE and that will get cleared when user space retries after corresponding ovl dentry has been dropped from cache (either dentry is evicted, cache is cleared forcibly or overlayfs is remounted)? If yes, that kind of makes sense. Overlay does not expect underlying layers to change and if a change it detected it is flagged to user space (and overlayfs does not try to fix it)? Thanks Vivek
On Tue, Feb 4, 2020 at 8:42 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Tue, Feb 04, 2020 at 07:02:05PM +0200, Amir Goldstein wrote: > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > > case. > > > > > > > > Hi Miklos, > > > > > > > > I have couple of very basic questions. > > > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > > > I haven't tested, but I think it will fail on the d_type test. > > > > But we do not fail mount on no d_type support... > > Besides, I though you were going to add the RENAME_WHITEOUT > > test to avert untested network fs as upper. > > > > > > > > > - What does revalidation on lower/upper mean? Does that mean that > > > > lower/upper can now change underneath overlayfs and overlayfs will > > > > cope with it. > > > > > > No, that's a more complicated thing. Especially with redirected > > > layers (i.e. revalidating a redirect actually means revalidating all > > > the path components of that redirect). > > > > > > > If we still expect underlying layers not to change, then > > > > what's the point of calling ->revalidate(). > > > > > > That's a good question; I guess because that's what the filesystem > > > expects. OTOH, it's probably unnecessary in most cases, since the > > > path could come from an open file descriptor, in which case the vfs > > > will not do any revalidation on that path. > > > > > > > Note that ovl_dentry_revalidate() never returns 0 and therefore, vfs > > will never actually redo the lookup, but instead return -ESTALE > > to userspace. Right? This makes some sense considering that underlying > > layers are not expected to change. > > > > The question is, with virtiofs, can we know that the server/host will not > > invalidate entries? > > I don't think virtiofs will ensure that server/host will not invalidate > entries. It will be more of a configuration thing where we will expect > users to configure things in such a way that shared directory does not > change. > > So that means, if user does not configure it properly and things change > unexpectedly, then overlayfs should be able to detect it and flag error > to user space? > > > And if it does, should it cause a permanent error > > in overlayfs or a transient error? If we do not want a permanent error, > > then ->revalidate() needs to be called to invalidate the overlay dentry. No? > > So as of now user space will get -ESTALE and that will get cleared when > user space retries after corresponding ovl dentry has been dropped from > cache (either dentry is evicted, cache is cleared forcibly or overlayfs > is remounted)? If yes, that kind of makes sense. Overlay does not expect > underlying layers to change and if a change it detected it is flagged > to user space (and overlayfs does not try to fix it)? > I looks like it. I don't really understand why overlayfs shouldn't drop the dentry on failure to revalidate. Maybe I am missing something. Thanks, Amir.
On Tue, Feb 4, 2020 at 8:11 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Feb 4, 2020 at 8:42 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > So as of now user space will get -ESTALE and that will get cleared when > > user space retries after corresponding ovl dentry has been dropped from > > cache (either dentry is evicted, cache is cleared forcibly or overlayfs > > is remounted)? If yes, that kind of makes sense. Overlay does not expect > > underlying layers to change and if a change it detected it is flagged > > to user space (and overlayfs does not try to fix it)? > > > > I looks like it. I don't really understand why overlayfs shouldn't drop > the dentry on failure to revalidate. Maybe I am missing something. I don't remember the exact reason. Maybe it's just that it makes little sense to redo the lookup on remote change, but not on local change... Note: I'm not against detection of changing underlying layers and redoing the lookup in that case. Maybe we can do it optionally (because it could be expensive), but first there needs to be a use case and we seem to lack that. Thanks, Miklos
On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > case. > > > > > > Hi Miklos, > > > > > > I have couple of very basic questions. > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > I haven't tested, but I think it will fail on the d_type test. > > But we do not fail mount on no d_type support... > Besides, I though you were going to add the RENAME_WHITEOUT > test to avert untested network fs as upper. > Pushed strict remote upper check to: https://github.com/amir73il/linux/commits/ovl-strict-upper FWIW, overlayfs-next+ovl-strict-upper passes the quick xfstests, except for overlay/031 - it fails because the RENAME_WHITEOUT check leaves behind a whiteout in workdir. I think it it is not worth to cleanup that whiteout leftover and easier to fix the test. Thanks, Amir.
On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > > case. > > > > > > > > Hi Miklos, > > > > > > > > I have couple of very basic questions. > > > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > > > I haven't tested, but I think it will fail on the d_type test. > > > > But we do not fail mount on no d_type support... > > Besides, I though you were going to add the RENAME_WHITEOUT > > test to avert untested network fs as upper. > > > > Pushed strict remote upper check to: > https://github.com/amir73il/linux/commits/ovl-strict-upper > > FWIW, overlayfs-next+ovl-strict-upper passes the quick xfstests, > except for overlay/031 - it fails because the RENAME_WHITEOUT check > leaves behind a whiteout in workdir. > I think it it is not worth to cleanup that whiteout leftover and > easier to fix the test. Nevermind. Fixed the whiteout cleanup and re-pushed. Thanks, Amir.
On Thu, Feb 20, 2020 at 10:00 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > > > case. > > > > > > > > > > Hi Miklos, > > > > > > > > > > I have couple of very basic questions. > > > > > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > > > > > I haven't tested, but I think it will fail on the d_type test. > > > > > > But we do not fail mount on no d_type support... > > > Besides, I though you were going to add the RENAME_WHITEOUT > > > test to avert untested network fs as upper. > > > > > > > Pushed strict remote upper check to: > > https://github.com/amir73il/linux/commits/ovl-strict-upper > > Vivek, Could you please make sure that the code in ovl-strict-upper branch works as expected for virtio as upper fs? I have rebased it on latest overlayfs-next merge into current master. I would very much prefer that the code merged to v5.7-rc1 will be more restrictive than the current overlayfs-next. Thanks, Amir.
On Sat, Mar 14, 2020 at 03:16:28PM +0200, Amir Goldstein wrote: > On Thu, Feb 20, 2020 at 10:00 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > > > > case. > > > > > > > > > > > > Hi Miklos, > > > > > > > > > > > > I have couple of very basic questions. > > > > > > > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > > > > > > > I haven't tested, but I think it will fail on the d_type test. > > > > > > > > But we do not fail mount on no d_type support... > > > > Besides, I though you were going to add the RENAME_WHITEOUT > > > > test to avert untested network fs as upper. > > > > > > > > > > Pushed strict remote upper check to: > > > https://github.com/amir73il/linux/commits/ovl-strict-upper > > > > > Vivek, > > Could you please make sure that the code in ovl-strict-upper branch > works as expected for virtio as upper fs? Hi Amir, Right now it fails becuase virtiofs doesn't seem to support tmpfile yet. overlayfs: upper fs does not support tmpfile overlayfs: upper fs missing required features. Will have to check what's required to support it. I also wanted to run either overlay xfstests or unionmount-testsuite. But none of these seem to give me enough flexibility where I can specify that overlayfs needs to be mounted on top of virtiofs. I feel that atleast for unionmount-testsuite, there should be an option where we can simply give a target directory and tests run on that directory and user mounts that directory as needed. > I have rebased it on latest overlayfs-next merge into current master. > > I would very much prefer that the code merged to v5.7-rc1 will be more > restrictive than the current overlayfs-next. In general I agree that if we want to not support some configuration with remote upper, this is the time to introduce that restriction otherwise we will later run into backward compatibility issue. Having said that, tmpfile support for upper sounds like a nice to have feature. Not sure why to make it mandatory. Vivek
On Mon, Mar 16, 2020 at 8:15 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Sat, Mar 14, 2020 at 03:16:28PM +0200, Amir Goldstein wrote: > > On Thu, Feb 20, 2020 at 10:00 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > > > > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote: > > > > > > > > No reason to prevent upper layer being a remote filesystem. Do the > > > > > > > > revalidation in that case, just as we already do for lower layers. > > > > > > > > > > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use > > > > > > > > case. > > > > > > > > > > > > > > Hi Miklos, > > > > > > > > > > > > > > I have couple of very basic questions. > > > > > > > > > > > > > > - So with this change, we will allow NFS to be upper layer also? > > > > > > > > > > > > I haven't tested, but I think it will fail on the d_type test. > > > > > > > > > > But we do not fail mount on no d_type support... > > > > > Besides, I though you were going to add the RENAME_WHITEOUT > > > > > test to avert untested network fs as upper. > > > > > > > > > > > > > Pushed strict remote upper check to: > > > > https://github.com/amir73il/linux/commits/ovl-strict-upper > > > > > > > > Vivek, > > > > Could you please make sure that the code in ovl-strict-upper branch > > works as expected for virtio as upper fs? > > Hi Amir, > > Right now it fails becuase virtiofs doesn't seem to support tmpfile yet. > > overlayfs: upper fs does not support tmpfile > overlayfs: upper fs missing required features. > > Will have to check what's required to support it. > > I also wanted to run either overlay xfstests or unionmount-testsuite. But > none of these seem to give me enough flexibility where I can specify > that overlayfs needs to be mounted on top of virtiofs. > > I feel that atleast for unionmount-testsuite, there should be an > option where we can simply give a target directory and tests run > on that directory and user mounts that directory as needed. > Need to see how patches look. Don't want too much configuration complexity, but I agree that some flexibly is needed. Maybe the provided target directory should be the upper/work basedir? > > I have rebased it on latest overlayfs-next merge into current master. > > > > I would very much prefer that the code merged to v5.7-rc1 will be more > > restrictive than the current overlayfs-next. > > In general I agree that if we want to not support some configuration > with remote upper, this is the time to introduce that restriction > otherwise we will later run into backward compatibility issue. > > Having said that, tmpfile support for upper sounds like a nice to > have feature. Not sure why to make it mandatory. > Agreed, I just went automatic on all the warnings. tmpfile should not be a requirement for upper. Could you please verify that if dropping the tmpfile strict check, virtio can be used as upper. Thanks, Amir.
On Mon, Mar 16, 2020 at 09:02:32PM +0200, Amir Goldstein wrote: [..] > > > Could you please make sure that the code in ovl-strict-upper branch > > > works as expected for virtio as upper fs? > > > > Hi Amir, > > > > Right now it fails becuase virtiofs doesn't seem to support tmpfile yet. > > > > overlayfs: upper fs does not support tmpfile > > overlayfs: upper fs missing required features. > > > > Will have to check what's required to support it. > > > > I also wanted to run either overlay xfstests or unionmount-testsuite. But > > none of these seem to give me enough flexibility where I can specify > > that overlayfs needs to be mounted on top of virtiofs. > > > > I feel that atleast for unionmount-testsuite, there should be an > > option where we can simply give a target directory and tests run > > on that directory and user mounts that directory as needed. > > > > Need to see how patches look. > Don't want too much configuration complexity, but I agree that some > flexibly is needed. > Maybe the provided target directory should be the upper/work basedir? > > > > I have rebased it on latest overlayfs-next merge into current master. > > > > > > I would very much prefer that the code merged to v5.7-rc1 will be more > > > restrictive than the current overlayfs-next. > > > > In general I agree that if we want to not support some configuration > > with remote upper, this is the time to introduce that restriction > > otherwise we will later run into backward compatibility issue. > > > > Having said that, tmpfile support for upper sounds like a nice to > > have feature. Not sure why to make it mandatory. > > > > Agreed, I just went automatic on all the warnings. > tmpfile should not be a requirement for upper. > Could you please verify that if dropping the tmpfile strict check, > virtio can be used as upper. I dropped tmpfile strict check and now I can mount overlayfs using virtiofs as upper. Tried few basic file operations and these are working. Vivek
> > I also wanted to run either overlay xfstests or unionmount-testsuite. But > > none of these seem to give me enough flexibility where I can specify > > that overlayfs needs to be mounted on top of virtiofs. > > > > I feel that atleast for unionmount-testsuite, there should be an > > option where we can simply give a target directory and tests run > > on that directory and user mounts that directory as needed. > > > > Need to see how patches look. > Don't want too much configuration complexity, but I agree that some > flexibly is needed. > Maybe the provided target directory should be the upper/work basedir? > Vivek, I was going to see what's the best way to add the needed flexibility, but then I realized I had already implemented this undocumented feature. I have been using this to test overlay over XFS as documented here: https://github.com/amir73il/overlayfs/wiki/Overlayfs-testing#Setup_overlayfs_mount_over_XFS_with_reflink_support That's an example of how to configure a custom /base mount for --samefs to be xfs. Similar hidden feature exists for configuring a custom /lower and /upper mounts via fstab, but I don't think I ever tested those, so not sure if they work as expected. unionmount testsuite will first try to mount the entry from fstab and fallback to mounting tmpfs. I admit this a lousy configuration method, but we could make it official using env vars or something. I also never liked the fact that unionmount testsuite hard codes the /lower /upper /mnt paths. The reason we 'need' the instructions how to mount the fs as opposed to an already mounted dir is that unmounting the underlying fs exposes dentry/inode reference leaks by overlayfs. But it is nice to have and xfstests has support for configuring an already mounted overlayfs for the generic tests. So if you think that you cannot use the existing hack and that pointing to an already mounted /upper or mounted overlay is needed, I suggest that you experiment with patches to make that change. Thanks, Amir.
On Wed, Mar 18, 2020 at 03:36:44PM +0200, Amir Goldstein wrote: > > > I also wanted to run either overlay xfstests or unionmount-testsuite. But > > > none of these seem to give me enough flexibility where I can specify > > > that overlayfs needs to be mounted on top of virtiofs. > > > > > > I feel that atleast for unionmount-testsuite, there should be an > > > option where we can simply give a target directory and tests run > > > on that directory and user mounts that directory as needed. > > > > > > > Need to see how patches look. > > Don't want too much configuration complexity, but I agree that some > > flexibly is needed. > > Maybe the provided target directory should be the upper/work basedir? > > > > Vivek, > > I was going to see what's the best way to add the needed flexibility, > but then I realized I had already implemented this undocumented > feature. > > I have been using this to test overlay over XFS as documented here: > https://github.com/amir73il/overlayfs/wiki/Overlayfs-testing#Setup_overlayfs_mount_over_XFS_with_reflink_support > > That's an example of how to configure a custom /base mount for > --samefs to be xfs. Hi Amir, This seems to work for me. Thanks for the idea. I put following entries in /etc/fstab. myfs /mnt/virtiofs-lower_layer virtiofs defaults 0 0 /mnt/virtiofs-lower_layer /base none bind 0 0 After that tests seem to start but soon I hit failures. Now its time to debug the failures one at a time. Vivek
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 76e61cc27822..0db23baf98e7 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -845,8 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, if (err) goto out; - if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL || - unlikely(ovl_dentry_remote(upperdentry)))) { + if (upperdentry && upperdentry->d_flags & DCACHE_OP_REAL) { dput(upperdentry); err = -EREMOTE; goto out; diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 26d4153240a8..ed3a11db9039 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -135,9 +135,14 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry, unsigned int flags, bool weak) { struct ovl_entry *oe = dentry->d_fsdata; + struct dentry *upper; unsigned int i; int ret = 1; + upper = ovl_dentry_upper(dentry); + if (upper) + ret = ovl_revalidate_real(upper, flags, weak); + for (i = 0; ret > 0 && i < oe->numlower; i++) { ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags, weak); @@ -747,8 +752,7 @@ static int ovl_mount_dir(const char *name, struct path *path) ovl_unescape(tmp); err = ovl_mount_dir_noesc(tmp, path); - if (!err && (ovl_dentry_remote(path->dentry) || - path->dentry->d_flags & DCACHE_OP_REAL)) { + if (!err && path->dentry->d_flags & DCACHE_OP_REAL) { pr_err("filesystem on '%s' not supported as upperdir\n", tmp); path_put_init(path); diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 3ad8fb291f7d..c793722739e1 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -96,6 +96,8 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry, struct ovl_entry *oe = OVL_E(dentry); unsigned int i, flags = 0; + if (upperdentry) + flags |= upperdentry->d_flags; for (i = 0; i < oe->numlower; i++) flags |= oe->lowerstack[i].dentry->d_flags;
No reason to prevent upper layer being a remote filesystem. Do the revalidation in that case, just as we already do for lower layers. This lets virtiofs be used as upper layer, which appears to be a real use case. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/overlayfs/namei.c | 3 +-- fs/overlayfs/super.c | 8 ++++++-- fs/overlayfs/util.c | 2 ++ 3 files changed, 9 insertions(+), 4 deletions(-)