| Message ID | 1484488652-611-2-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote: > Factor out some common vfs bits from do_tmpfile() > to be used by overlayfs for concurrent copy up. I'm wondering whether the vfs helper should do everything except the path lookup and the open: d_alloc(), ->tmpfile() and setting I_LINKABLE. This will also aid in doing a ->tmpfile() for overlayfs. Thanks, Miklos > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/namei.c | 26 ++++++++++++++++---------- > include/linux/fs.h | 1 + > 2 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index ad74877..4cbaf54 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3353,6 +3353,20 @@ static int do_last(struct nameidata *nd, > return error; > } > > +int vfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) > +{ > + int error; > + > + /* we want directory to be writable */ > + error = inode_permission(dir, MAY_WRITE | MAY_EXEC); > + if (error) > + return error; > + if (!dir->i_op->tmpfile) > + return -EOPNOTSUPP; > + return dir->i_op->tmpfile(dir, dentry, mode); > +} > +EXPORT_SYMBOL(vfs_tmpfile); > + > static int do_tmpfile(struct nameidata *nd, unsigned flags, > const struct open_flags *op, > struct file *file, int *opened) > @@ -3368,14 +3382,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, > if (unlikely(error)) > goto out; > dir = path.dentry->d_inode; > - /* we want directory to be writable */ > - error = inode_permission(dir, MAY_WRITE | MAY_EXEC); > - if (error) > - goto out2; > - if (!dir->i_op->tmpfile) { > - error = -EOPNOTSUPP; > - goto out2; > - } > child = d_alloc(path.dentry, &name); > if (unlikely(!child)) { > error = -ENOMEM; > @@ -3383,8 +3389,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, > } > dput(path.dentry); > path.dentry = child; > - error = dir->i_op->tmpfile(dir, child, op->mode); > - if (error) > + error = vfs_tmpfile(dir, child, op->mode); > + if (unlikely(error)) > goto out2; > audit_inode(nd->name, child, 0); > /* Don't check for other permissions, the inode was just created */ > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 2ba0743..13f56f1 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1560,6 +1560,7 @@ extern int vfs_rmdir(struct inode *, struct dentry *); > extern int vfs_unlink(struct inode *, struct dentry *, struct inode **); > extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **, unsigned int); > extern int vfs_whiteout(struct inode *, struct dentry *); > +extern int vfs_tmpfile(struct inode *, struct dentry *, umode_t); > > /* > * VFS file helper functions. > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 16, 2017 at 1:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> Factor out some common vfs bits from do_tmpfile() >> to be used by overlayfs for concurrent copy up. > > I'm wondering whether the vfs helper should do everything except the > path lookup and the open: d_alloc(), ->tmpfile() and setting > I_LINKABLE. This will also aid in doing a ->tmpfile() for overlayfs. > I started with that, but slowly trimmed it down to this minimal version. First, mnt_want_write() can't be in there. Then dentry * return value would be a strange deviation from other vfs_ helpers. Lastly, all the open related operations are already performed by ovl_path_open() and I did not want to make an exception, so I resorted to doing d_alloc in ovl_alloc_tmpfile(), which becomes a drop-in replacement for ovl_lookup_temp() and set LINKABLE in ovl_link_tmpfile(), which becomes a drop-in replacement for ovl_do_rename(). -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 16, 2017 at 12:19 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Mon, Jan 16, 2017 at 1:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> Factor out some common vfs bits from do_tmpfile() >>> to be used by overlayfs for concurrent copy up. >> >> I'm wondering whether the vfs helper should do everything except the >> path lookup and the open: d_alloc(), ->tmpfile() and setting >> I_LINKABLE. This will also aid in doing a ->tmpfile() for overlayfs. >> > > I started with that, but slowly trimmed it down to this minimal version. > First, mnt_want_write() can't be in there. > Then dentry * return value would be a strange deviation from other vfs_ helpers. What about lookup_one_len()? It's not called vfs_something but that's beside the point, I think. > Lastly, all the open related operations are already performed by > ovl_path_open() and I did not want to make an exception, > so I resorted to doing d_alloc in ovl_alloc_tmpfile(), which becomes > a drop-in replacement for ovl_lookup_temp() and set LINKABLE in > ovl_link_tmpfile(), which becomes a drop-in replacement for ovl_do_rename(). The logical place to set I_LINKABLE is in vfs_tmpfile(). You are arguing about overlayfs code structure, but that's mostly irrelevant when doing a new vfs interface. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 16, 2017 at 3:22 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Mon, Jan 16, 2017 at 12:19 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Mon, Jan 16, 2017 at 1:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> Factor out some common vfs bits from do_tmpfile() >>>> to be used by overlayfs for concurrent copy up. >>> >>> I'm wondering whether the vfs helper should do everything except the >>> path lookup and the open: d_alloc(), ->tmpfile() and setting >>> I_LINKABLE. This will also aid in doing a ->tmpfile() for overlayfs. >>> >> >> I started with that, but slowly trimmed it down to this minimal version. >> First, mnt_want_write() can't be in there. >> Then dentry * return value would be a strange deviation from other vfs_ helpers. > > What about lookup_one_len()? It's not called vfs_something but that's > beside the point, I think. > >> Lastly, all the open related operations are already performed by >> ovl_path_open() and I did not want to make an exception, >> so I resorted to doing d_alloc in ovl_alloc_tmpfile(), which becomes >> a drop-in replacement for ovl_lookup_temp() and set LINKABLE in >> ovl_link_tmpfile(), which becomes a drop-in replacement for ovl_do_rename(). > > The logical place to set I_LINKABLE is in vfs_tmpfile(). You are > arguing about overlayfs code structure, but that's mostly irrelevant > when doing a new vfs interface. > What I tried to do at first is leave d_alloc inside vfs_tmpfile() and return dentry, but I did not want this interface to deal with struct file as well. so do_tmpfile() is very roughly equivalent to: path_lookupat() mnt_want_write() vfs_tmpfile() (including d_alloc) dentry_open() inode->i_state |= I_LINKABLE So I can set LINKABLE inside vfs_tmpfile() only if I also pass it open_flag and then it would also happen before open returns success. My intention was to keep the VFS patch as boring and uncontroversial as possible. I am tempted to say: could you possibly create that extra re-factoring yourself either before or after this patch set? You are the one who is going to be selling it to Al after all... Either that, or just send me better guidelines and I'll do the leg work myself. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 16, 2017 at 3:04 PM, Amir Goldstein <amir73il@gmail.com> wrote: > What I tried to do at first is leave d_alloc inside vfs_tmpfile() > and return dentry, but I did not want this interface to deal with struct file > as well. Agreed, I think you should do that. > > so do_tmpfile() is very roughly equivalent to: > path_lookupat() > mnt_want_write() > vfs_tmpfile() (including d_alloc) > dentry_open() > inode->i_state |= I_LINKABLE > > So I can set LINKABLE inside vfs_tmpfile() only if I also pass > it open_flag Yes. > and then it would also happen before open returns > success. It doesn't matter. If open fails the temporary file will be deleted, dentry, inode and all. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/namei.c b/fs/namei.c index ad74877..4cbaf54 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3353,6 +3353,20 @@ static int do_last(struct nameidata *nd, return error; } +int vfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) +{ + int error; + + /* we want directory to be writable */ + error = inode_permission(dir, MAY_WRITE | MAY_EXEC); + if (error) + return error; + if (!dir->i_op->tmpfile) + return -EOPNOTSUPP; + return dir->i_op->tmpfile(dir, dentry, mode); +} +EXPORT_SYMBOL(vfs_tmpfile); + static int do_tmpfile(struct nameidata *nd, unsigned flags, const struct open_flags *op, struct file *file, int *opened) @@ -3368,14 +3382,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, if (unlikely(error)) goto out; dir = path.dentry->d_inode; - /* we want directory to be writable */ - error = inode_permission(dir, MAY_WRITE | MAY_EXEC); - if (error) - goto out2; - if (!dir->i_op->tmpfile) { - error = -EOPNOTSUPP; - goto out2; - } child = d_alloc(path.dentry, &name); if (unlikely(!child)) { error = -ENOMEM; @@ -3383,8 +3389,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, } dput(path.dentry); path.dentry = child; - error = dir->i_op->tmpfile(dir, child, op->mode); - if (error) + error = vfs_tmpfile(dir, child, op->mode); + if (unlikely(error)) goto out2; audit_inode(nd->name, child, 0); /* Don't check for other permissions, the inode was just created */ diff --git a/include/linux/fs.h b/include/linux/fs.h index 2ba0743..13f56f1 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1560,6 +1560,7 @@ extern int vfs_rmdir(struct inode *, struct dentry *); extern int vfs_unlink(struct inode *, struct dentry *, struct inode **); extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **, unsigned int); extern int vfs_whiteout(struct inode *, struct dentry *); +extern int vfs_tmpfile(struct inode *, struct dentry *, umode_t); /* * VFS file helper functions.
Factor out some common vfs bits from do_tmpfile() to be used by overlayfs for concurrent copy up. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/namei.c | 26 ++++++++++++++++---------- include/linux/fs.h | 1 + 2 files changed, 17 insertions(+), 10 deletions(-)