Skip to contentSkip to conversation

Overlay inode lock cleanups

PATCH In review Amir Goldstein 2 participants 5 messages Oct 18, 2018 – Oct 29, 2018 Overlay filesystem

lore
Share & tools
lore

Amir Goldstein

author maintainer
#0 lore raw

cover Overlay inode lock cleanups

Miklos,

First patch removes pretty big ugliness. Second patch is just a matter of taste, so as you wish.

I'll use this opportunity to recap the queue of my proposed patches for v4.20, so you can see if you missed to consider any of them:

* 5401b99fcbb9 - (overlayfs-devel) ovl: abstract ovl_inode lock with a helper
* 8edcbcef4e55 - ovl: remove the 'locked' argument of ovl_nlink_{start,end}
* bd2ea818ecca - ovl: relax requirement for non null uuid of lower fs
* 19c221c3a6b5 - ovl: disable new features for deprecated upper fs
* 1c785e83823c - ovl: untangle copy up call chain
* b67871490628 - (ovl-fixes) ovl: fix recursive oi->lock in ovl_link()
* e39a0eb89ae4 - vfs: fix FIGETBSZ ioctl on an overlayfs file
* 431d5170b91d - ovl: fix error handling in ovl_verify_set_fh()
* 388b00daecb8 - kernel/acct.c: fix locking order when switching acct files

Thanks,
Amir.

Amir Goldstein (2):
ovl: remove the 'locked' argument of ovl_nlink_{start,end}
ovl: abstract ovl_inode lock with a helper

 fs/overlayfs/dir.c       | 21 ++++++++++----------
 fs/overlayfs/overlayfs.h | 14 +++++++++++--
 fs/overlayfs/util.c      | 43 +++++++++++++++++++---------------------
 3 files changed, 43 insertions(+), 35 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH 1/2 ovl: remove the 'locked' argument of ovl_nlink_{start,end}

It just makes the interface strange without adding any significant value. The only case where locked is false and return value is 0 is in ovl_rename() when new is negative, so handle that case explicitly in ovl_rename().

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/overlayfs/dir.c       | 21 +++++++++++----------
 fs/overlayfs/overlayfs.h |  4 ++--
 fs/overlayfs/util.c      | 28 ++++++++++++----------------
 3 files changed, 25 insertions(+), 28 deletions(-)
fs/overlayfs/dir.c +11 -10
index e1a55ecb7aba..e03f39550ccf 100644
@@ -652,7 +652,6 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 		    struct dentry *new)
 {
 	int err;
-	bool locked = false;
 	struct inode *inode;
 
 	err = ovl_want_write(old);

@@ -673,7 +672,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 			goto out_drop_write;
 	}
 
-	err = ovl_nlink_start(old, &locked);
+	err = ovl_nlink_start(old);
 	if (err)
 		goto out_drop_write;
 

@@ -686,7 +685,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 	if (err)
 		iput(inode);
 
-	ovl_nlink_end(old, locked);
+	ovl_nlink_end(old);
 out_drop_write:
 	ovl_drop_write(old);
 out:

@@ -811,7 +810,6 @@ static bool ovl_pure_upper(struct dentry *dentry)
 static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 {
 	int err;
-	bool locked = false;
 	const struct cred *old_cred;
 	struct dentry *upperdentry;
 	bool lower_positive = ovl_lower_positive(dentry);

@@ -832,7 +830,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 	if (err)
 		goto out_drop_write;
 
-	err = ovl_nlink_start(dentry, &locked);
+	err = ovl_nlink_start(dentry);
 	if (err)
 		goto out_drop_write;
 

@@ -848,7 +846,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 		else
 			drop_nlink(dentry->d_inode);
 	}
-	ovl_nlink_end(dentry, locked);
+	ovl_nlink_end(dentry);
 
 	/*
 	 * Copy ctime

@@ -1012,7 +1010,6 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 		      unsigned int flags)
 {
 	int err;
-	bool locked = false;
 	struct dentry *old_upperdir;
 	struct dentry *new_upperdir;
 	struct dentry *olddentry;

@@ -1021,6 +1018,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 	bool old_opaque;
 	bool new_opaque;
 	bool cleanup_whiteout = false;
+	bool update_nlink = false;
 	bool overwrite = !(flags & RENAME_EXCHANGE);
 	bool is_dir = d_is_dir(old);
 	bool new_is_dir = d_is_dir(new);

@@ -1078,10 +1076,12 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 		err = ovl_copy_up(new);
 		if (err)
 			goto out_drop_write;
-	} else {
-		err = ovl_nlink_start(new, &locked);
+	} else if (d_inode(new)) {
+		err = ovl_nlink_start(new);
 		if (err)
 			goto out_drop_write;
+
+		update_nlink = true;
 	}
 
 	old_cred = ovl_override_creds(old->d_sb);

@@ -1210,7 +1210,8 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
 	revert_creds(old_cred);
-	ovl_nlink_end(new, locked);
+	if (update_nlink)
+		ovl_nlink_end(new);
 out_drop_write:
 	ovl_drop_write(old);
 out:
fs/overlayfs/overlayfs.h +2 -2
index a3c0d9584312..b48ef22ef96b 100644
@@ -271,8 +271,8 @@ bool ovl_test_flag(unsigned long flag, struct inode *inode);
 bool ovl_inuse_trylock(struct dentry *dentry);
 void ovl_inuse_unlock(struct dentry *dentry);
 bool ovl_need_index(struct dentry *dentry);
-int ovl_nlink_start(struct dentry *dentry, bool *locked);
-void ovl_nlink_end(struct dentry *dentry, bool locked);
+int ovl_nlink_start(struct dentry *dentry);
+void ovl_nlink_end(struct dentry *dentry);
 int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
 int ovl_check_metacopy_xattr(struct dentry *dentry);
 bool ovl_is_metacopy_dentry(struct dentry *dentry);
fs/overlayfs/util.c +12 -16
index 8cd37baf5d0a..d3a786006729 100644
@@ -738,14 +738,14 @@ static void ovl_cleanup_index(struct dentry *dentry)
  * Operations that change overlay inode and upper inode nlink need to be
  * synchronized with copy up for persistent nlink accounting.
  */
-int ovl_nlink_start(struct dentry *dentry, bool *locked)
+int ovl_nlink_start(struct dentry *dentry)
 {
 	struct ovl_inode *oi = OVL_I(d_inode(dentry));
 	const struct cred *old_cred;
 	int err;
 
-	if (!d_inode(dentry))
-		return 0;
+	if (WARN_ON(!d_inode(dentry)))
+		return -ENOENT;
 
 	/*
 	 * With inodes index is enabled, we store the union overlay nlink

@@ -787,26 +787,22 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked)
 out:
 	if (err)
 		mutex_unlock(&oi->lock);
-	else
-		*locked = true;
 
 	return err;
 }
 
-void ovl_nlink_end(struct dentry *dentry, bool locked)
+void ovl_nlink_end(struct dentry *dentry)
 {
-	if (locked) {
-		if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) &&
-		    d_inode(dentry)->i_nlink == 0) {
-			const struct cred *old_cred;
-
-			old_cred = ovl_override_creds(dentry->d_sb);
-			ovl_cleanup_index(dentry);
-			revert_creds(old_cred);
-		}
+	if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) &&
+	    d_inode(dentry)->i_nlink == 0) {
+		const struct cred *old_cred;
 
-		mutex_unlock(&OVL_I(d_inode(dentry))->lock);
+		old_cred = ovl_override_creds(dentry->d_sb);
+		ovl_cleanup_index(dentry);
+		revert_creds(old_cred);
 	}
+
+	mutex_unlock(&OVL_I(d_inode(dentry))->lock);
 }
 
 int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
Signature · 1 line
2.17.1
author maintainer
#2 lore raw

PATCH 2/2 ovl: abstract ovl_inode lock with a helper

In reply to Amir Goldstein

The abstraction improves code readabilty (to some).

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/overlayfs/overlayfs.h | 10 ++++++++++
 fs/overlayfs/util.c      | 25 +++++++++++++------------
 2 files changed, 23 insertions(+), 12 deletions(-)
fs/overlayfs/overlayfs.h +10 -0
index b48ef22ef96b..5e45cb3630a0 100644
@@ -290,6 +290,16 @@ static inline unsigned int ovl_xino_bits(struct super_block *sb)
 	return ofs->xino_bits;
 }
 
+static inline int ovl_inode_lock(struct inode *inode)
+{
+	return mutex_lock_interruptible(&OVL_I(inode)->lock);
+}
+
+static inline void ovl_inode_unlock(struct inode *inode)
+{
+	mutex_unlock(&OVL_I(inode)->lock);
+}
+
 
 /* namei.c */
 int ovl_check_fh_len(struct ovl_fh *fh, int fh_len);
fs/overlayfs/util.c +13 -12
index d3a786006729..7c01327b1852 100644
@@ -521,13 +521,13 @@ bool ovl_already_copied_up(struct dentry *dentry, int flags)
 
 int ovl_copy_up_start(struct dentry *dentry, int flags)
 {
-	struct ovl_inode *oi = OVL_I(d_inode(dentry));
+	struct inode *inode = d_inode(dentry);
 	int err;
 
-	err = mutex_lock_interruptible(&oi->lock);
+	err = ovl_inode_lock(inode);
 	if (!err && ovl_already_copied_up_locked(dentry, flags)) {
 		err = 1; /* Already copied up */
-		mutex_unlock(&oi->lock);
+		ovl_inode_unlock(inode);
 	}
 
 	return err;

@@ -535,7 +535,7 @@ int ovl_copy_up_start(struct dentry *dentry, int flags)
 
 void ovl_copy_up_end(struct dentry *dentry)
 {
-	mutex_unlock(&OVL_I(d_inode(dentry))->lock);
+	ovl_inode_unlock(d_inode(dentry));
 }
 
 bool ovl_check_origin_xattr(struct dentry *dentry)

@@ -740,11 +740,11 @@ static void ovl_cleanup_index(struct dentry *dentry)
  */
 int ovl_nlink_start(struct dentry *dentry)
 {
-	struct ovl_inode *oi = OVL_I(d_inode(dentry));
+	struct inode *inode = d_inode(dentry);
 	const struct cred *old_cred;
 	int err;
 
-	if (WARN_ON(!d_inode(dentry)))
+	if (WARN_ON(!inode))
 		return -ENOENT;
 
 	/*

@@ -767,11 +767,11 @@ int ovl_nlink_start(struct dentry *dentry)
 			return err;
 	}
 
-	err = mutex_lock_interruptible(&oi->lock);
+	err = ovl_inode_lock(inode);
 	if (err)
 		return err;
 
-	if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, d_inode(dentry)))
+	if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
 		goto out;
 
 	old_cred = ovl_override_creds(dentry->d_sb);

@@ -786,15 +786,16 @@ int ovl_nlink_start(struct dentry *dentry)
 
 out:
 	if (err)
-		mutex_unlock(&oi->lock);
+		ovl_inode_unlock(inode);
 
 	return err;
 }
 
 void ovl_nlink_end(struct dentry *dentry)
 {
-	if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) &&
-	    d_inode(dentry)->i_nlink == 0) {
+	struct inode *inode = d_inode(dentry);
+
+	if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) {
 		const struct cred *old_cred;
 
 		old_cred = ovl_override_creds(dentry->d_sb);

@@ -802,7 +803,7 @@ void ovl_nlink_end(struct dentry *dentry)
 		revert_creds(old_cred);
 	}
 
-	mutex_unlock(&OVL_I(d_inode(dentry))->lock);
+	ovl_inode_unlock(inode);
 }
 
 int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
Signature · 1 line
2.17.1
maintainer
#3 lore raw
In reply to Amir Goldstein
Quoted 8 lines · Amir Goldstein

Miklos,

First patch removes pretty big ugliness. Second patch is just a matter of taste, so as you wish.

I'll use this opportunity to recap the queue of my proposed patches for v4.20, so you can see if you missed to consider any of them:

Quoted 9 lines

* 5401b99fcbb9 - (overlayfs-devel) ovl: abstract ovl_inode lock with a helper
* 8edcbcef4e55 - ovl: remove the 'locked' argument of ovl_nlink_{start,end}
* bd2ea818ecca - ovl: relax requirement for non null uuid of lower fs
* 19c221c3a6b5 - ovl: disable new features for deprecated upper fs
* 1c785e83823c - ovl: untangle copy up call chain
* b67871490628 - (ovl-fixes) ovl: fix recursive oi->lock in ovl_link()
* e39a0eb89ae4 - vfs: fix FIGETBSZ ioctl on an overlayfs file
* 431d5170b91d - ovl: fix error handling in ovl_verify_set_fh()
* 388b00daecb8 - kernel/acct.c: fix locking order when switching acct files

Pushed an update to overlayfs-next that contains most of these. I've dropped the kernel/acct.c one as I didn't yet have time to review it, and also the option fallback one as that is still under discussion.

Passes my tests, but more eyes and testing cannot hurt...

Thanks,
Miklos

author maintainer
#4 lore raw
Quoted 20 lines · Miklos Szeredi
Quoted 8 lines · Amir Goldstein

Miklos,

First patch removes pretty big ugliness. Second patch is just a matter of taste, so as you wish.

I'll use this opportunity to recap the queue of my proposed patches for v4.20, so you can see if you missed to consider any of them:

Quoted 9 lines

* 5401b99fcbb9 - (overlayfs-devel) ovl: abstract ovl_inode lock with a helper
* 8edcbcef4e55 - ovl: remove the 'locked' argument of ovl_nlink_{start,end}
* bd2ea818ecca - ovl: relax requirement for non null uuid of lower fs
* 19c221c3a6b5 - ovl: disable new features for deprecated upper fs
* 1c785e83823c - ovl: untangle copy up call chain
* b67871490628 - (ovl-fixes) ovl: fix recursive oi->lock in ovl_link()
* e39a0eb89ae4 - vfs: fix FIGETBSZ ioctl on an overlayfs file
* 431d5170b91d - ovl: fix error handling in ovl_verify_set_fh()
* 388b00daecb8 - kernel/acct.c: fix locking order when switching acct files

Quoted 6 lines

Pushed an update to overlayfs-next that contains most of these. I've dropped the kernel/acct.c one as I didn't yet have time to review it, and also the option fallback one as that is still under discussion.

Passes my tests, but more eyes and testing cannot hurt...

Tested.
Looks good.

Thanks,
Amir.