shiftfs status and future development

Submitted by Seth Forshee on June 15, 2018, 3:46 p.m.

Details

Message ID 20180615154641.GH30028@ubuntu-xps13
State New
Series "shiftfs status and future development"
Headers show

Commit Message

Seth Forshee June 15, 2018, 3:46 p.m.
On Fri, Jun 15, 2018 at 08:28:54AM -0700, James Bottomley wrote:
> On Thu, 2018-06-14 at 13:44 -0500, Seth Forshee wrote:
> > I wanted to inquire about the current status of shiftfs and the plans
> > for it moving forward. We'd like to have this functionality available
> > for use in lxd, and I'm interesetd in helping with development (or
> > picking up development if it's stalled).
> 
> Well, I'm still using it and technically still developing it; it's just
> that its self contained and there haven't been any interface changes
> necessitating an update that I've noticed, so the last published patch
> still applies (well, unless my git rebasing quietly changed something
> and I didn't notice).

I did have to make some changes when I tried it out with 4.17, see
below.

> > To start, is anyone still working on shiftfs or similar
> > functionality? I haven't found it in any git tree on kernel.org, and
> > as far as mailing list activity the last submission I can find is
> > [1]. Is there anything newer than this?
> 
> I'm working on it, but it mostly works for my use case.
> 
> > Based on past mailing list discussions, it seems like there was still
> > debate as to whether this feature should be an overlay filesystem or
> > something supported at the vfs level. Was this ever resolved?
> 
> I think that's the main problem: I don't really have anything
> actionable to work on for upstreaming.  I suspect growing more
> consumers would help with this.

Kind of a chicken/egg problem, as it's difficult to grow consumers when
it's not upstream. I'm starting to work with it though.

However it seems to me that the question of whether or not this should
be an overlay filesystem is pretty fundamental. There was a fairly long
thread between you and Christoph Hellwig on the topic that didn't really
seem to come to a resolution.

Thanks,
Seth


From 70352c549e9c023cd0c0732329a44f5bbcac0ace Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Thu, 24 May 2018 09:40:13 -0500
Subject: [PATCH] shiftfs: Forward port to 4.17-rc6

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/shiftfs.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index ea8ac57b3ce1..fa0797bd7880 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -109,12 +109,12 @@  static void shiftfs_d_release(struct dentry *dentry)
 
 static struct dentry *shiftfs_d_real(struct dentry *dentry,
 				     const struct inode *inode,
-				     unsigned int flags)
+				     unsigned int open_flags, unsigned int flags)
 {
 	struct dentry *real = dentry->d_fsdata;
 
 	if (unlikely(real->d_flags & DCACHE_OP_REAL))
-		return real->d_op->d_real(real, real->d_inode, flags);
+		return real->d_op->d_real(real, real->d_inode, open_flags, flags);
 
 	return real;
 }
@@ -545,19 +545,20 @@  static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
 	return 0;
 }
 
-static int shiftfs_getattr(struct vfsmount *mnt, struct dentry *dentry,
-			   struct kstat *stat)
+static int shiftfs_getattr(const struct path *path, struct kstat *stat,
+			   u32 request_mask, unsigned int flags)
 {
+	struct dentry *dentry = path->dentry;
+	struct shiftfs_super_info *ssi = dentry->d_sb->s_fs_info;
 	struct inode *inode = dentry->d_inode;
 	struct dentry *real = inode->i_private;
 	struct inode *reali = real->d_inode;
 	const struct inode_operations *iop = reali->i_op;
+	struct path realpath = { .mnt = ssi->mnt, .dentry = real };
 	int err = 0;
 
-	mnt = dentry->d_sb->s_fs_info;
-
 	if (iop->getattr)
-		err = iop->getattr(mnt, real, stat);
+		err = iop->getattr(&realpath, stat, request_mask, flags);
 	else
 		generic_fillattr(reali, stat);
 
@@ -625,7 +626,7 @@  static struct inode *shiftfs_new_inode(struct super_block *sb, umode_t mode,
 	 * may be aliases plus a few others.
 	 */
 	if (reali)
-		use_inode_hash = ACCESS_ONCE(reali->i_nlink) > 1 &&
+		use_inode_hash = READ_ONCE(reali->i_nlink) > 1 &&
 			!S_ISDIR(reali->i_mode);
 
 	if (use_inode_hash) {

Comments

Christian Brauner June 15, 2018, 4:16 p.m.
On Fri, Jun 15, 2018 at 10:46:41AM -0500, Seth Forshee wrote:
> On Fri, Jun 15, 2018 at 08:28:54AM -0700, James Bottomley wrote:
> > On Thu, 2018-06-14 at 13:44 -0500, Seth Forshee wrote:
> > > I wanted to inquire about the current status of shiftfs and the plans
> > > for it moving forward. We'd like to have this functionality available
> > > for use in lxd, and I'm interesetd in helping with development (or
> > > picking up development if it's stalled).
> > 
> > Well, I'm still using it and technically still developing it; it's just
> > that its self contained and there haven't been any interface changes
> > necessitating an update that I've noticed, so the last published patch
> > still applies (well, unless my git rebasing quietly changed something
> > and I didn't notice).
> 
> I did have to make some changes when I tried it out with 4.17, see
> below.
> 
> > > To start, is anyone still working on shiftfs or similar
> > > functionality? I haven't found it in any git tree on kernel.org, and
> > > as far as mailing list activity the last submission I can find is
> > > [1]. Is there anything newer than this?
> > 
> > I'm working on it, but it mostly works for my use case.
> > 
> > > Based on past mailing list discussions, it seems like there was still
> > > debate as to whether this feature should be an overlay filesystem or
> > > something supported at the vfs level. Was this ever resolved?
> > 
> > I think that's the main problem: I don't really have anything
> > actionable to work on for upstreaming.  I suspect growing more
> > consumers would help with this.
> 
> Kind of a chicken/egg problem, as it's difficult to grow consumers when

Exactly, but if we have it it'll basically enable a whole bunch of
usecases all at once. This is an essential feature to make user
namespaces easier to use without compromising on security. Right now, to
be able to write to a mapped fstree the easiest way is to punch a hole
for the {g,u}id in question into the idmap but that is obviously not
ideal. It won't matter that much if the uid in question is 1000 but if
your uid is 0 it very much does.

> it's not upstream. I'm starting to work with it though.
> 
> However it seems to me that the question of whether or not this should
> be an overlay filesystem is pretty fundamental. There was a fairly long

Yes, I think this is something we need to settle first and we have
already been going back and forth on this. An rough agreement on what
the preferred solution is would be good. There'll surely also be a
conference where we can present this in more detail and also have a
face-to-face discussion.

> thread between you and Christoph Hellwig on the topic that didn't really

I'M CCing Christoph. He had some good insights in prior threads.

Christian

> seem to come to a resolution.
> 
> Thanks,
> Seth
> 
> 
> From 70352c549e9c023cd0c0732329a44f5bbcac0ace Mon Sep 17 00:00:00 2001
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Thu, 24 May 2018 09:40:13 -0500
> Subject: [PATCH] shiftfs: Forward port to 4.17-rc6
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  fs/shiftfs.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index ea8ac57b3ce1..fa0797bd7880 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -109,12 +109,12 @@ static void shiftfs_d_release(struct dentry *dentry)
>  
>  static struct dentry *shiftfs_d_real(struct dentry *dentry,
>  				     const struct inode *inode,
> -				     unsigned int flags)
> +				     unsigned int open_flags, unsigned int flags)
>  {
>  	struct dentry *real = dentry->d_fsdata;
>  
>  	if (unlikely(real->d_flags & DCACHE_OP_REAL))
> -		return real->d_op->d_real(real, real->d_inode, flags);
> +		return real->d_op->d_real(real, real->d_inode, open_flags, flags);
>  
>  	return real;
>  }
> @@ -545,19 +545,20 @@ static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
>  	return 0;
>  }
>  
> -static int shiftfs_getattr(struct vfsmount *mnt, struct dentry *dentry,
> -			   struct kstat *stat)
> +static int shiftfs_getattr(const struct path *path, struct kstat *stat,
> +			   u32 request_mask, unsigned int flags)
>  {
> +	struct dentry *dentry = path->dentry;
> +	struct shiftfs_super_info *ssi = dentry->d_sb->s_fs_info;
>  	struct inode *inode = dentry->d_inode;
>  	struct dentry *real = inode->i_private;
>  	struct inode *reali = real->d_inode;
>  	const struct inode_operations *iop = reali->i_op;
> +	struct path realpath = { .mnt = ssi->mnt, .dentry = real };
>  	int err = 0;
>  
> -	mnt = dentry->d_sb->s_fs_info;
> -
>  	if (iop->getattr)
> -		err = iop->getattr(mnt, real, stat);
> +		err = iop->getattr(&realpath, stat, request_mask, flags);
>  	else
>  		generic_fillattr(reali, stat);
>  
> @@ -625,7 +626,7 @@ static struct inode *shiftfs_new_inode(struct super_block *sb, umode_t mode,
>  	 * may be aliases plus a few others.
>  	 */
>  	if (reali)
> -		use_inode_hash = ACCESS_ONCE(reali->i_nlink) > 1 &&
> +		use_inode_hash = READ_ONCE(reali->i_nlink) > 1 &&
>  			!S_ISDIR(reali->i_mode);
>  
>  	if (use_inode_hash) {
> -- 
> 2.17.0
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
James Bottomley June 15, 2018, 4:35 p.m.
On Fri, 2018-06-15 at 10:46 -0500, Seth Forshee wrote:
[...]
> Subject: [PATCH] shiftfs: Forward port to 4.17-rc6
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  fs/shiftfs.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index ea8ac57b3ce1..fa0797bd7880 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -109,12 +109,12 @@ static void shiftfs_d_release(struct dentry
> *dentry)
>  
>  static struct dentry *shiftfs_d_real(struct dentry *dentry,
>  				     const struct inode *inode,
> -				     unsigned int flags)
> +				     unsigned int open_flag

Well, that's an oopsie: I already have this change in my code base.  I
think I probably did it at the last minute for a conference
presentation (the log indicates it was done in the 4.14 timeframe) and
then forgot to repost the patches, which would explain why it's still
nicely compiling for me ...

I can repost so we've got code to hang the discussion on.

James
Seth Forshee June 15, 2018, 8:17 p.m.
On Fri, Jun 15, 2018 at 09:35:49AM -0700, James Bottomley wrote:
> On Fri, 2018-06-15 at 10:46 -0500, Seth Forshee wrote:
> [...]
> > Subject: [PATCH] shiftfs: Forward port to 4.17-rc6
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  fs/shiftfs.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> > index ea8ac57b3ce1..fa0797bd7880 100644
> > --- a/fs/shiftfs.c
> > +++ b/fs/shiftfs.c
> > @@ -109,12 +109,12 @@ static void shiftfs_d_release(struct dentry
> > *dentry)
> >  
> >  static struct dentry *shiftfs_d_real(struct dentry *dentry,
> >  				     const struct inode *inode,
> > -				     unsigned int flags)
> > +				     unsigned int open_flag
> 
> Well, that's an oopsie: I already have this change in my code base.  I
> think I probably did it at the last minute for a conference
> presentation (the log indicates it was done in the 4.14 timeframe) and
> then forgot to repost the patches, which would explain why it's still
> nicely compiling for me ...
> 
> I can repost so we've got code to hang the discussion on.

That would be great, thanks!

Seth