* [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile()
2017-01-16 17:45 [PATCH v3 0/6] ovl: concurrent copy up Amir Goldstein
@ 2017-01-16 17:46 ` Amir Goldstein
2017-01-16 19:47 ` Miklos Szeredi
2017-01-16 17:46 ` [PATCH v3 2/6] ovl: check if upperdir fs supports O_TMPFILE Amir Goldstein
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2017-01-16 17:46 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
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 | 66 +++++++++++++++++++++++++++++++++++-------------------
include/linux/fs.h | 3 +++
2 files changed, 46 insertions(+), 23 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index ad74877..3e7c7a6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3353,11 +3353,49 @@ static int do_last(struct nameidata *nd,
return error;
}
+struct dentry *vfs_tmpfile(struct inode *dir, struct dentry *dentry,
+ umode_t mode, int open_flag)
+{
+ static const struct qstr name = QSTR_INIT("/", 1);
+ struct dentry *child = NULL;
+ struct inode *inode;
+ int error;
+
+ /* we want directory to be writable */
+ error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
+ if (error)
+ goto out_err;
+ error = -EOPNOTSUPP;
+ if (!dir->i_op->tmpfile)
+ goto out_err;
+ error = -ENOMEM;
+ child = d_alloc(dentry, &name);
+ if (unlikely(!child))
+ goto out_err;
+ error = dir->i_op->tmpfile(dir, child, mode);
+ if (error)
+ goto out_err;
+ error = -ENOENT;
+ inode = child->d_inode;
+ if (unlikely(!inode))
+ goto out_err;
+ if (!(open_flag & O_EXCL)) {
+ spin_lock(&inode->i_lock);
+ inode->i_state |= I_LINKABLE;
+ spin_unlock(&inode->i_lock);
+ }
+ return child;
+
+out_err:
+ dput(child);
+ return ERR_PTR(error);
+}
+EXPORT_SYMBOL(vfs_tmpfile);
+
static int do_tmpfile(struct nameidata *nd, unsigned flags,
const struct open_flags *op,
struct file *file, int *opened)
{
- static const struct qstr name = QSTR_INIT("/", 1);
struct dentry *child;
struct inode *dir;
struct path path;
@@ -3368,24 +3406,12 @@ 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;
+ child = vfs_tmpfile(dir, path.dentry, op->mode, op->open_flag);
+ error = PTR_ERR(child);
+ if (unlikely(IS_ERR(child)))
goto out2;
- }
dput(path.dentry);
path.dentry = child;
- error = dir->i_op->tmpfile(dir, child, op->mode);
- if (error)
- goto out2;
audit_inode(nd->name, child, 0);
/* Don't check for other permissions, the inode was just created */
error = may_open(&path, 0, op->open_flag);
@@ -3396,14 +3422,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
if (error)
goto out2;
error = open_check_o_direct(file);
- if (error) {
+ if (error)
fput(file);
- } else if (!(op->open_flag & O_EXCL)) {
- struct inode *inode = file_inode(file);
- spin_lock(&inode->i_lock);
- inode->i_state |= I_LINKABLE;
- spin_unlock(&inode->i_lock);
- }
out2:
mnt_drop_write(path.mnt);
out:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba0743..8c7cbcb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1561,6 +1561,9 @@ 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 struct dentry *vfs_tmpfile(struct inode *dir, struct dentry *dentry,
+ umode_t mode, int open_flag);
+
/*
* VFS file helper functions.
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile()
2017-01-16 17:46 ` [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile() Amir Goldstein
@ 2017-01-16 19:47 ` Miklos Szeredi
2017-02-19 3:27 ` Al Viro
0 siblings, 1 reply; 14+ messages in thread
From: Miklos Szeredi @ 2017-01-16 19:47 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Al Viro, linux-unionfs, linux-fsdevel
On Mon, Jan 16, 2017 at 6:46 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.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/namei.c | 66 +++++++++++++++++++++++++++++++++++-------------------
> include/linux/fs.h | 3 +++
> 2 files changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index ad74877..3e7c7a6 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3353,11 +3353,49 @@ static int do_last(struct nameidata *nd,
> return error;
> }
>
> +struct dentry *vfs_tmpfile(struct inode *dir, struct dentry *dentry,
dir and dentry refer to the same thing; can just pass the dentry.
> + umode_t mode, int open_flag)
> +{
> + static const struct qstr name = QSTR_INIT("/", 1);
> + struct dentry *child = NULL;
> + struct inode *inode;
> + int error;
> +
> + /* we want directory to be writable */
> + error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
This is not in the scope of this patch, but shoudln't we be using
may_create() here? Or at least a variant without the audit thing...
Al?
Thanks,
Miklos
> + if (error)
> + goto out_err;
> + error = -EOPNOTSUPP;
> + if (!dir->i_op->tmpfile)
> + goto out_err;
> + error = -ENOMEM;
> + child = d_alloc(dentry, &name);
> + if (unlikely(!child))
> + goto out_err;
> + error = dir->i_op->tmpfile(dir, child, mode);
> + if (error)
> + goto out_err;
> + error = -ENOENT;
> + inode = child->d_inode;
> + if (unlikely(!inode))
> + goto out_err;
> + if (!(open_flag & O_EXCL)) {
> + spin_lock(&inode->i_lock);
> + inode->i_state |= I_LINKABLE;
> + spin_unlock(&inode->i_lock);
> + }
> + return child;
> +
> +out_err:
> + dput(child);
> + return ERR_PTR(error);
> +}
> +EXPORT_SYMBOL(vfs_tmpfile);
> +
> static int do_tmpfile(struct nameidata *nd, unsigned flags,
> const struct open_flags *op,
> struct file *file, int *opened)
> {
> - static const struct qstr name = QSTR_INIT("/", 1);
> struct dentry *child;
> struct inode *dir;
> struct path path;
> @@ -3368,24 +3406,12 @@ 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;
> + child = vfs_tmpfile(dir, path.dentry, op->mode, op->open_flag);
> + error = PTR_ERR(child);
> + if (unlikely(IS_ERR(child)))
> goto out2;
> - }
> dput(path.dentry);
> path.dentry = child;
> - error = dir->i_op->tmpfile(dir, child, op->mode);
> - if (error)
> - goto out2;
> audit_inode(nd->name, child, 0);
> /* Don't check for other permissions, the inode was just created */
> error = may_open(&path, 0, op->open_flag);
> @@ -3396,14 +3422,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
> if (error)
> goto out2;
> error = open_check_o_direct(file);
> - if (error) {
> + if (error)
> fput(file);
> - } else if (!(op->open_flag & O_EXCL)) {
> - struct inode *inode = file_inode(file);
> - spin_lock(&inode->i_lock);
> - inode->i_state |= I_LINKABLE;
> - spin_unlock(&inode->i_lock);
> - }
> out2:
> mnt_drop_write(path.mnt);
> out:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba0743..8c7cbcb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1561,6 +1561,9 @@ 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 struct dentry *vfs_tmpfile(struct inode *dir, struct dentry *dentry,
> + umode_t mode, int open_flag);
> +
> /*
> * VFS file helper functions.
> */
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile()
2017-01-16 19:47 ` Miklos Szeredi
@ 2017-02-19 3:27 ` Al Viro
2017-03-09 11:13 ` Miklos Szeredi
0 siblings, 1 reply; 14+ messages in thread
From: Al Viro @ 2017-02-19 3:27 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Amir Goldstein, linux-unionfs, linux-fsdevel
On Mon, Jan 16, 2017 at 08:47:32PM +0100, Miklos Szeredi wrote:
> > + umode_t mode, int open_flag)
> > +{
> > + static const struct qstr name = QSTR_INIT("/", 1);
> > + struct dentry *child = NULL;
> > + struct inode *inode;
> > + int error;
> > +
> > + /* we want directory to be writable */
> > + error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
>
> This is not in the scope of this patch, but shoudln't we be using
> may_create() here? Or at least a variant without the audit thing...
>
> Al?
may_create() expects directory + child dentry; here we have only parent.
IS_DEADDIR is rather pointless here - directory is not locked, for
starters, so rmdir might happen right under you. Or right after you've
returned from your function, for that matter. userns checks...
FWIW, no such checks are done in ->atomic_open() paths, so I'm not sure
how much are those worth...
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile()
2017-02-19 3:27 ` Al Viro
@ 2017-03-09 11:13 ` Miklos Szeredi
2017-03-09 17:31 ` Eric W. Biederman
0 siblings, 1 reply; 14+ messages in thread
From: Miklos Szeredi @ 2017-03-09 11:13 UTC (permalink / raw)
To: Al Viro; +Cc: Amir Goldstein, linux-unionfs, linux-fsdevel, Eric W. Biederman
On Sun, Feb 19, 2017 at 4:27 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jan 16, 2017 at 08:47:32PM +0100, Miklos Szeredi wrote:
>
>> > + umode_t mode, int open_flag)
>> > +{
>> > + static const struct qstr name = QSTR_INIT("/", 1);
>> > + struct dentry *child = NULL;
>> > + struct inode *inode;
>> > + int error;
>> > +
>> > + /* we want directory to be writable */
>> > + error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
>>
>> This is not in the scope of this patch, but shoudln't we be using
>> may_create() here? Or at least a variant without the audit thing...
>>
>> Al?
>
> may_create() expects directory + child dentry; here we have only parent.
> IS_DEADDIR is rather pointless here - directory is not locked, for
> starters, so rmdir might happen right under you. Or right after you've
> returned from your function, for that matter. userns checks...
> FWIW, no such checks are done in ->atomic_open() paths, so I'm not sure
> how much are those worth...
Eric would know since he added those checks.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile()
2017-03-09 11:13 ` Miklos Szeredi
@ 2017-03-09 17:31 ` Eric W. Biederman
0 siblings, 0 replies; 14+ messages in thread
From: Eric W. Biederman @ 2017-03-09 17:31 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, Amir Goldstein, linux-unionfs, linux-fsdevel
Miklos Szeredi <miklos@szeredi.hu> writes:
> On Sun, Feb 19, 2017 at 4:27 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Mon, Jan 16, 2017 at 08:47:32PM +0100, Miklos Szeredi wrote:
>>
>>> > + umode_t mode, int open_flag)
>>> > +{
>>> > + static const struct qstr name = QSTR_INIT("/", 1);
>>> > + struct dentry *child = NULL;
>>> > + struct inode *inode;
>>> > + int error;
>>> > +
>>> > + /* we want directory to be writable */
>>> > + error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
>>>
>>> This is not in the scope of this patch, but shoudln't we be using
>>> may_create() here? Or at least a variant without the audit thing...
>>>
>>> Al?
>>
>> may_create() expects directory + child dentry; here we have only parent.
>> IS_DEADDIR is rather pointless here - directory is not locked, for
>> starters, so rmdir might happen right under you. Or right after you've
>> returned from your function, for that matter. userns checks...
>> FWIW, no such checks are done in ->atomic_open() paths, so I'm not sure
>> how much are those worth...
>
> Eric would know since he added those checks.
Unless I am missing something the atomic_open path was fixed this merge
window when may_o_create was fixed. Missing places any place where
we create files is an oversight.
The point of those checks is when we have a filesystem mounted by root
in a user namespace like tmpfs or hopefully soon fuse that it will let
the vfs filter out uids and gids that the filesystem does not know how
to map thus has no hope of understanding. Since the filesystem does not
care about the uids and gids odds are filesystems won't be bothered to
test or deal with that case and corruption will result. As far as I can
see not filtering out umappable uids and gids is just laying a trap for
filesystem developers.
Which means vfs_tmpfile is definitely something that needs to be patched
to verify that the current_fsuid and current_fsgid are valid from
the filesystems point of view.
At the same time this only matters for filesystems that set
FS_USERNS_MOUNT and implement tmpfile. Which right now is tmpfs. Given
that tmpfs actually only uses the vfs inode, there are no corruption or
other filesystem misbehaviors right now. So it won't kill us if we
don't fix this for 4.11.
I am hoping things are far enough along that we can merge the patches to
fuse that make it safe to set FS_USER_NS for 4.12-rc1, and have truly
unprivileged fuse mounts. At which point this will matter more.
Eric
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile()
@ 2017-03-09 17:31 ` Eric W. Biederman
0 siblings, 0 replies; 14+ messages in thread
From: Eric W. Biederman @ 2017-03-09 17:31 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, Amir Goldstein, linux-unionfs, linux-fsdevel
Miklos Szeredi <miklos@szeredi.hu> writes:
> On Sun, Feb 19, 2017 at 4:27 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Mon, Jan 16, 2017 at 08:47:32PM +0100, Miklos Szeredi wrote:
>>
>>> > + umode_t mode, int open_flag)
>>> > +{
>>> > + static const struct qstr name = QSTR_INIT("/", 1);
>>> > + struct dentry *child = NULL;
>>> > + struct inode *inode;
>>> > + int error;
>>> > +
>>> > + /* we want directory to be writable */
>>> > + error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
>>>
>>> This is not in the scope of this patch, but shoudln't we be using
>>> may_create() here? Or at least a variant without the audit thing...
>>>
>>> Al?
>>
>> may_create() expects directory + child dentry; here we have only parent.
>> IS_DEADDIR is rather pointless here - directory is not locked, for
>> starters, so rmdir might happen right under you. Or right after you've
>> returned from your function, for that matter. userns checks...
>> FWIW, no such checks are done in ->atomic_open() paths, so I'm not sure
>> how much are those worth...
>
> Eric would know since he added those checks.
Unless I am missing something the atomic_open path was fixed this merge
window when may_o_create was fixed. Missing places any place where
we create files is an oversight.
The point of those checks is when we have a filesystem mounted by root
in a user namespace like tmpfs or hopefully soon fuse that it will let
the vfs filter out uids and gids that the filesystem does not know how
to map thus has no hope of understanding. Since the filesystem does not
care about the uids and gids odds are filesystems won't be bothered to
test or deal with that case and corruption will result. As far as I can
see not filtering out umappable uids and gids is just laying a trap for
filesystem developers.
Which means vfs_tmpfile is definitely something that needs to be patched
to verify that the current_fsuid and current_fsgid are valid from
the filesystems point of view.
At the same time this only matters for filesystems that set
FS_USERNS_MOUNT and implement tmpfile. Which right now is tmpfs. Given
that tmpfs actually only uses the vfs inode, there are no corruption or
other filesystem misbehaviors right now. So it won't kill us if we
don't fix this for 4.11.
I am hoping things are far enough along that we can merge the patches to
fuse that make it safe to set FS_USER_NS for 4.12-rc1, and have truly
unprivileged fuse mounts. At which point this will matter more.
Eric
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/6] ovl: check if upperdir fs supports O_TMPFILE
2017-01-16 17:45 [PATCH v3 0/6] ovl: concurrent copy up Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile() Amir Goldstein
@ 2017-01-16 17:46 ` Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 3/6] ovl: rearrange code in ovl_copy_up_locked() Amir Goldstein
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Amir Goldstein @ 2017-01-16 17:46 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
This is needed for choosing between concurrent copyup
using O_TMPFILE and legacy copyup using workdir+rename.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/overlayfs.h | 9 +++++++++
fs/overlayfs/ovl_entry.h | 1 +
fs/overlayfs/super.c | 10 ++++++++++
3 files changed, 20 insertions(+)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 8af450b..d5984c4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -127,6 +127,15 @@ static inline int ovl_do_whiteout(struct inode *dir, struct dentry *dentry)
return err;
}
+static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
+{
+ struct dentry *ret = vfs_tmpfile(dentry->d_inode, dentry, mode, 0);
+ int err = IS_ERR(ret) ? PTR_ERR(ret) : 0;
+
+ pr_debug("tmpfile(%pd2, 0%o) = %i\n", dentry, mode, err);
+ return ret;
+}
+
static inline struct inode *ovl_inode_real(struct inode *inode, bool *is_upper)
{
unsigned long x = (unsigned long) READ_ONCE(inode->i_private);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index d14bca1..65f24000 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -27,6 +27,7 @@ struct ovl_fs {
struct ovl_config config;
/* creds of process who forced instantiation of super block */
const struct cred *creator_cred;
+ bool tmpfile;
};
/* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 20f48ab..ff05065 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -825,6 +825,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
* creation of workdir in previous step.
*/
if (ufs->workdir) {
+ struct dentry *temp;
+
err = ovl_check_d_type_supported(&workpath);
if (err < 0)
goto out_put_workdir;
@@ -836,6 +838,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
*/
if (!err)
pr_warn("overlayfs: upper fs needs to support d_type.\n");
+
+ /* Check if upper/work fs supports O_TMPFILE */
+ temp = ovl_do_tmpfile(ufs->workdir, S_IFREG | 0);
+ ufs->tmpfile = !IS_ERR(temp);
+ if (ufs->tmpfile)
+ dput(temp);
+ else
+ pr_warn("overlayfs: upper fs does not support tmpfile.\n");
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 3/6] ovl: rearrange code in ovl_copy_up_locked()
2017-01-16 17:45 [PATCH v3 0/6] ovl: concurrent copy up Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 1/6] vfs: create vfs helper vfs_tmpfile() Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 2/6] ovl: check if upperdir fs supports O_TMPFILE Amir Goldstein
@ 2017-01-16 17:46 ` Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 4/6] ovl: copy up regular file using O_TMPFILE Amir Goldstein
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Amir Goldstein @ 2017-01-16 17:46 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
As preparation to implementing copy up with O_TMPFILE,
name the variable for dentry before final rename 'temp' and
assign it to 'newdentry' only after rename.
Also lookup upper dentry before looking up temp dentry and
move ovl_set_timestamps() into ovl_copy_up_locked(), because
that is going to be more convenient for upcoming change.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/copy_up.c | 49 +++++++++++++++++++++++++------------------------
1 file changed, 25 insertions(+), 24 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f57043d..01e3327 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -232,12 +232,14 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
struct dentry *dentry, struct path *lowerpath,
- struct kstat *stat, const char *link)
+ struct kstat *stat, const char *link,
+ struct kstat *pstat)
{
struct inode *wdir = workdir->d_inode;
struct inode *udir = upperdir->d_inode;
struct dentry *newdentry = NULL;
struct dentry *upper = NULL;
+ struct dentry *temp = NULL;
int err;
const struct cred *old_creds = NULL;
struct cred *new_creds = NULL;
@@ -248,25 +250,25 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
.link = link
};
- newdentry = ovl_lookup_temp(workdir, dentry);
- err = PTR_ERR(newdentry);
- if (IS_ERR(newdentry))
- goto out;
-
upper = lookup_one_len(dentry->d_name.name, upperdir,
dentry->d_name.len);
err = PTR_ERR(upper);
if (IS_ERR(upper))
- goto out1;
+ goto out;
err = security_inode_copy_up(dentry, &new_creds);
if (err < 0)
- goto out2;
+ goto out1;
if (new_creds)
old_creds = override_creds(new_creds);
- err = ovl_create_real(wdir, newdentry, &cattr, NULL, true);
+ temp = ovl_lookup_temp(workdir, dentry);
+ err = PTR_ERR(temp);
+ if (IS_ERR(temp))
+ goto out1;
+
+ err = ovl_create_real(wdir, temp, &cattr, NULL, true);
if (new_creds) {
revert_creds(old_creds);
@@ -281,39 +283,42 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
ovl_path_upper(dentry, &upperpath);
BUG_ON(upperpath.dentry != NULL);
- upperpath.dentry = newdentry;
+ upperpath.dentry = temp;
err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
if (err)
goto out_cleanup;
}
- err = ovl_copy_xattr(lowerpath->dentry, newdentry);
+ err = ovl_copy_xattr(lowerpath->dentry, temp);
if (err)
goto out_cleanup;
- inode_lock(newdentry->d_inode);
- err = ovl_set_attr(newdentry, stat);
- inode_unlock(newdentry->d_inode);
+ inode_lock(temp->d_inode);
+ err = ovl_set_attr(temp, stat);
+ inode_unlock(temp->d_inode);
if (err)
goto out_cleanup;
- err = ovl_do_rename(wdir, newdentry, udir, upper, 0);
+ err = ovl_do_rename(wdir, temp, udir, upper, 0);
if (err)
goto out_cleanup;
+ newdentry = dget(temp);
ovl_dentry_update(dentry, newdentry);
ovl_inode_update(d_inode(dentry), d_inode(newdentry));
- newdentry = NULL;
+
+ /* Restore timestamps on parent (best effort) */
+ ovl_set_timestamps(upperdir, pstat);
out2:
- dput(upper);
+ dput(temp);
out1:
- dput(newdentry);
+ dput(upper);
out:
return err;
out_cleanup:
- ovl_cleanup(wdir, newdentry);
+ ovl_cleanup(wdir, temp);
goto out2;
}
@@ -368,11 +373,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
}
err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
- stat, link);
- if (!err) {
- /* Restore timestamps on parent (best effort) */
- ovl_set_timestamps(upperdir, &pstat);
- }
+ stat, link, &pstat);
out_unlock:
unlock_rename(workdir, upperdir);
do_delayed_call(&done);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 4/6] ovl: copy up regular file using O_TMPFILE
2017-01-16 17:45 [PATCH v3 0/6] ovl: concurrent copy up Amir Goldstein
` (2 preceding siblings ...)
2017-01-16 17:46 ` [PATCH v3 3/6] ovl: rearrange code in ovl_copy_up_locked() Amir Goldstein
@ 2017-01-16 17:46 ` Amir Goldstein
2017-04-05 18:32 ` Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 5/6] ovl: introduce copy up waitqueue Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 6/6] ovl: concurrent copy up of regular files Amir Goldstein
5 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2017-01-16 17:46 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
In preparation for concurrent copy up, implement copy up
of regular file as O_TMPFILE that is linked to upperdir
instead of a file in workdir that is moved to upperdir.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/copy_up.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 01e3327..6e39e90 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -20,6 +20,7 @@
#include <linux/fdtable.h>
#include <linux/ratelimit.h>
#include "overlayfs.h"
+#include "ovl_entry.h"
#define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
@@ -233,7 +234,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
struct dentry *dentry, struct path *lowerpath,
struct kstat *stat, const char *link,
- struct kstat *pstat)
+ struct kstat *pstat, bool tmpfile)
{
struct inode *wdir = workdir->d_inode;
struct inode *udir = upperdir->d_inode;
@@ -263,12 +264,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
if (new_creds)
old_creds = override_creds(new_creds);
- temp = ovl_lookup_temp(workdir, dentry);
+ if (tmpfile)
+ temp = ovl_do_tmpfile(upperdir, stat->mode);
+ else
+ temp = ovl_lookup_temp(workdir, dentry);
err = PTR_ERR(temp);
if (IS_ERR(temp))
goto out1;
- err = ovl_create_real(wdir, temp, &cattr, NULL, true);
+ err = 0;
+ if (!tmpfile)
+ err = ovl_create_real(wdir, temp, &cattr, NULL, true);
if (new_creds) {
revert_creds(old_creds);
@@ -300,11 +306,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
if (err)
goto out_cleanup;
- err = ovl_do_rename(wdir, temp, udir, upper, 0);
+ if (tmpfile)
+ err = ovl_do_link(temp, udir, upper, true);
+ else
+ err = ovl_do_rename(wdir, temp, udir, upper, 0);
if (err)
goto out_cleanup;
- newdentry = dget(temp);
+ newdentry = dget(tmpfile ? upper : temp);
ovl_dentry_update(dentry, newdentry);
ovl_inode_update(d_inode(dentry), d_inode(newdentry));
@@ -318,7 +327,8 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
return err;
out_cleanup:
- ovl_cleanup(wdir, temp);
+ if (!tmpfile)
+ ovl_cleanup(wdir, temp);
goto out2;
}
@@ -342,6 +352,9 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
struct dentry *lowerdentry = lowerpath->dentry;
struct dentry *upperdir;
const char *link = NULL;
+ struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ /* Should we copyup with O_TMPFILE or with workdir? */
+ bool tmpfile = S_ISREG(stat->mode) && ofs->tmpfile;
if (WARN_ON(!workdir))
return -EROFS;
@@ -373,7 +386,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
}
err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
- stat, link, &pstat);
+ stat, link, &pstat, tmpfile);
out_unlock:
unlock_rename(workdir, upperdir);
do_delayed_call(&done);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/6] ovl: copy up regular file using O_TMPFILE
2017-01-16 17:46 ` [PATCH v3 4/6] ovl: copy up regular file using O_TMPFILE Amir Goldstein
@ 2017-04-05 18:32 ` Amir Goldstein
2017-04-05 19:36 ` Vivek Goyal
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2017-04-05 18:32 UTC (permalink / raw)
To: Vivek Goyal; +Cc: Al Viro, linux-unionfs, linux-fsdevel, Miklos Szeredi
On Mon, Jan 16, 2017 at 7:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> In preparation for concurrent copy up, implement copy up
> of regular file as O_TMPFILE that is linked to upperdir
> instead of a file in workdir that is moved to upperdir.
>
Hi Vivek,
This commit is already in v4.11-rc1 and I haven't heard any complains.
But I wanted to ask all the same.
With this commit the semantics of security checks are modified a bit.
Before this change it was:
- Create temp file in workdir and negative dentry in upperdir with
security_inode_copy_up() creds
- Move temp file to upperdir with mounter creds
After this change it is:
- Create an O_TMPFILE and negative dentry in upperdir with
security_inode_copy_up() creds
- Link O_TMPFILE to upperdir with mounter creds
Is this equivalent as far as SELinux goes?
Does it mean that users may have to fix their sepolicy in some way?
Were you able to verify that there are no overlay+selinux regression
with v4.11-rc1? I just don't have selinux in my test setup...
Thanks,
Amir.
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/copy_up.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 01e3327..6e39e90 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -20,6 +20,7 @@
> #include <linux/fdtable.h>
> #include <linux/ratelimit.h>
> #include "overlayfs.h"
> +#include "ovl_entry.h"
>
> #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
>
> @@ -233,7 +234,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
> static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> struct dentry *dentry, struct path *lowerpath,
> struct kstat *stat, const char *link,
> - struct kstat *pstat)
> + struct kstat *pstat, bool tmpfile)
> {
> struct inode *wdir = workdir->d_inode;
> struct inode *udir = upperdir->d_inode;
> @@ -263,12 +264,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> if (new_creds)
> old_creds = override_creds(new_creds);
>
> - temp = ovl_lookup_temp(workdir, dentry);
> + if (tmpfile)
> + temp = ovl_do_tmpfile(upperdir, stat->mode);
> + else
> + temp = ovl_lookup_temp(workdir, dentry);
> err = PTR_ERR(temp);
> if (IS_ERR(temp))
> goto out1;
>
> - err = ovl_create_real(wdir, temp, &cattr, NULL, true);
> + err = 0;
> + if (!tmpfile)
> + err = ovl_create_real(wdir, temp, &cattr, NULL, true);
>
> if (new_creds) {
> revert_creds(old_creds);
> @@ -300,11 +306,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> if (err)
> goto out_cleanup;
>
> - err = ovl_do_rename(wdir, temp, udir, upper, 0);
> + if (tmpfile)
> + err = ovl_do_link(temp, udir, upper, true);
> + else
> + err = ovl_do_rename(wdir, temp, udir, upper, 0);
> if (err)
> goto out_cleanup;
>
> - newdentry = dget(temp);
> + newdentry = dget(tmpfile ? upper : temp);
> ovl_dentry_update(dentry, newdentry);
> ovl_inode_update(d_inode(dentry), d_inode(newdentry));
>
> @@ -318,7 +327,8 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> return err;
>
> out_cleanup:
> - ovl_cleanup(wdir, temp);
> + if (!tmpfile)
> + ovl_cleanup(wdir, temp);
> goto out2;
> }
>
> @@ -342,6 +352,9 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> struct dentry *lowerdentry = lowerpath->dentry;
> struct dentry *upperdir;
> const char *link = NULL;
> + struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> + /* Should we copyup with O_TMPFILE or with workdir? */
> + bool tmpfile = S_ISREG(stat->mode) && ofs->tmpfile;
>
> if (WARN_ON(!workdir))
> return -EROFS;
> @@ -373,7 +386,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> }
>
> err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
> - stat, link, &pstat);
> + stat, link, &pstat, tmpfile);
> out_unlock:
> unlock_rename(workdir, upperdir);
> do_delayed_call(&done);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/6] ovl: copy up regular file using O_TMPFILE
2017-04-05 18:32 ` Amir Goldstein
@ 2017-04-05 19:36 ` Vivek Goyal
0 siblings, 0 replies; 14+ messages in thread
From: Vivek Goyal @ 2017-04-05 19:36 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Al Viro, linux-unionfs, linux-fsdevel, Miklos Szeredi
On Wed, Apr 05, 2017 at 09:32:36PM +0300, Amir Goldstein wrote:
> On Mon, Jan 16, 2017 at 7:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > In preparation for concurrent copy up, implement copy up
> > of regular file as O_TMPFILE that is linked to upperdir
> > instead of a file in workdir that is moved to upperdir.
> >
>
> Hi Vivek,
>
> This commit is already in v4.11-rc1 and I haven't heard any complains.
> But I wanted to ask all the same.
>
> With this commit the semantics of security checks are modified a bit.
>
> Before this change it was:
> - Create temp file in workdir and negative dentry in upperdir with
> security_inode_copy_up() creds
> - Move temp file to upperdir with mounter creds
>
> After this change it is:
> - Create an O_TMPFILE and negative dentry in upperdir with
> security_inode_copy_up() creds
> - Link O_TMPFILE to upperdir with mounter creds
>
> Is this equivalent as far as SELinux goes?
> Does it mean that users may have to fix their sepolicy in some way?
Hi Amir,
I think this change is fine. security_inode_copy_up() returns new creds
using which new file should be created during copy up. And using tmpfile
does not seem to change it. We still switch to creds retruned by
security_inode_copy_up() and then tmpfile is created.
>
> Were you able to verify that there are no overlay+selinux regression
> with v4.11-rc1? I just don't have selinux in my test setup...
I ran selinux-testsuite overlay test cases and they all passed. So I can't
think of any regression yet.
https://github.com/SELinuxProject/selinux-testsuite
Thanks
Vivek
>
> Thanks,
> Amir.
>
> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> > fs/overlayfs/copy_up.c | 27 ++++++++++++++++++++-------
> > 1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 01e3327..6e39e90 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -20,6 +20,7 @@
> > #include <linux/fdtable.h>
> > #include <linux/ratelimit.h>
> > #include "overlayfs.h"
> > +#include "ovl_entry.h"
> >
> > #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
> >
> > @@ -233,7 +234,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
> > static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> > struct dentry *dentry, struct path *lowerpath,
> > struct kstat *stat, const char *link,
> > - struct kstat *pstat)
> > + struct kstat *pstat, bool tmpfile)
> > {
> > struct inode *wdir = workdir->d_inode;
> > struct inode *udir = upperdir->d_inode;
> > @@ -263,12 +264,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> > if (new_creds)
> > old_creds = override_creds(new_creds);
> >
> > - temp = ovl_lookup_temp(workdir, dentry);
> > + if (tmpfile)
> > + temp = ovl_do_tmpfile(upperdir, stat->mode);
> > + else
> > + temp = ovl_lookup_temp(workdir, dentry);
> > err = PTR_ERR(temp);
> > if (IS_ERR(temp))
> > goto out1;
> >
> > - err = ovl_create_real(wdir, temp, &cattr, NULL, true);
> > + err = 0;
> > + if (!tmpfile)
> > + err = ovl_create_real(wdir, temp, &cattr, NULL, true);
> >
> > if (new_creds) {
> > revert_creds(old_creds);
> > @@ -300,11 +306,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> > if (err)
> > goto out_cleanup;
> >
> > - err = ovl_do_rename(wdir, temp, udir, upper, 0);
> > + if (tmpfile)
> > + err = ovl_do_link(temp, udir, upper, true);
> > + else
> > + err = ovl_do_rename(wdir, temp, udir, upper, 0);
> > if (err)
> > goto out_cleanup;
> >
> > - newdentry = dget(temp);
> > + newdentry = dget(tmpfile ? upper : temp);
> > ovl_dentry_update(dentry, newdentry);
> > ovl_inode_update(d_inode(dentry), d_inode(newdentry));
> >
> > @@ -318,7 +327,8 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> > return err;
> >
> > out_cleanup:
> > - ovl_cleanup(wdir, temp);
> > + if (!tmpfile)
> > + ovl_cleanup(wdir, temp);
> > goto out2;
> > }
> >
> > @@ -342,6 +352,9 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > struct dentry *lowerdentry = lowerpath->dentry;
> > struct dentry *upperdir;
> > const char *link = NULL;
> > + struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > + /* Should we copyup with O_TMPFILE or with workdir? */
> > + bool tmpfile = S_ISREG(stat->mode) && ofs->tmpfile;
> >
> > if (WARN_ON(!workdir))
> > return -EROFS;
> > @@ -373,7 +386,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > }
> >
> > err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
> > - stat, link, &pstat);
> > + stat, link, &pstat, tmpfile);
> > out_unlock:
> > unlock_rename(workdir, upperdir);
> > do_delayed_call(&done);
> > --
> > 2.7.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 5/6] ovl: introduce copy up waitqueue
2017-01-16 17:45 [PATCH v3 0/6] ovl: concurrent copy up Amir Goldstein
` (3 preceding siblings ...)
2017-01-16 17:46 ` [PATCH v3 4/6] ovl: copy up regular file using O_TMPFILE Amir Goldstein
@ 2017-01-16 17:46 ` Amir Goldstein
2017-01-16 17:46 ` [PATCH v3 6/6] ovl: concurrent copy up of regular files Amir Goldstein
5 siblings, 0 replies; 14+ messages in thread
From: Amir Goldstein @ 2017-01-16 17:46 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
The overlay sb 'copyup_wq' and overlay inode 'copying' condition
variable are about to replace the upper sb rename_lock, as finer
grained synchronization objects for concurrent copy up.
Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/overlayfs.h | 2 ++
fs/overlayfs/ovl_entry.h | 2 ++
fs/overlayfs/super.c | 1 +
fs/overlayfs/util.c | 30 ++++++++++++++++++++++++++++++
4 files changed, 35 insertions(+)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d5984c4..c997dc8 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -178,6 +178,8 @@ void ovl_dentry_version_inc(struct dentry *dentry);
u64 ovl_dentry_version_get(struct dentry *dentry);
bool ovl_is_whiteout(struct dentry *dentry);
struct file *ovl_path_open(struct path *path, int flags);
+int ovl_copy_up_start(struct dentry *dentry);
+void ovl_copy_up_end(struct dentry *dentry);
/* namei.c */
int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 65f24000..59614fa 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -28,6 +28,7 @@ struct ovl_fs {
/* creds of process who forced instantiation of super block */
const struct cred *creator_cred;
bool tmpfile;
+ wait_queue_head_t copyup_wq;
};
/* private information held for every overlayfs dentry */
@@ -39,6 +40,7 @@ struct ovl_entry {
u64 version;
const char *redirect;
bool opaque;
+ bool copying;
};
struct rcu_head rcu;
};
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ff05065..6792bb7 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -708,6 +708,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
if (!ufs)
goto out;
+ init_waitqueue_head(&ufs->copyup_wq);
ufs->config.redirect_dir = ovl_redirect_dir_def;
err = ovl_parse_opt((char *) data, &ufs->config);
if (err)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 952286f..01157d6 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -263,3 +263,33 @@ struct file *ovl_path_open(struct path *path, int flags)
{
return dentry_open(path, flags | O_NOATIME, current_cred());
}
+
+int ovl_copy_up_start(struct dentry *dentry)
+{
+ struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_entry *oe = dentry->d_fsdata;
+ int err;
+
+ spin_lock(&ofs->copyup_wq.lock);
+ err = wait_event_interruptible_locked(ofs->copyup_wq, !oe->copying);
+ if (!err) {
+ if (oe->__upperdentry)
+ err = 1; /* Already copied up */
+ else
+ oe->copying = true;
+ }
+ spin_unlock(&ofs->copyup_wq.lock);
+
+ return err;
+}
+
+void ovl_copy_up_end(struct dentry *dentry)
+{
+ struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_entry *oe = dentry->d_fsdata;
+
+ spin_lock(&ofs->copyup_wq.lock);
+ oe->copying = false;
+ wake_up_locked(&ofs->copyup_wq);
+ spin_unlock(&ofs->copyup_wq.lock);
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 6/6] ovl: concurrent copy up of regular files
2017-01-16 17:45 [PATCH v3 0/6] ovl: concurrent copy up Amir Goldstein
` (4 preceding siblings ...)
2017-01-16 17:46 ` [PATCH v3 5/6] ovl: introduce copy up waitqueue Amir Goldstein
@ 2017-01-16 17:46 ` Amir Goldstein
5 siblings, 0 replies; 14+ messages in thread
From: Amir Goldstein @ 2017-01-16 17:46 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Al Viro, linux-unionfs, linux-fsdevel
Now that copy up of regular file is done using O_TMPFILE,
we don't need to hold rename_lock throughout copy up.
Use the copy up waitqueue to synchronize concurrent copy up
of the same file. Different regular files can be copied up
concurrently.
The upper dir inode_lock is taken instead of rename_lock,
because it is needed for lookup and later for linking the
temp file, but it is released while copying up data.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/copy_up.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 6e39e90..a53b1a3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -291,7 +291,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
BUG_ON(upperpath.dentry != NULL);
upperpath.dentry = temp;
+ if (tmpfile)
+ inode_unlock(udir);
+
err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
+
+ if (tmpfile)
+ inode_lock_nested(udir, I_MUTEX_PARENT);
+
if (err)
goto out_cleanup;
}
@@ -374,6 +381,24 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
return PTR_ERR(link);
}
+ if (tmpfile) {
+ err = ovl_copy_up_start(dentry);
+ /* err < 0: interrupted, err > 0: raced with another copy-up */
+ if (unlikely(err)) {
+ pr_debug("ovl_copy_up_start(%pd2) = %i\n", dentry, err);
+ if (err > 0)
+ err = 0;
+ goto out_done;
+ }
+
+ inode_lock_nested(upperdir->d_inode, I_MUTEX_PARENT);
+ err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
+ stat, link, &pstat, tmpfile);
+ inode_unlock(upperdir->d_inode);
+ ovl_copy_up_end(dentry);
+ goto out_done;
+ }
+
err = -EIO;
if (lock_rename(workdir, upperdir) != NULL) {
pr_err("overlayfs: failed to lock workdir+upperdir\n");
@@ -389,6 +414,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
stat, link, &pstat, tmpfile);
out_unlock:
unlock_rename(workdir, upperdir);
+out_done:
do_delayed_call(&done);
return err;
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread