[PATCH 0/2] Overlay inode lock cleanups

linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/2] Overlay inode lock cleanups
@ 2018-10-18 15:37 Amir Goldstein
  2018-10-18 15:37 ` [PATCH 1/2] ovl: remove the 'locked' argument of ovl_nlink_{start,end} Amir Goldstein
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Amir Goldstein @ 2018-10-18 15:37 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-unionfs

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(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] ovl: remove the 'locked' argument of ovl_nlink_{start,end}
  2018-10-18 15:37 [PATCH 0/2] Overlay inode lock cleanups Amir Goldstein
@ 2018-10-18 15:37 ` Amir Goldstein
  2018-10-18 15:37 ` [PATCH 2/2] ovl: abstract ovl_inode lock with a helper Amir Goldstein
  2018-10-26 21:55 ` [PATCH 0/2] Overlay inode lock cleanups Miklos Szeredi
  2 siblings, 0 replies; 5+ messages in thread
From: Amir Goldstein @ 2018-10-18 15:37 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-unionfs

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 <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 21 +++++++++++----------
 fs/overlayfs/overlayfs.h |  4 ++--
 fs/overlayfs/util.c      | 28 ++++++++++++----------------
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index e1a55ecb7aba..e03f39550ccf 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -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:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index a3c0d9584312..b48ef22ef96b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -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);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 8cd37baf5d0a..d3a786006729 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -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)
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] ovl: abstract ovl_inode lock with a helper
  2018-10-18 15:37 [PATCH 0/2] Overlay inode lock cleanups Amir Goldstein
  2018-10-18 15:37 ` [PATCH 1/2] ovl: remove the 'locked' argument of ovl_nlink_{start,end} Amir Goldstein
@ 2018-10-18 15:37 ` Amir Goldstein
  2018-10-26 21:55 ` [PATCH 0/2] Overlay inode lock cleanups Miklos Szeredi
  2 siblings, 0 replies; 5+ messages in thread
From: Amir Goldstein @ 2018-10-18 15:37 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-unionfs

The abstraction improves code readabilty (to some).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h | 10 ++++++++++
 fs/overlayfs/util.c      | 25 +++++++++++++------------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index b48ef22ef96b..5e45cb3630a0 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -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);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index d3a786006729..7c01327b1852 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -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)
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 0/2] Overlay inode lock cleanups
  2018-10-18 15:37 [PATCH 0/2] Overlay inode lock cleanups Amir Goldstein
  2018-10-18 15:37 ` [PATCH 1/2] ovl: remove the 'locked' argument of ovl_nlink_{start,end} Amir Goldstein
  2018-10-18 15:37 ` [PATCH 2/2] ovl: abstract ovl_inode lock with a helper Amir Goldstein
@ 2018-10-26 21:55 ` Miklos Szeredi
  2018-10-29 20:38   ` Amir Goldstein
  2 siblings, 1 reply; 5+ messages in thread
From: Miklos Szeredi @ 2018-10-26 21:55 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: overlayfs

On Thu, Oct 18, 2018 at 5:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> 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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 0/2] Overlay inode lock cleanups
  2018-10-26 21:55 ` [PATCH 0/2] Overlay inode lock cleanups Miklos Szeredi
@ 2018-10-29 20:38   ` Amir Goldstein
  0 siblings, 0 replies; 5+ messages in thread
From: Amir Goldstein @ 2018-10-29 20:38 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs

On Sat, Oct 27, 2018 at 12:55 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Oct 18, 2018 at 5:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > 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
>
> 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-10-29 20:38 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-18 15:37 [PATCH 0/2] Overlay inode lock cleanups Amir Goldstein
2018-10-18 15:37 ` [PATCH 1/2] ovl: remove the 'locked' argument of ovl_nlink_{start,end} Amir Goldstein
2018-10-18 15:37 ` [PATCH 2/2] ovl: abstract ovl_inode lock with a helper Amir Goldstein
2018-10-26 21:55 ` [PATCH 0/2] Overlay inode lock cleanups Miklos Szeredi
2018-10-29 20:38   ` Amir Goldstein

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).